Merge remote-tracking branch 'cve/fod-cves-2.29' into 2.29-maintenance

src/libstore/local-store.cc

@@ -247,7 +247,7 @@ LocalStore::LocalStore(ref<const Config> config)
     else if (curSchema == 0) { /* new store */
         curSchema = nixSchemaVersion;
         openDB(*state, true);
-        writeFile(schemaPath, fmt("%1%", curSchema), 0666, true);
+        writeFile(schemaPath, fmt("%1%", curSchema), 0666, FsSync::Yes);
     }
 
     else if (curSchema < nixSchemaVersion) {
@@ -298,7 +298,7 @@ LocalStore::LocalStore(ref<const Config> config)
             txn.commit();
         }
 
-        writeFile(schemaPath, fmt("%1%", nixSchemaVersion), 0666, true);
+        writeFile(schemaPath, fmt("%1%", nixSchemaVersion), 0666, FsSync::Yes);
 
         lockFile(globalLock.get(), ltRead, true);
     }

src/libstore/unix/build/derivation-builder.cc

@@ -129,6 +129,11 @@ private:
      */
     Path topTmpDir;
 
+    /**
+     * The file descriptor of the temporary directory.
+     */
+    AutoCloseFD tmpDirFd;
+
     /**
      * The path of the temporary directory in the sandbox.
      */
@@ -325,9 +330,24 @@ private:
 
     /**
      * Make a file owned by the builder.
+     *
+     * SAFETY: this function is prone to TOCTOU as it receives a path and not a descriptor.
+     * It's only safe to call in a child of a directory only visible to the owner.
      */
     void chownToBuilder(const Path & path);
 
+    /**
+     * Make a file owned by the builder addressed by its file descriptor.
+     */
+    void chownToBuilder(int fd, const Path & path);
+
+    /**
+     * Create a file in `tmpDir` owned by the builder.
+     */
+    void writeBuilderFile(
+        const std::string & name,
+        std::string_view contents);
+
     /**
      * Run the builder's process.
      */
@@ -895,7 +915,14 @@ void DerivationBuilderImpl::startBuilder()
     } else {
         tmpDir = topTmpDir;
     }
