From 533cced249eb2c781496551cb4761937bbe59d12 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Tue, 18 Nov 2025 00:29:23 +0300 Subject: [PATCH 1/4] libutil: Add requireCString, make renderUrlPathEnsureLegal error on NUL bytes better Same utility as in lix's change I3caf476e59dcb7899ac5a3d83dfa3fb7ceaaabf0. Co-authored-by: eldritch horrors --- src/libutil/include/nix/util/strings.hh | 6 ++++++ src/libutil/strings.cc | 11 +++++++++++ src/libutil/url.cc | 7 +++++-- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/libutil/include/nix/util/strings.hh b/src/libutil/include/nix/util/strings.hh index da6decc31..da8409e6a 100644 --- a/src/libutil/include/nix/util/strings.hh +++ b/src/libutil/include/nix/util/strings.hh @@ -166,4 +166,10 @@ public: } }; +/** + * Check that the string does not contain any NUL bytes and return c_str(). + * @throws Error if str contains '\0' bytes. + */ +const char * requireCString(const std::string & str); + } // namespace nix diff --git a/src/libutil/strings.cc b/src/libutil/strings.cc index c0c3d6602..91a0f73ec 100644 --- a/src/libutil/strings.cc +++ b/src/libutil/strings.cc @@ -5,6 +5,7 @@ #include "nix/util/strings-inline.hh" #include "nix/util/os-string.hh" #include "nix/util/error.hh" +#include "nix/util/util.hh" namespace nix { @@ -152,4 +153,14 @@ std::string optionalBracket(std::string_view prefix, std::string_view content, s return result; } +const char * requireCString(const std::string & s) +{ + if (std::memchr(s.data(), '\0', s.size())) [[unlikely]] { + using namespace std::string_view_literals; + auto str = replaceStrings(s, "\0"sv, "␀"sv); + throw Error("string '%s' with null (\\0) bytes used where it's not allowed", str); + } + return s.c_str(); +} + } // namespace nix diff --git a/src/libutil/url.cc b/src/libutil/url.cc index 72042901c..0a8b64528 100644 --- a/src/libutil/url.cc +++ b/src/libutil/url.cc @@ -327,8 +327,11 @@ Path renderUrlPathEnsureLegal(const std::vector & urlPath) /* This is only really valid for UNIX. Windows has more restrictions. */ if (comp.contains('/')) throw BadURL("URL path component '%s' contains '/', which is not allowed in file names", comp); - if (comp.contains(char(0))) - throw BadURL("URL path component '%s' contains NUL byte which is not allowed", comp); + if (comp.contains(char(0))) { + using namespace std::string_view_literals; + auto str = replaceStrings(comp, "\0"sv, "␀"sv); + throw BadURL("URL path component '%s' contains NUL byte which is not allowed", str); + } } return concatStringsSep("/", urlPath); From fa380e09918893ca7d771cce816ab742e92d8435 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 20 Nov 2025 00:28:23 +0300 Subject: [PATCH 2/4] libutil: Make RestoreSink use *at system calls on UNIX This is necessary to ban symlink following. It can be considered a defense in depth against issues similar to CVE-2024-45593. By slightly changing the API in a follow-up commit we will be able to mitigate the symlink following issue for good. --- src/libutil/fs-sink.cc | 42 ++++++++++++++++++++++++- src/libutil/include/nix/util/fs-sink.hh | 12 +++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/libutil/fs-sink.cc b/src/libutil/fs-sink.cc index 45ef57a9f..5b5f73067 100644 --- a/src/libutil/fs-sink.cc +++ b/src/libutil/fs-sink.cc @@ -73,8 +73,34 @@ static std::filesystem::path append(const std::filesystem::path & src, const Can void RestoreSink::createDirectory(const CanonPath & path) { auto p = append(dstPath, path); + +#ifndef _WIN32 + if (dirFd) { + if (path.isRoot()) + /* Trying to create a directory that we already have a file descriptor for. */ + throw Error("path '%s' already exists", p.string()); + + if (::mkdirat(dirFd.get(), path.rel_c_str(), 0777) == -1) + throw SysError("creating directory '%s'", p.string()); + + return; + } +#endif + if (!std::filesystem::create_directory(p)) throw Error("path '%s' already exists", p.string()); + +#ifndef _WIN32 + if (path.isRoot()) { + assert(!dirFd); // Handled above + + /* Open directory for further *at operations relative to the sink root + directory. */ + dirFd = open(p.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + if (!dirFd) + throw SysError("creating directory '%1%'", p.string()); + } +#endif }; struct RestoreRegularFile : CreateRegularFileSink @@ -114,7 +140,14 @@ void RestoreSink::createRegularFile(const CanonPath & path, std::function '%2%'", p.string(), target); + return; + } +#endif nix::createSymlink(target, p.string()); } diff --git a/src/libutil/include/nix/util/fs-sink.hh b/src/libutil/include/nix/util/fs-sink.hh index bd2db7f53..132658b5d 100644 --- a/src/libutil/include/nix/util/fs-sink.hh +++ b/src/libutil/include/nix/util/fs-sink.hh @@ -82,6 +82,18 @@ struct NullFileSystemObjectSink : FileSystemObjectSink struct RestoreSink : FileSystemObjectSink { std::filesystem::path dstPath; +#ifndef _WIN32 + /** + * File descriptor for the directory located at dstPath. Used for *at + * operations relative to this file descriptor. This sink must *never* + * follow intermediate symlinks (starting from dstPath) in case a file + * collision is encountered for various reasons like case-insensitivity or + * other types on normalization. using appropriate *at system calls and traversing + * only one path component at a time ensures that writing is race-free and is + * is not susceptible to symlink replacement. + */ + AutoCloseFD dirFd; +#endif bool startFsync = false; explicit RestoreSink(bool startFsync) From 09755e696a04e16d9f51f48a529e1b4881af368c Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 20 Nov 2025 01:27:18 +0300 Subject: [PATCH 3/4] libutil: Add callback-based FileSystemObjectSink::createDirectory --- src/libutil/archive.cc | 78 ++++++++++++------------- src/libutil/fs-sink.cc | 11 ++-- src/libutil/include/nix/util/fs-sink.hh | 17 ++++++ 3 files changed, 62 insertions(+), 44 deletions(-) diff --git a/src/libutil/archive.cc b/src/libutil/archive.cc index 3b5b610db..0291d6827 100644 --- a/src/libutil/archive.cc +++ b/src/libutil/archive.cc @@ -200,54 +200,54 @@ static void parse(FileSystemObjectSink & sink, Source & source, const CanonPath } else if (type == "directory") { - sink.createDirectory(path); + sink.createDirectory(path, [&](FileSystemObjectSink & dirSink, const CanonPath & relDirPath) { + std::map names; - std::map names; + std::string prevName; - std::string prevName; + while (1) { + auto tag = getString(); - while (1) { - auto tag = getString(); + if (tag == ")") + break; - if (tag == ")") - break; + if (tag != "entry") + throw badArchive("expected tag 'entry' or ')', got '%s'", tag); - if (tag != "entry") - throw badArchive("expected tag 'entry' or ')', got '%s'", tag); + expectTag("("); - expectTag("("); + expectTag("name"); - expectTag("name"); + auto name = getString(); + if (name.empty() || name == "." || name == ".." || name.find('/') != std::string::npos + || name.find((char) 0) != std::string::npos) + throw badArchive("NAR contains invalid file name '%1%'", name); + if (name <= prevName) + throw badArchive("NAR directory is not sorted"); + prevName = name; + if (archiveSettings.useCaseHack) { + auto i = names.find(name); + if (i != names.end()) { + debug("case collision between '%1%' and '%2%'", i->first, name); + name += caseHackSuffix; + name += std::to_string(++i->second); + auto j = names.find(name); + if (j != names.end()) + throw badArchive( + "NAR contains file name '%s' that collides with case-hacked file name '%s'", + prevName, + j->first); + } else + names[name] = 0; + } - auto name = getString(); - if (name.empty() || name == "." || name == ".." || name.find('/') != std::string::npos - || name.find((char) 0) != std::string::npos) - throw badArchive("NAR contains invalid file name '%1%'", name); - if (name <= prevName) - throw badArchive("NAR directory is not sorted"); - prevName = name; - if (archiveSettings.useCaseHack) { - auto i = names.find(name); - if (i != names.end()) { - debug("case collision between '%1%' and '%2%'", i->first, name); - name += caseHackSuffix; - name += std::to_string(++i->second); - auto j = names.find(name); - if (j != names.end()) - throw badArchive( - "NAR contains file name '%s' that collides with case-hacked file name '%s'", - prevName, - j->first); - } else - names[name] = 0; + expectTag("node"); + + parse(dirSink, source, relDirPath / name); + + expectTag(")"); } - - expectTag("node"); - - parse(sink, source, path / name); - - expectTag(")"); - } + }); } else if (type == "symlink") { diff --git a/src/libutil/fs-sink.cc b/src/libutil/fs-sink.cc index 5b5f73067..9058d6e00 100644 --- a/src/libutil/fs-sink.cc +++ b/src/libutil/fs-sink.cc @@ -34,11 +34,12 @@ void copyRecursive(SourceAccessor & accessor, const CanonPath & from, FileSystem } case SourceAccessor::tDirectory: { - sink.createDirectory(to); - for (auto & [name, _] : accessor.readDirectory(from)) { - copyRecursive(accessor, from / name, sink, to / name); - break; - } + sink.createDirectory(to, [&](FileSystemObjectSink & dirSink, const CanonPath & relDirPath) { + for (auto & [name, _] : accessor.readDirectory(from)) { + copyRecursive(accessor, from / name, dirSink, relDirPath / name); + break; + } + }); break; } diff --git a/src/libutil/include/nix/util/fs-sink.hh b/src/libutil/include/nix/util/fs-sink.hh index 132658b5d..b2b009b83 100644 --- a/src/libutil/include/nix/util/fs-sink.hh +++ b/src/libutil/include/nix/util/fs-sink.hh @@ -36,6 +36,23 @@ struct FileSystemObjectSink virtual void createDirectory(const CanonPath & path) = 0; + using DirectoryCreatedCallback = std::function; + + /** + * Create a directory and invoke a callback with a pair of sink + CanonPath + * of the created subdirectory relative to dirSink. + * + * @note This allows for UNIX RestoreSink implementations to implement + * *at-style accessors that always keep an open file descriptor for the + * freshly created directory. Use this when it's important to disallow any + * intermediate path components from being symlinks. + */ + virtual void createDirectory(const CanonPath & path, DirectoryCreatedCallback callback) + { + createDirectory(path); + callback(*this, path); + } + /** * This function in general is no re-entrant. Only one file can be * written at a time. From 40b25153b8a33b44e7b0d6f92b3db6c5a29f9594 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 20 Nov 2025 02:03:19 +0300 Subject: [PATCH 4/4] libutil: Implement second overload of createDirectory for RestoreSink Now the intermediate symlink following issue should be completely plugged. --- src/libutil/fs-sink.cc | 23 +++++++++++++++++++++++ src/libutil/include/nix/util/fs-sink.hh | 4 ++++ tests/functional/nars.sh | 2 +- 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/libutil/fs-sink.cc b/src/libutil/fs-sink.cc index 9058d6e00..45c0262e6 100644 --- a/src/libutil/fs-sink.cc +++ b/src/libutil/fs-sink.cc @@ -71,6 +71,29 @@ static std::filesystem::path append(const std::filesystem::path & src, const Can return dst; } +#ifndef _WIN32 +void RestoreSink::createDirectory(const CanonPath & path, DirectoryCreatedCallback callback) +{ + if (path.isRoot()) { + createDirectory(path); + callback(*this, path); + return; + } + + createDirectory(path); + assert(dirFd); // If that's not true the above call must have thrown an exception. + + RestoreSink dirSink{startFsync}; + dirSink.dstPath = append(dstPath, path); + dirSink.dirFd = ::openat(dirFd.get(), path.rel_c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + + if (!dirSink.dirFd) + throw SysError("opening directory '%s'", dirSink.dstPath.string()); + + callback(dirSink, CanonPath::root); +} +#endif + void RestoreSink::createDirectory(const CanonPath & path) { auto p = append(dstPath, path); diff --git a/src/libutil/include/nix/util/fs-sink.hh b/src/libutil/include/nix/util/fs-sink.hh index b2b009b83..60e8441dd 100644 --- a/src/libutil/include/nix/util/fs-sink.hh +++ b/src/libutil/include/nix/util/fs-sink.hh @@ -120,6 +120,10 @@ struct RestoreSink : FileSystemObjectSink void createDirectory(const CanonPath & path) override; +#ifndef _WIN32 + void createDirectory(const CanonPath & path, DirectoryCreatedCallback callback) override; +#endif + void createRegularFile(const CanonPath & path, std::function) override; void createSymlink(const CanonPath & path, const std::string & target) override; diff --git a/tests/functional/nars.sh b/tests/functional/nars.sh index a52c257bc..2925177c5 100755 --- a/tests/functional/nars.sh +++ b/tests/functional/nars.sh @@ -114,7 +114,7 @@ if (( unicodeTestCode == 1 )); then # If the command failed (MacOS or ZFS + normalization), checks that it failed # with the expected "already exists" error, and that this is the same # behavior as `touch` - echo "$unicodeTestOut" | grepQuiet "path '.*/out/â' already exists" + echo "$unicodeTestOut" | grepQuiet "creating directory '.*/out/â': File exists" (( touchFilesCount == 1 )) elif (( unicodeTestCode == 0 )); then