Backport Lix CVE fixes to Nix 2.3 #5

tazjin · 2025-06-29T10:24:54Z

This backports the most important CVE fixes, but not all of them.

Included are fixes for:

Not included are fixes for:

We might backport these separately, but I don't think we'll go all the way to the pasta stuff in this fork. We'll see.

This is useful for certain error recovery paths (no pun intended) that does not thread through the original path name. Change-Id: I2d800740cb4f9912e64c923120d3f977c58ccb7e Signed-off-by: Raito Bezarius <raito@lix.systems>

We now keep around a proper AutoCloseFD around the temporary directory which we plan to use for openat operations and avoiding the build directory being swapped out while we are doing something else. Change-Id: I18d387b0f123ebf2d20c6405cd47ebadc5505f2a Signed-off-by: Raito Bezarius <raito@lix.systems>

We use it immediately for the build temporary directory. Change-Id: I180193c63a2b98721f5fb8e542c4e39c099bb947 Signed-off-by: Raito Bezarius <raito@lix.systems>

- call close explicitly in writeFile to prevent the close exception from being ignored - fsync after writing schema file to flush data to disk - fsync schema file parent to flush metadata to disk NixOS#7064

`writeFile` lose its `sync` boolean flag to make things simpler. A new `writeFileAndSync` function is created and all call sites are converted to it. Change-Id: Ib871a5283a9c047db1e4fe48a241506e4aab9192 Signed-off-by: Raito Bezarius <raito@lix.systems>

This ensures that `passAsFile` data is created inside the expected temporary build directory by `openat()` from the parent directory file descriptor. Fixes CVE-2025-52993. Change-Id: Ie5273446c4a19403088d0389ae8e3f473af8879a Signed-off-by: Raito Bezarius <raito@lix.systems>

Suppose I have a path /nix/store/[hash]-[name]/a/a/a/a/a/[...]/a, long enough that everything after "/nix/store/" is longer than 4096 (MAX_PATH) bytes. Nix will happily allow such a path to be inserted into the store, because it doesn't look at all the nested structure. It just cares about the /nix/store/[hash]-[name] part. But, when the path is deleted, we encounter a problem. Nix will move the path to /nix/store/trash, but then when it's trying to recursively delete the trash directory, it will at some point try to unlink /nix/store/trash/[hash]-[name]/a/a/a/a/a/[...]/a. This will fail, because the path is too long. After this has failed, any store deletion operation will never work again, because Nix needs to delete the trash directory before recreating it to move new things to it. (I assume this is because otherwise a path being deleted could already exist in the trash, and then moving it would fail.) This means that if I can trick somebody into just fetching a tarball containing a path of the right length, they won't be able to delete store paths or garbage collect ever again, until the offending path is manually removed from /nix/store/trash. (And even fixing this manually is quite difficult if you don't understand the issue, because the absolute path that Nix says it failed to remove is also too long for rm(1).) This patch fixes the issue by making Nix's recursive delete operation use unlinkat(2). This function takes a relative path and a directory file descriptor. We ensure that the relative path is always just the name of the directory entry, and therefore its length will never exceed 255 bytes. This means that it will never even come close to AX_PATH, and Nix will therefore be able to handle removing arbitrarily deep directory hierachies. Since the directory file descriptor is used for recursion after being used in readDirectory, I made a variant of readDirectory that takes an already open directory stream, to avoid the directory being opened multiple times. As we have seen from this issue, the less we have to interact with paths, the better, and so it's good to reuse file descriptors where possible. I left _deletePath as succeeding even if the parent directory doesn't exist, even though that feels wrong to me, because without that early return, the linux-sandbox test failed. Reported-by: Alyssa Ross <hi@alyssa.is> Thanks-to: Puck Meerburg <puck@puckipedia.com> Tested-by: Puck Meerburg <puck@puckipedia.com> Reviewed-by: Puck Meerburg <puck@puckipedia.com>

…tors Also remove an erroneous comment.

…irfds When calling `_deletePath` with a parent file descriptor, `openat` is made effective by using relative paths to the directory file descriptor. To avoid the problem, the signature is changed to resist misuse with an assert in the prologue of the function. Fixes CVE-2025-46415. Change-Id: I6b3fc766bad2afe54dc27d47d1df3873e188de96 Signed-off-by: Raito Bezarius <raito@lix.systems>

sternenseemann

Just found the one mistake when backporting the Lix patches. I've also added some confusions I've had, but I guess they are to some extent also directed at Raito.

Also looks like this would break the build on darwin which isn't super urgent for depot at least.

sternenseemann · 2025-06-30T10:06:34Z

src/libutil/util.cc

+        return readLink(fmt("/proc/self/fd/%1%", fd).c_str());
+    } catch (...) {
+    }
+#elif defined (HAVE_F_GETPATH) && HAVE_F_GETPATH


This CPP symbol is set by meson in Lix, so I think this is effectively dead code here unless we add it to the autoconf machinery.

sternenseemann · 2025-06-30T10:21:13Z

src/libutil/util.hh

+void writeFile(AutoCloseFD & fd, const std::string& s, mode_t mode = 0666);
+
+/* Write a string to a file and flush the file and its parent directory to disk. */
+void writeFileAndSync(const Path & path, const std::string& s, mode_t mode = 0666);


Do you know the significance of writeFileUninterruptible? Is that something we also need?

Apparently related to cgroups in Lix, unnecessary.

sternenseemann · 2025-06-30T10:23:48Z

src/libutil/util.cc

