diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index c6e3af456..c5489444e 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -187,7 +187,7 @@ void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd) txn.commit(); } - writeFile(schemaPath, fmt("%d", nixCASchemaVersion), 0666, true); + writeFile(schemaPath, fmt("%d", nixCASchemaVersion), 0666, FsSync::Yes); lockFile(lockFd.get(), ltRead, true); } } @@ -345,7 +345,7 @@ LocalStore::LocalStore( 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) { @@ -394,7 +394,7 @@ LocalStore::LocalStore( 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/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index f8824e9ce..82c79f361 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -526,7 +526,14 @@ void LocalDerivationGoal::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. @@ -1110,9 +1117,7 @@ void LocalDerivationGoal::initTmpDir() { } 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; } } @@ -1217,11 +1222,9 @@ void LocalDerivationGoal::writeStructuredAttrs() auto jsonSh = writeStructuredAttrsShell(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"; } } @@ -1730,6 +1733,24 @@ void setupSeccomp() #endif } +void LocalDerivationGoal::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 LocalDerivationGoal::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 LocalDerivationGoal::runChild() { @@ -3006,6 +3027,15 @@ void LocalDerivationGoal::checkOutputs(const std::mapisBuiltin()) { diff --git a/src/libstore/unix/build/local-derivation-goal.hh b/src/libstore/unix/build/local-derivation-goal.hh index bf25cf2a6..69c517c4a 100644 --- a/src/libstore/unix/build/local-derivation-goal.hh +++ b/src/libstore/unix/build/local-derivation-goal.hh @@ -37,6 +37,11 @@ struct LocalDerivationGoal : public DerivationGoal */ Path topTmpDir; + /** + * The file descriptor of the temporary directory. + */ + AutoCloseFD tmpDirFd; + /** * The path of the temporary directory in the sandbox. */ @@ -232,9 +237,24 @@ struct LocalDerivationGoal : public DerivationGoal /** * 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); + int getChildStatus() override; /** diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 8ec38e73b..554214d66 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -247,7 +247,7 @@ void readFile(const Path & path, Sink & sink) } -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 @@ -257,22 +257,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); - } catch (Error & e) { - e.addTrace({}, "writing file '%1%'", path); - throw; - } - if (sync) - fd.fsync(); - // Explicitly close to make sure exceptions are propagated. + + writeFile(fd, path, s, mode, sync); + + /* Close explicitly to propagate the exceptions. */ 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 @@ -296,11 +303,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); } @@ -318,7 +325,8 @@ static void _deletePath(Descriptor parentfd, const fs::path & path, uint64_t & b #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, @@ -359,7 +367,7 @@ static void _deletePath(Descriptor parentfd, const fs::path & path, uint64_t & b 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)); @@ -371,7 +379,7 @@ static void _deletePath(Descriptor parentfd, const fs::path & path, uint64_t & b 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); } @@ -389,14 +397,13 @@ static void _deletePath(Descriptor parentfd, const fs::path & path, uint64_t & b static void _deletePath(const fs::path & path, uint64_t & bytesFreed) { - 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()); } _deletePath(dirfd.get(), path, bytesFreed); diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index ed1112c7e..32b84456d 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -148,12 +148,16 @@ Descriptor openDirectory(const std::filesystem::path & path); std::string readFile(const Path & path); void readFile(const Path & path, Sink & sink); +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); -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); + +void writeFile(AutoCloseFD & fd, const Path & origPath, std::string_view s, mode_t mode = 0666, FsSync sync = FsSync::No); /** * Flush a file's parent directory to disk