diff --git a/doc/manual/rl-next/build-dir-mandatory.md b/doc/manual/rl-next/build-dir-mandatory.md new file mode 100644 index 000000000..cb45a4315 --- /dev/null +++ b/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). diff --git a/misc/systemd/nix-daemon.conf.in b/misc/systemd/nix-daemon.conf.in index e7b264234..a0ddc4019 100644 --- a/misc/systemd/nix-daemon.conf.in +++ b/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 - diff --git a/src/libstore/include/nix/store/globals.hh b/src/libstore/include/nix/store/globals.hh index b5157b4f4..0ac689b55 100644 --- a/src/libstore/include/nix/store/globals.hh +++ b/src/libstore/include/nix/store/globals.hh @@ -697,14 +697,7 @@ public: Setting> 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 allowedImpureHostPrefixes{this, {}, "allowed-impure-host-deps", diff --git a/src/libstore/include/nix/store/local-store.hh b/src/libstore/include/nix/store/local-store.hh index 9a118fcc5..fd7e6fc36 100644 --- a/src/libstore/include/nix/store/local-store.hh +++ b/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, virtual LocalFSStoreConfig +struct LocalBuildStoreConfig : virtual LocalFSStoreConfig +{ + +private: + /** + Input for computing the build directory. See `getBuildDir()`. + */ + Setting> 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, virtual LocalFSStoreConfig, virtual LocalBuildStoreConfig { using LocalFSStoreConfig::LocalFSStoreConfig; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index e53cab2dc..0d2d96e61 100644 --- a/src/libstore/local-store.cc +++ b/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 LocalStore::Config::openStore() const { return make_ref(ref{shared_from_this()}); @@ -247,7 +257,7 @@ LocalStore::LocalStore(ref 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 config) txn.commit(); } - writeFile(schemaPath, fmt("%1%", nixSchemaVersion), 0666, true); + writeFile(schemaPath, fmt("%1%", nixSchemaVersion), 0666, FsSync::Yes); lockFile(globalLock.get(), ltRead, true); } diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 85b586373..fd62aa664 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -95,6 +95,11 @@ protected: */ Path topTmpDir; + /** + * The file descriptor of the temporary directory. + */ + AutoCloseFD tmpDirFd; + /** * The sort of derivation we are building. * @@ -300,9 +305,24 @@ protected: /** * 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::mapd_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; diff --git a/src/libutil/include/nix/util/file-system.hh b/src/libutil/include/nix/util/file-system.hh index 0121745ab..c45cb55aa 100644 --- a/src/libutil/include/nix/util/file-system.hh +++ b/src/libutil/include/nix/util/file-system.hh @@ -169,21 +169,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); -static inline void writeFile(const std::filesystem::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, 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); -static inline void writeFile(const std::filesystem::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, 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. */ diff --git a/tests/functional/build-remote-trustless-should-fail-0.sh b/tests/functional/build-remote-trustless-should-fail-0.sh index 3401de1b0..e79527d72 100755 --- a/tests/functional/build-remote-trustless-should-fail-0.sh +++ b/tests/functional/build-remote-trustless-should-fail-0.sh @@ -12,7 +12,6 @@ requiresUnprivilegedUserNamespaces [[ $busybox =~ busybox ]] || skipTest "no busybox" unset NIX_STORE_DIR -unset NIX_STATE_DIR # We first build a dependency of the derivation we eventually want to # build. diff --git a/tests/functional/build-remote-trustless.sh b/tests/functional/build-remote-trustless.sh index 9f91a91a9..6014b57bb 100644 --- a/tests/functional/build-remote-trustless.sh +++ b/tests/functional/build-remote-trustless.sh @@ -9,7 +9,6 @@ requiresUnprivilegedUserNamespaces [[ "$busybox" =~ busybox ]] || skipTest "no busybox" unset NIX_STORE_DIR -unset NIX_STATE_DIR remoteDir=$TEST_ROOT/remote diff --git a/tests/functional/build-remote.sh b/tests/functional/build-remote.sh index 765cd71b4..f396bc72e 100644 --- a/tests/functional/build-remote.sh +++ b/tests/functional/build-remote.sh @@ -8,7 +8,6 @@ requiresUnprivilegedUserNamespaces # Avoid store dir being inside sandbox build-dir unset NIX_STORE_DIR -unset NIX_STATE_DIR function join_by { local d=$1; shift; echo -n "$1"; shift; printf "%s" "${@/#/$d}"; } diff --git a/tests/functional/repl.sh b/tests/functional/repl.sh index 6db9e2d3d..bfe18c9e5 100755 --- a/tests/functional/repl.sh +++ b/tests/functional/repl.sh @@ -67,7 +67,7 @@ testRepl () { # Simple test, try building a drv testRepl # Same thing (kind-of), but with a remote store. -testRepl --store "$TEST_ROOT/store?real=$NIX_STORE_DIR" +testRepl --store "$TEST_ROOT/other-root?real=$NIX_STORE_DIR" # Remove ANSI escape sequences. They can prevent grep from finding a match. stripColors () { diff --git a/tests/functional/supplementary-groups.sh b/tests/functional/supplementary-groups.sh index 400333f7d..a667d3e99 100755 --- a/tests/functional/supplementary-groups.sh +++ b/tests/functional/supplementary-groups.sh @@ -14,7 +14,6 @@ execUnshare <&2") + + # Building in /tmp should fail for security reasons. + err = machine.fail("nix build --offline --store /tmp/nix --expr 'builtins.derivation { name = \"foo\"; system = \"x86_64-linux\"; builder = \"/foo\"; }' 2>&1") + assert "is world-writable" in err ''; }