-    chownToBuilder(tmpDir);
+
+    /* The TOCTOU between the previous mkdir call and this open call is unavoidable due to
+       POSIX semantics.*/
+    tmpDirFd = AutoCloseFD{open(tmpDir.c_str(), O_RDONLY | O_NOFOLLOW | O_DIRECTORY)};
+    if (!tmpDirFd)
+        throw SysError("failed to open the build temporary directory descriptor '%1%'", tmpDir);
+
+    chownToBuilder(tmpDirFd.get(), tmpDir);
 
     for (auto & [outputName, status] : initialOutputs) {
         /* Set scratch path we'll actually use during the build.
@@ -1469,9 +1496,7 @@ void DerivationBuilderImpl::initTmpDir()
             } else {
                 auto hash = hashString(HashAlgorithm::SHA256, i.first);
                 std::string fn = ".attr-" + hash.to_string(HashFormat::Nix32, false);
-                Path p = tmpDir + "/" + fn;
+                writeBuilderFile(fn, rewriteStrings(i.second, inputRewrites));
-                writeFile(p, rewriteStrings(i.second, inputRewrites));
-                chownToBuilder(p);
                 env[i.first + "Path"] = tmpDirInSandbox + "/" + fn;
             }
         }
@@ -1580,11 +1605,9 @@ void DerivationBuilderImpl::writeStructuredAttrs()
 
         auto jsonSh = StructuredAttrs::writeShell(json);
 
-        writeFile(tmpDir + "/.attrs.sh", rewriteStrings(jsonSh, inputRewrites));
+        writeBuilderFile(".attrs.sh", rewriteStrings(jsonSh, inputRewrites));
-        chownToBuilder(tmpDir + "/.attrs.sh");
         env["NIX_ATTRS_SH_FILE"] = tmpDirInSandbox + "/.attrs.sh";
-        writeFile(tmpDir + "/.attrs.json", rewriteStrings(json.dump(), inputRewrites));
+        writeBuilderFile(".attrs.json", rewriteStrings(json.dump(), inputRewrites));
-        chownToBuilder(tmpDir + "/.attrs.json");
         env["NIX_ATTRS_JSON_FILE"] = tmpDirInSandbox + "/.attrs.json";
     }
 }
@@ -1838,6 +1861,24 @@ void setupSeccomp()
 #endif
 }
 
+void DerivationBuilderImpl::chownToBuilder(int fd, const Path & path)
+{
+    if (!buildUser) return;
+    if (fchown(fd, buildUser->getUID(), buildUser->getGID()) == -1)
+        throw SysError("cannot change ownership of file '%1%'", path);
+}
+
+void DerivationBuilderImpl::writeBuilderFile(
+    const std::string & name,
+    std::string_view contents)
+{
+    auto path = std::filesystem::path(tmpDir) / name;
+    AutoCloseFD fd{openat(tmpDirFd.get(), name.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC | O_EXCL | O_NOFOLLOW, 0666)};
+    if (!fd)
+        throw SysError("creating file %s", path);
+    writeFile(fd, path, contents);
+    chownToBuilder(fd.get(), path);
+}
 
 void DerivationBuilderImpl::runChild()
 {
@@ -3043,6 +3084,15 @@ void DerivationBuilderImpl::checkOutputs(const std::map<std::string, ValidPathIn
 void DerivationBuilderImpl::deleteTmpDir(bool force)
 {
     if (topTmpDir != "") {
+        /* As an extra precaution, even in the event of `deletePath` failing to
+         * clean up, the `tmpDir` will be chowned as if we were to move
+         * it inside the Nix store.
+         *
+         * This hardens against an attack which smuggles a file descriptor
+         * to make use of the temporary directory.
+         */
+        chmod(topTmpDir.c_str(), 0000);
+
         /* Don't keep temporary directories for builtins because they
            might have privileged stuff (like a copy of netrc). */
         if (settings.keepFailed && !force && !drv.isBuiltin()) {

src/libutil/file-content-address.cc

@@ -93,7 +93,7 @@ void restorePath(
 {
     switch (method) {
     case FileSerialisationMethod::Flat:
-        writeFile(path, source, 0666, startFsync);
+        writeFile(path, source, 0666, startFsync ? FsSync::Yes : FsSync::No);
         break;
     case FileSerialisationMethod::NixArchive:
         restorePath(path, source, startFsync);

src/libutil/file-system.cc

@@ -303,7 +303,7 @@ void readFile(const Path & path, Sink & sink, bool memory_map)
 }
 
 
-void writeFile(const Path & path, std::string_view s, mode_t mode, bool sync)
+void writeFile(const Path & path, std::string_view s, mode_t mode, FsSync sync)
 {
     AutoCloseFD fd = toDescriptor(open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT
 // TODO
@@ -313,22 +313,29 @@ void writeFile(const Path & path, std::string_view s, mode_t mode, bool sync)
        , mode));
     if (!fd)
         throw SysError("opening file '%1%'", path);
-    try {
+
-        writeFull(fd.get(), s);
+    writeFile(fd, path, s, mode, sync);
-    } catch (Error & e) {
+
-        e.addTrace({}, "writing file '%1%'", path);
+    /* Close explicitly to propagate the exceptions. */
-        throw;
-    }
-    if (sync)
-        fd.fsync();
-    // Explicitly close to make sure exceptions are propagated.
     fd.close();
-    if (sync)
-        syncParent(path);
 }
 
+void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode, FsSync sync)
+{
+    assert(fd);
+    try {
+        writeFull(fd.get(), s);
 
-void writeFile(const Path & path, Source & source, mode_t mode, bool sync)
+        if (sync == FsSync::Yes)
+            fd.fsync();
+
+    } catch (Error & e) {
+        e.addTrace({}, "writing file '%1%'", origPath);
+        throw;
+    }
+}
+
+void writeFile(const Path & path, Source & source, mode_t mode, FsSync sync)
 {
     AutoCloseFD fd = toDescriptor(open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT
 // TODO
@@ -352,11 +359,11 @@ void writeFile(const Path & path, Source & source, mode_t mode, bool sync)
         e.addTrace({}, "writing file '%1%'", path);
         throw;
     }
-    if (sync)
+    if (sync == FsSync::Yes)
         fd.fsync();
     // Explicitly close to make sure exceptions are propagated.
     fd.close();
-    if (sync)
+    if (sync == FsSync::Yes)
         syncParent(path);
 }
 
@@ -419,7 +426,8 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path,
 #ifndef _WIN32
     checkInterrupt();
 
-    std::string name(baseNameOf(path.native()));
+    std::string name(path.filename());
+    assert(name != "." && name != ".." && !name.empty());
 
     struct stat st;
     if (fstatat(parentfd, name.c_str(), &st,
@@ -460,7 +468,7 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path,
                 throw SysError("chmod %1%", path);
         }
 
-        int fd = openat(parentfd, path.c_str(), O_RDONLY);
+        int fd = openat(parentfd, name.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
         if (fd == -1)
             throw SysError("opening directory %1%", path);
         AutoCloseDir dir(fdopendir(fd));
@@ -472,7 +480,7 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path,
             checkInterrupt();
             std::string childName = dirent->d_name;
             if (childName == "." || childName == "..") continue;
-            _deletePath(dirfd(dir.get()), path + "/" + childName, bytesFreed);
+            _deletePath(dirfd(dir.get()), path / childName, bytesFreed);
         }
         if (errno) throw SysError("reading directory %1%", path);
     }
@@ -490,14 +498,13 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path,
 
 static void _deletePath(const std::filesystem::path & path, uint64_t & bytesFreed)
 {
-    Path dir = dirOf(path.string());
+    assert(path.is_absolute());
-    if (dir == "")
+    assert(path.parent_path() != path);
-        dir = "/";
 
-    AutoCloseFD dirfd = toDescriptor(open(dir.c_str(), O_RDONLY));
+    AutoCloseFD dirfd = toDescriptor(open(path.parent_path().string().c_str(), O_RDONLY));
     if (!dirfd) {
         if (errno == ENOENT) return;
-        throw SysError("opening directory '%1%'", path);
+        throw SysError("opening directory %s", path.parent_path());
     }
 
     _deletePath(dirfd.get(), path, bytesFreed);

src/libutil/include/nix/util/file-system.hh

@@ -175,21 +175,27 @@ std::string readFile(const Path & path);
 std::string readFile(const std::filesystem::path & path);
 void readFile(const Path & path, Sink & sink, bool memory_map = true);
 
+enum struct FsSync { Yes, No };
+
 /**
  * Write a string to a file.
  */
-void writeFile(const Path & path, std::string_view s, mode_t mode = 0666, bool sync = false);
+void writeFile(const Path & path, std::string_view s, mode_t mode = 0666, FsSync sync = FsSync::No);
-static inline void writeFile(const std::filesystem::path & path, std::string_view s, mode_t mode = 0666, bool sync = false)
+
+static inline void writeFile(const std::filesystem::path & path, std::string_view s, mode_t mode = 0666, FsSync sync = FsSync::No)
 {
     return writeFile(path.string(), s, mode, sync);
 }
 
-void writeFile(const Path & path, Source & source, mode_t mode = 0666, bool sync = false);
+void writeFile(const Path & path, Source & source, mode_t mode = 0666, FsSync sync = FsSync::No);
-static inline void writeFile(const std::filesystem::path & path, Source & source, mode_t mode = 0666, bool sync = false)
+
+static inline void writeFile(const std::filesystem::path & path, Source & source, mode_t mode = 0666, FsSync sync = FsSync::No)
 {
     return writeFile(path.string(), source, mode, sync);
 }
 
+void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode = 0666, FsSync sync = FsSync::No);
+
 /**
  * Flush a path's parent directory to disk.
  */