+void writeFile(AutoCloseFD & fd, const std::string& s, mode_t mode)
+{
+    assert(fd);
    writeFull(fd.get(), s);


Why isn't the fd closed in here, but in the wrapping function? Is the calling function entitled to keep using the fd? A previous specifcally started closing the fd here to “propagate exceptions”, so I'm confused why it's being reverted here…

sternenseemann · 2025-06-30T10:25:45Z

src/libutil/util.cc

+            throw SysError("opening file '%1%'", path);
+
+        writeFile(fd, s, mode);
+        fd.fsync();


couldn't closeForWrite be used here?

sternenseemann · 2025-06-30T10:34:46Z

src/libutil/util.cc

-        for (auto & i : readDirectory(path))
-            _deletePath(path + "/" + i.name, bytesFreed);
+        int fd = openat(parentfd, name.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+        if (fd = -1)


Suggested change

if (fd = -1)

if (fd == -1)

sternenseemann · 2025-06-30T10:36:25Z

src/libutil/util.cc



-static void _deletePath(int parentfd, const Path & path, unsigned long long & bytesFreed)
+static void _deletePath(int parentfd, const Path& name, unsigned long long & bytesFreed)


Why are we continuing to use Path here?

sternenseemann · 2025-06-30T10:40:12Z

src/libutil/util.cc

-        for (auto & i : readDirectory(dir.get(), path))
-            _deletePath(dirfd(dir.get()), path + "/" + i.name, bytesFreed);
+            throw SysError("opening directory '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd));
+        for (auto & i : readDirectory(dir.get(), name))


Also here we don't have a flag for interruptibility.

(Apparently related to cgroups in Lix, unnecessary.)

sternenseemann · 2025-06-30T10:40:26Z

src/libutil/util.cc

-            _deletePath(dirfd(dir.get()), path + "/" + i.name, bytesFreed);
+            throw SysError("opening directory '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd));
+        for (auto & i : readDirectory(dir.get(), name))
+            _deletePath(dirfd(dir.get()), i.name, bytesFreed);


And here…

(Apparently related to cgroups in Lix, unnecessary.)

sternenseemann · 2025-06-30T11:07:36Z

Seems like backporting the following commits would also be a good idea, though I'm not sure whether this is entangled with pasta somehow?

05eba34893122d395774b4ee5eaf4c03ae588ea0 libstore: don't default build-dir to temp-dir
ea51a906a7adb917f0fd57bc5d665be2e5e1badd libstore/build: automatic clean up of unsuccessfully built scratch outputs

The following commits aren't backported, but related to pasta (and thus future work):

500a7406a0f6fe2d9132da70d10688cfc7fa598d libutil: make RunningProgram more useful
7f127054bec18da811bf3364909870f7a54f6b8d libutil: add generic redirections runProgram2
582f775ac358f9da682f707a3f58f228f7fdaed8 libutil: add capability support to runProgram2
6a61eea281de2c4d7d2b4f375511db0dacfec5ec libstore: use pasta for FODs if available

sternenseemann · 2025-07-07T21:39:13Z

Seems like backporting the following commits would also be a good idea, though I'm not sure whether this is entangled with pasta somehow?
* 05eba34893122d395774b4ee5eaf4c03ae588ea0 libstore: don't default build-dir to temp-dir

* ea51a906a7adb917f0fd57bc5d665be2e5e1badd libstore/build: automatic clean up of unsuccessfully built scratch outputs

Seems like this needs some extra care: https://lix.systems/blog/2025-06-27-lix-critical-bug/

sternenseemann · 2025-11-16T14:18:23Z

Raito Bezarius and others added 9 commits June 29, 2025 09:03

libutil: guess or invent a path from file descriptors

c7745b2

This is useful for certain error recovery paths (no pun intended) that does not thread through the original path name. Change-Id: I2d800740cb4f9912e64c923120d3f977c58ccb7e Signed-off-by: Raito Bezarius <raito@lix.systems>

libstore: chown to builder variant for file descriptors

e47a472

We use it immediately for the build temporary directory. Change-Id: I180193c63a2b98721f5fb8e542c4e39c099bb947 Signed-off-by: Raito Bezarius <raito@lix.systems>

Improve durability of schema version file writes

8251672

- call close explicitly in writeFile to prevent the close exception from being ignored - fsync after writing schema file to flush data to disk - fsync schema file parent to flush metadata to disk NixOS#7064

deletePath(): Return ENFILE instead of EBADF when out of file descrip…

2e04bd1

…tors Also remove an erroneous comment.

tazjin requested a review from sternenseemann June 30, 2025 09:12

tazjin mentioned this pull request Jun 30, 2025

nixVersions.nix_2_3: add knownVulnerabilities NixOS/nixpkgs#420974

Merged

13 tasks

sternenseemann suggested changes Jun 30, 2025

View reviewed changes

wolfgangwalther mentioned this pull request Jul 24, 2025

ci/pinned: update NixOS/nixpkgs#427437

Merged

1 task



		static void _deletePath(int parentfd, const Path & path, unsigned long long & bytesFreed)
		static void _deletePath(int parentfd, const Path& name, unsigned long long & bytesFreed)

Backport Lix CVE fixes to Nix 2.3 #5

Are you sure you want to change the base?

Backport Lix CVE fixes to Nix 2.3 #5

Uh oh!

Conversation

tazjin commented Jun 29, 2025

Uh oh!

sternenseemann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sternenseemann commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sternenseemann commented Jul 7, 2025

Uh oh!

sternenseemann commented Nov 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sternenseemann commented Jun 30, 2025 •

edited

Loading