Merge remote-tracking branch 'cve/fod-cves-master'

Author: edolstra

doc/manual/rl-next/build-dir-mandatory.md

@@ -0,0 +1,9 @@
+---
+synopsis: "`build-dir` no longer defaults to `$TMPDIR`"
+---
+
+The directory in which temporary build directories are created no longer defaults
+to `TMPDIR` or `/tmp`, to avoid builders making their directories
+world-accessible. This behavior allowed escaping the build sandbox and can
+cause build impurities even when not used maliciously. We now default to `builds`
+in `NIX_STATE_DIR` (which is `/nix/var/nix/builds` in the default configuration).

misc/systemd/nix-daemon.conf.in

@@ -1 +1,2 @@
-d @localstatedir@/nix/daemon-socket 0755 root root - -
+d @localstatedir@/nix/daemon-socket 0755 root root -  -
+d @localstatedir@/nix/builds        0755 root root 7d -

src/libstore/include/nix/store/globals.hh

@@ -697,14 +697,7 @@ public:
 
     Setting<std::optional<Path>> buildDir{this, std::nullopt, "build-dir",
         R"(
-            The directory on the host, in which derivations' temporary build directories are created.
-
-            If not set, Nix uses the system temporary directory indicated by the `TMPDIR` environment variable.
-            Note that builds are often performed by the Nix daemon, so its `TMPDIR` is used, and not that of the Nix command line interface.
-
-            This is also the location where [`--keep-failed`](@docroot@/command-ref/opt-common.md#opt-keep-failed) leaves its files.
-
-            If Nix runs without sandbox, or if the platform does not support sandboxing with bind mounts (e.g. macOS), then the [`builder`](@docroot@/language/derivations.md#attr-builder)'s environment contains this directory instead of the virtual location [`sandbox-build-dir`](#conf-sandbox-build-dir).
+            Override the `build-dir` store setting for all stores that have this setting.
         )"};
 
     Setting<PathSet> allowedImpureHostPrefixes{this, {}, "allowed-impure-host-deps",

src/libstore/include/nix/store/local-store.hh

@@ -34,7 +34,39 @@ struct OptimiseStats
     uint64_t bytesFreed = 0;
 };
 
-struct LocalStoreConfig : std::enable_shared_from_this<LocalStoreConfig>, virtual LocalFSStoreConfig
+struct LocalBuildStoreConfig : virtual LocalFSStoreConfig
+{
+
+private:
+    /**
+      Input for computing the build directory. See `getBuildDir()`.
+     */
+     Setting<std::optional<Path>> buildDir{this, std::nullopt, "build-dir",
+        R"(
+            The directory on the host, in which derivations' temporary build directories are created.
+
+            If not set, Nix will use the `builds` subdirectory of its configured state directory.
+
+            Note that builds are often performed by the Nix daemon, so its `build-dir` applies.
+
+            Nix will create this directory automatically with suitable permissions if it does not exist.
+            Otherwise its permissions must allow all users to traverse the directory (i.e. it must have `o+x` set, in unix parlance) for non-sandboxed builds to work correctly.
+
+            This is also the location where [`--keep-failed`](@docroot@/command-ref/opt-common.md#opt-keep-failed) leaves its files.
+
+            If Nix runs without sandbox, or if the platform does not support sandboxing with bind mounts (e.g. macOS), then the [`builder`](@docroot@/language/derivations.md#attr-builder)'s environment will contain this directory, instead of the virtual location [`sandbox-build-dir`](#conf-sandbox-build-dir).
+
+            > **Warning**
+            >
+            > `build-dir` must not be set to a world-writable directory.
+            > Placing temporary build directories in a world-writable place allows other users to access or modify build data that is currently in use.
+            > This alone is merely an impurity, but combined with another factor this has allowed malicious derivations to escape the build sandbox.
+        )"};
+public:
+    Path getBuildDir() const;
+};
+
+struct LocalStoreConfig : std::enable_shared_from_this<LocalStoreConfig>, virtual LocalFSStoreConfig, virtual LocalBuildStoreConfig
 {
     using LocalFSStoreConfig::LocalFSStoreConfig;
 

src/libstore/local-store.cc

@@ -77,6 +77,16 @@ std::string LocalStoreConfig::doc()
         ;
 }
 
+Path LocalBuildStoreConfig::getBuildDir() const
+{
+    return
+        settings.buildDir.get().has_value()
+        ? *settings.buildDir.get()
+        : buildDir.get().has_value()
+        ? *buildDir.get()
+        : stateDir.get() + "/builds";
+}
+
 ref<Store> LocalStore::Config::openStore() const
 {
     return make_ref<LocalStore>(ref{shared_from_this()});
@@ -247,7 +257,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 +308,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

@@ -95,6 +95,11 @@ class DerivationBuilderImpl : public DerivationBuilder, public DerivationBuilder
      */
     Path topTmpDir;
 
+    /**
+     * The file descriptor of the temporary directory.
+     */
+    AutoCloseFD tmpDirFd;
+
     /**
      * The sort of derivation we are building.
      *
@@ -300,9 +305,24 @@ class DerivationBuilderImpl : public DerivationBuilder, public DerivationBuilder
 
     /**
      * 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.
      */
@@ -678,6 +698,18 @@ static void handleChildException(bool sendException)
     }
 }
 
+static bool checkNotWorldWritable(std::filesystem::path path)
+{
+    while (true) {
+        auto st = lstat(path);
+        if (st.st_mode & S_IWOTH)
+            return false;
+        if (path == path.parent_path()) break;
+        path = path.parent_path();
+    }
+    return true;
+}
+
 void DerivationBuilderImpl::startBuilder()
 {
     /* Make sure that no other processes are executing under the
@@ -705,12 +737,26 @@ void DerivationBuilderImpl::startBuilder()
         throw BuildError(msg);
     }
 
+    auto buildDir = getLocalStore(store).config->getBuildDir();
+
+    createDirs(buildDir);
+
+    if (buildUser && !checkNotWorldWritable(buildDir))
+        throw Error("Path %s or a parent directory is world-writable or a symlink. That's not allowed for security.", buildDir);
+
     /* Create a temporary directory where the build will take
        place. */
-    topTmpDir = createTempDir(settings.buildDir.get().value_or(""), "nix-build-" + std::string(drvPath.name()), 0700);
+    topTmpDir = createTempDir(buildDir, "nix-build-" + std::string(drvPath.name()), 0700);
     setBuildTmpDir();
     assert(!tmpDir.empty());
-    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.
@@ -876,7 +922,7 @@ DerivationBuilderImpl::PathsInChroot DerivationBuilderImpl::getPathsInSandbox()
                 store.computeFSClosure(store.toStorePath(i.second.source).first, closure);
         } catch (InvalidPath & e) {
         } catch (Error & e) {
-            e.addTrace({}, "while processing 'sandbox-paths'");
+            e.addTrace({}, "while processing sandbox path '%s'", i.second.source);
             throw;
         }
     for (auto & i : closure) {
@@ -1049,13 +1095,10 @@ void DerivationBuilderImpl::initEnv()
             } else {
                 auto hash = hashString(HashAlgorithm::SHA256, i.first);
                 std::string fn = ".attr-" + hash.to_string(HashFormat::Nix32, false);
-                Path p = tmpDir + "/" + fn;
-                writeFile(p, rewriteStrings(i.second, inputRewrites));
-                chownToBuilder(p);
+                writeBuilderFile(fn, rewriteStrings(i.second, inputRewrites));
                 env[i.first + "Path"] = tmpDirInSandbox() + "/" + fn;
             }
         }
-
     }
 
     /* For convenience, set an environment pointing to the top build
@@ -1130,11 +1173,9 @@ void DerivationBuilderImpl::writeStructuredAttrs()
 
         auto jsonSh = StructuredAttrs::writeShell(json);
 
-        writeFile(tmpDir + "/.attrs.sh", rewriteStrings(jsonSh, inputRewrites));
-        chownToBuilder(tmpDir + "/.attrs.sh");
+        writeBuilderFile(".attrs.sh", rewriteStrings(jsonSh, inputRewrites));
         env["NIX_ATTRS_SH_FILE"] = tmpDirInSandbox() + "/.attrs.sh";
-        writeFile(tmpDir + "/.attrs.json", rewriteStrings(json.dump(), inputRewrites));
-        chownToBuilder(tmpDir + "/.attrs.json");
+        writeBuilderFile(".attrs.json", rewriteStrings(json.dump(), inputRewrites));
         env["NIX_ATTRS_JSON_FILE"] = tmpDirInSandbox() + "/.attrs.json";
     }
 }
@@ -1255,6 +1296,25 @@ void DerivationBuilderImpl::chownToBuilder(const Path & path)
         throw SysError("cannot change ownership of '%1%'", path);
 }
 
+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()
 {
     /* Warning: in the child we should absolutely not make any SQLite
@@ -2063,6 +2123,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

@@ -304,7 +304,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
@@ -314,22 +314,29 @@ void writeFile(const Path & path, std::string_view s, mode_t mode, bool sync)
        , mode));
     if (!fd)
         throw SysError("opening file '%1%'", path);
+
+    writeFile(fd, path, s, mode, sync);
+
+    /* Close explicitly to propagate the exceptions. */
+    fd.close();
+}
+
+void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode, FsSync sync)
+{
+    assert(fd);
     try {
         writeFull(fd.get(), s);
+
+        if (sync == FsSync::Yes)
+            fd.fsync();
+
     } catch (Error & e) {
-        e.addTrace({}, "writing file '%1%'", path);
+        e.addTrace({}, "writing file '%1%'", origPath);
         throw;
     }
-    if (sync)
-        fd.fsync();
-    // Explicitly close to make sure exceptions are propagated.
-    fd.close();
-    if (sync)
-        syncParent(path);
 }
 
-
-void writeFile(const Path & path, Source & source, mode_t mode, bool sync)
+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
@@ -353,11 +360,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);
 }
 
@@ -435,7 +442,8 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path,
     }
 #endif
 
-    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,
@@ -476,7 +484,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));
@@ -488,7 +496,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, ex MOUNTEDPATHS_ARG);
+            _deletePath(dirfd(dir.get()), path / childName, bytesFreed, ex MOUNTEDPATHS_ARG);
         }
         if (errno) throw SysError("reading directory %1%", path);
     }
@@ -513,14 +521,13 @@ static void _deletePath(Descriptor parentfd, const std::filesystem::path & path,
 
 static void _deletePath(const std::filesystem::path & path, uint64_t & bytesFreed MOUNTEDPATHS_PARAM)
 {
-    Path dir = dirOf(path.string());
-    if (dir == "")
-        dir = "/";
+    assert(path.is_absolute());
+    assert(path.parent_path() != path);
 
-    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());
     }
 
     std::exception_ptr ex;