libutil-tests: Add tests for makeFSSourceAccessor

Should be pretty self-explanatory. We didn't really have unit tests
for the filesystem source accessor. Now we do and this will be immensely
useful for implementing a unix-only smarter accessor that doesn't suffer
from TOCTOU on symlinks.

src/libutil-tests/meson.build

@@ -33,6 +33,9 @@ deps_private += rapidcheck
 gtest = dependency('gtest', main : true)
 deps_private += gtest
 
+gmock = dependency('gmock')
+deps_private += gmock
+
 configdata = configuration_data()
 configdata.set_quoted('PACKAGE_VERSION', meson.project_version())
 
@@ -72,6 +75,7 @@ sources = files(
   'position.cc',
   'processes.cc',
   'sort.cc',
+  'source-accessor.cc',
   'spawn.cc',
   'strings.cc',
   'suggestions.cc',

src/libutil-tests/source-accessor.cc

@@ -0,0 +1,138 @@
+#include "nix/util/fs-sink.hh"
+#include "nix/util/file-system.hh"
+#include "nix/util/processes.hh"
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+#include <rapidcheck/gtest.h>
+
+namespace nix {
+
+MATCHER_P2(HasContents, path, expected, "")
+{
+    auto stat = arg->maybeLstat(path);
+    if (!stat) {
+        *result_listener << arg->showPath(path) << " does not exist";
+        return false;
+    }
+    if (stat->type != SourceAccessor::tRegular) {
+        *result_listener << arg->showPath(path) << " is not a regular file";
+        return false;
+    }
+    auto actual = arg->readFile(path);
+    if (actual != expected) {
+        *result_listener << arg->showPath(path) << " has contents " << ::testing::PrintToString(actual);
+        return false;
+    }
+    return true;
+}
+
+MATCHER_P2(HasSymlink, path, target, "")
+{
+    auto stat = arg->maybeLstat(path);
+    if (!stat) {
+        *result_listener << arg->showPath(path) << " does not exist";
+        return false;
+    }
+    if (stat->type != SourceAccessor::tSymlink) {
+        *result_listener << arg->showPath(path) << " is not a symlink";
+        return false;
+    }
+    auto actual = arg->readLink(path);
+    if (actual != target) {
+        *result_listener << arg->showPath(path) << " points to " << ::testing::PrintToString(actual);
+        return false;
+    }
+    return true;
+}
+
+MATCHER_P2(HasDirectory, path, dirents, "")
+{
+    auto stat = arg->maybeLstat(path);
+    if (!stat) {
+        *result_listener << arg->showPath(path) << " does not exist";
+        return false;
+    }
+    if (stat->type != SourceAccessor::tDirectory) {
+        *result_listener << arg->showPath(path) << " is not a directory";
+        return false;
+    }
+    auto actual = arg->readDirectory(path);
+    std::set<std::string> actualKeys, expectedKeys(dirents.begin(), dirents.end());
+    for (auto & [k, _] : actual)
+        actualKeys.insert(k);
+    if (actualKeys != expectedKeys) {
+        *result_listener << arg->showPath(path) << " has entries " << ::testing::PrintToString(actualKeys);
+        return false;
+    }
+    return true;
+}
+
+class FSSourceAccessorTest : public ::testing::Test
+{
+protected:
+    std::filesystem::path tmpDir;
+    std::unique_ptr<nix::AutoDelete> delTmpDir;
+
+    void SetUp() override
+    {
+        tmpDir = nix::createTempDir();
+        delTmpDir = std::make_unique<nix::AutoDelete>(tmpDir, true);
+    }
+
+    void TearDown() override
+    {
+        delTmpDir.release();
+    }
+};
+
+TEST_F(FSSourceAccessorTest, works)
+{
+    {
+        RestoreSink sink(false);
+        sink.dstPath = tmpDir;
+        sink.dirFd = openDirectory(tmpDir);
+        sink.createDirectory(CanonPath("subdir"));
+        sink.createRegularFile(CanonPath("file1"), [](CreateRegularFileSink & crf) { crf("content1"); });
+        sink.createRegularFile(CanonPath("subdir/file2"), [](CreateRegularFileSink & crf) { crf("content2"); });
+        sink.createSymlink(CanonPath("rootlink"), "target");
+        sink.createDirectory(CanonPath("a"));
+        sink.createSymlink(CanonPath("a/dirlink"), "../subdir");
+    }
+
+    EXPECT_THAT(makeFSSourceAccessor(tmpDir / "file1"), HasContents(CanonPath::root, "content1"));
+    EXPECT_THAT(makeFSSourceAccessor(tmpDir / "rootlink"), HasSymlink(CanonPath::root, "target"));
+    EXPECT_THAT(
+        makeFSSourceAccessor(tmpDir),
+        HasDirectory(CanonPath::root, std::set<std::string>{"file1", "subdir", "rootlink", "a"}));
+    EXPECT_THAT(makeFSSourceAccessor(tmpDir / "subdir"), HasDirectory(CanonPath::root, std::set<std::string>{"file2"}));
+
+    {
+        auto accessor = makeFSSourceAccessor(tmpDir);
+        EXPECT_THAT(accessor, HasContents(CanonPath("file1"), "content1"));
+        EXPECT_THAT(accessor, HasContents(CanonPath("subdir/file2"), "content2"));
+
+        EXPECT_TRUE(accessor->pathExists(CanonPath("file1")));
+        EXPECT_FALSE(accessor->pathExists(CanonPath("nonexistent")));
+
+        EXPECT_THROW(accessor->readFile(CanonPath("a/dirlink/file2")), SymlinkNotAllowed);
+        EXPECT_THROW(accessor->maybeLstat(CanonPath("a/dirlink/file2")), SymlinkNotAllowed);
+        EXPECT_THROW(accessor->readDirectory(CanonPath("a/dirlink")), SymlinkNotAllowed);
+        EXPECT_THROW(accessor->pathExists(CanonPath("a/dirlink/file2")), SymlinkNotAllowed);
+    }
+
+    {
+        auto accessor = makeFSSourceAccessor(tmpDir / "nonexistent");
+        EXPECT_FALSE(accessor->maybeLstat(CanonPath::root));
+        EXPECT_THROW(accessor->readFile(CanonPath::root), SystemError);
+    }
+
+    {
+        auto accessor = makeFSSourceAccessor(tmpDir, true);
+        EXPECT_EQ(accessor->getLastModified(), 0);
+        accessor->maybeLstat(CanonPath("file1"));
+        EXPECT_GT(accessor->getLastModified(), 0);
+    }
+}
+
+} // namespace nix

src/libutil/archive.cc

@@ -103,7 +103,8 @@ void SourceAccessor::dumpPath(const CanonPath & path, Sink & sink, PathFilter &
 
 time_t dumpPathAndGetMtime(const Path & path, Sink & sink, PathFilter & filter)
 {
-    auto path2 = PosixSourceAccessor::createAtRoot(path, /*trackLastModified=*/true);
+    SourcePath path2 = {
+        makeFSSourceAccessor(std::filesystem::path{}, /*trackLastModified=*/true), CanonPath(absPath(path))};
     path2.dumpPath(sink, filter);
     return path2.accessor->getLastModified().value();
 }

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

@@ -2,7 +2,6 @@
 ///@file
 
 #include "nix/util/canon-path.hh"
-#include "nix/util/types.hh"
 #include "nix/util/error.hh"
 
 #ifdef _WIN32
@@ -236,18 +235,6 @@ std::wstring handleToFileName(Descriptor handle);
 #ifndef _WIN32
 namespace unix {
 
-struct SymlinkNotAllowed : public Error
-{
-    CanonPath path;
-
-    SymlinkNotAllowed(CanonPath path)
-        /* Can't provide better error message, since the parent directory is only known to the caller. */
-        : Error("relative path '%s' points to a symlink, which is not allowed", path.rel())
-        , path(std::move(path))
-    {
-    }
-};
-
 /**
  * Safe(r) function to open \param path file relative to \param dirFd, while
  * disallowing escaping from a directory and resolving any symlinks in the

src/libutil/include/nix/util/posix-source-accessor.hh

@@ -43,36 +43,6 @@ public:
 
     std::optional<std::filesystem::path> getPhysicalPath(const CanonPath & path) override;
 
-    /**
-     * Create a `PosixSourceAccessor` and `SourcePath` corresponding to
-     * some native path.
-     *
-     * @param Whether the accessor should return a non-null getLastModified.
-     * When true the accessor must be used only by a single thread.
-     *
-     * The `PosixSourceAccessor` is rooted as far up the tree as
-     * possible, (e.g. on Windows it could scoped to a drive like
-     * `C:\`). This allows more `..` parent accessing to work.
-     *
-     * @note When `path` is trusted user input, canonicalize it using
-     * `std::filesystem::canonical`, `makeParentCanonical`, `std::filesystem::weakly_canonical`, etc,
-     * as appropriate for the use case. At least weak canonicalization is
-     * required for the `SourcePath` to do anything useful at the location it
-     * points to.
-     *
-     * @note A canonicalizing behavior is not built in `createAtRoot` so that
-     * callers do not accidentally introduce symlink-related security vulnerabilities.
-     * Furthermore, `createAtRoot` does not know whether the file pointed to by
-     * `path` should be resolved if it is itself a symlink. In other words,
-     * `createAtRoot` can not decide between aforementioned `canonical`, `makeParentCanonical`, etc. for its callers.
-     *
-     * See
-     * [`std::filesystem::path::root_path`](https://en.cppreference.com/w/cpp/filesystem/path/root_path)
-     * and
-     * [`std::filesystem::path::relative_path`](https://en.cppreference.com/w/cpp/filesystem/path/relative_path).
-     */
-    static SourcePath createAtRoot(const std::filesystem::path & path, bool trackLastModified = false);
-
     std::optional<std::time_t> getLastModified() override
     {
         return trackLastModified ? std::optional{mtime} : std::nullopt;

src/libutil/include/nix/util/source-accessor.hh

@@ -222,6 +222,24 @@ ref<SourceAccessor> makeEmptySourceAccessor();
  */
 MakeError(RestrictedPathError, Error);
 
+struct SymlinkNotAllowed : public Error
+{
+    CanonPath path;
+
+    SymlinkNotAllowed(CanonPath path)
+        : Error("relative path '%s' points to a symlink, which is not allowed", path.rel())
+        , path(std::move(path))
+    {
+    }
+
+    template<typename... Args>
+    SymlinkNotAllowed(CanonPath path, const std::string & fs, Args &&... args)
+        : Error(fs, std::forward<Args>(args)...)
+        , path(std::move(path))
+    {
+    }
+};
+
 /**
  * Return an accessor for the root filesystem.
  */
@@ -233,7 +251,7 @@ ref<SourceAccessor> getFSSourceAccessor();
  * elements, and that absolute symlinks are resolved relative to
  * `root`.
  */
-ref<SourceAccessor> makeFSSourceAccessor(std::filesystem::path root);
+ref<SourceAccessor> makeFSSourceAccessor(std::filesystem::path root, bool trackLastModified = false);
 
 /**
  * Construct an accessor that presents a "union" view of a vector of

src/libutil/posix-source-accessor.cc

@@ -1,7 +1,5 @@
 #include "nix/util/posix-source-accessor.hh"
-#include "nix/util/source-path.hh"
 #include "nix/util/signals.hh"
-#include "nix/util/sync.hh"
 
 #include <boost/unordered/concurrent_flat_map.hpp>
 
@@ -20,15 +18,6 @@ PosixSourceAccessor::PosixSourceAccessor()
 {
 }
 
-SourcePath PosixSourceAccessor::createAtRoot(const std::filesystem::path & path, bool trackLastModified)
-{
-    std::filesystem::path path2 = absPath(path);
-    return {
-        make_ref<PosixSourceAccessor>(path2.root_path(), trackLastModified),
-        CanonPath{path2.relative_path().string()},
-    };
-}
-
 std::filesystem::path PosixSourceAccessor::makeAbsPath(const CanonPath & path)
 {
     return root.empty()    ? (std::filesystem::path{path.abs()})
@@ -208,7 +197,7 @@ void PosixSourceAccessor::assertNoSymlinks(CanonPath path)
     while (!path.isRoot()) {
         auto st = cachedLstat(path);
         if (st && S_ISLNK(st->st_mode))
-            throw Error("path '%s' is a symlink", showPath(path));
+            throw SymlinkNotAllowed(path, "path '%s' is a symlink", showPath(path));
         path.pop();
     }
 }
@@ -219,8 +208,8 @@ ref<SourceAccessor> getFSSourceAccessor()
     return rootFS;
 }
 
-ref<SourceAccessor> makeFSSourceAccessor(std::filesystem::path root)
+ref<SourceAccessor> makeFSSourceAccessor(std::filesystem::path root, bool trackLastModified)
 {
-    return make_ref<PosixSourceAccessor>(std::move(root));
+    return make_ref<PosixSourceAccessor>(std::move(root), trackLastModified);
 }
 } // namespace nix

src/libutil/unix/file-descriptor.cc

@@ -3,6 +3,7 @@
 #include "nix/util/signals.hh"
 #include "nix/util/finally.hh"
 #include "nix/util/serialise.hh"
+#include "nix/util/source-accessor.hh"
 
 #include <fcntl.h>
 #include <unistd.h>
@@ -301,10 +302,10 @@ openFileEnsureBeneathNoSymlinksIterative(Descriptor dirFd, const CanonPath & pat
             if (errno == ENOTDIR) /* Path component might be a symlink. */ {
                 struct ::stat st;
                 if (::fstatat(getParentFd(), component.c_str(), &st, AT_SYMLINK_NOFOLLOW) == 0 && S_ISLNK(st.st_mode))
-                    throw unix::SymlinkNotAllowed(path2);
+                    throw SymlinkNotAllowed(path2);
                 errno = ENOTDIR; /* Restore the errno. */
             } else if (errno == ELOOP) {
-                throw unix::SymlinkNotAllowed(path2);
+                throw SymlinkNotAllowed(path2);
             }
 
             return INVALID_DESCRIPTOR;
@@ -315,7 +316,7 @@ openFileEnsureBeneathNoSymlinksIterative(Descriptor dirFd, const CanonPath & pat
 
     auto res = ::openat(getParentFd(), std::string(path.baseName().value()).c_str(), flags | O_NOFOLLOW, mode);
     if (res < 0 && errno == ELOOP)
-        throw unix::SymlinkNotAllowed(path);
+        throw SymlinkNotAllowed(path);
     return res;
 }
 
@@ -328,7 +329,7 @@ Descriptor unix::openFileEnsureBeneathNoSymlinks(Descriptor dirFd, const CanonPa
         dirFd, path.rel_c_str(), flags, static_cast<uint64_t>(mode), RESOLVE_BENEATH | RESOLVE_NO_SYMLINKS);
     if (maybeFd) {
         if (*maybeFd < 0 && errno == ELOOP)
-            throw unix::SymlinkNotAllowed(path);
+            throw SymlinkNotAllowed(path);
         return *maybeFd;
     }
 #endif

src/nix/add-to-store.cc

@@ -38,7 +38,7 @@ struct CmdAddToStore : MixDryRun, StoreCommand
         if (!namePart)
             namePart = baseNameOf(path);
 
-        auto sourcePath = PosixSourceAccessor::createAtRoot(makeParentCanonical(path));
+        SourcePath sourcePath = makeFSSourceAccessor(makeParentCanonical(path));
 
         auto storePath = dryRun ? store->computeStorePath(*namePart, sourcePath, caMethod, hashAlgo, {}).first
                                 : store->addToStoreSlow(*namePart, sourcePath, caMethod, hashAlgo, {}).path;

src/nix/hash.cc

@@ -85,9 +85,7 @@ struct CmdHashBase : Command
                     return std::make_unique<HashSink>(hashAlgo);
             };
 
-            auto makeSourcePath = [&]() -> SourcePath {
-                return PosixSourceAccessor::createAtRoot(makeParentCanonical(path));
-            };
+            auto makeSourcePath = [&]() -> SourcePath { return makeFSSourceAccessor(makeParentCanonical(path)); };
 
             Hash h{HashAlgorithm::SHA256}; // throwaway def to appease C++
             switch (mode) {

src/nix/nix-store/nix-store.cc

@@ -191,7 +191,7 @@ static void opAdd(Strings opFlags, Strings opArgs)
         throw UsageError("unknown flag");
 
     for (auto & i : opArgs) {
-        auto sourcePath = PosixSourceAccessor::createAtRoot(makeParentCanonical(i));
+        SourcePath sourcePath = makeFSSourceAccessor(makeParentCanonical(i));
         cout << fmt("%s\n", store->printStorePath(store->addToStore(std::string(baseNameOf(i)), sourcePath)));
     }
 }
@@ -215,7 +215,7 @@ static void opAddFixed(Strings opFlags, Strings opArgs)
     opArgs.pop_front();
 
     for (auto & i : opArgs) {
-        auto sourcePath = PosixSourceAccessor::createAtRoot(makeParentCanonical(i));
+        SourcePath sourcePath = makeFSSourceAccessor(makeParentCanonical(i));
         std::cout << fmt(
             "%s\n", store->printStorePath(store->addToStoreSlow(baseNameOf(i), sourcePath, method, hashAlgo).path));
     }

src/perl/lib/Nix/Store.xs

@@ -256,7 +256,7 @@ hashPath(char * algo, int base32, char * path)
     PPCODE:
         try {
             Hash h = hashPath(
-                PosixSourceAccessor::createAtRoot(path),
+                SourcePath{getFSSourceAccessor(), CanonPath(absPath(Path(path)))},
                 FileIngestionMethod::NixArchive, parseHashAlgo(algo)).first;
             auto s = h.to_string(base32 ? HashFormat::Nix32 : HashFormat::Base16, false);
             XPUSHs(sv_2mortal(newSVpv(s.c_str(), 0)));
@@ -336,7 +336,7 @@ StoreWrapper::addToStore(char * srcPath, int recursive, char * algo)
             auto method = recursive ? ContentAddressMethod::Raw::NixArchive : ContentAddressMethod::Raw::Flat;
             auto path = THIS->store->addToStore(
                 std::string(baseNameOf(srcPath)),
-                PosixSourceAccessor::createAtRoot(srcPath),
+                SourcePath{getFSSourceAccessor(), CanonPath(absPath(Path(srcPath)))},
                 method, parseHashAlgo(algo));
             XPUSHs(sv_2mortal(newSVpv(THIS->store->printStorePath(path).c_str(), 0)));
         } catch (Error & e) {