Merge pull request #12144 from NixOS/mergify/bp/2.24-maintenance/pr-12046

CLI symlink fixes (backport #12046)

src/libutil-tests/file-system.cc

@@ -0,0 +1,278 @@
+#include "util.hh"
+#include "types.hh"
+#include "file-system.hh"
+#include "processes.hh"
+#include "terminal.hh"
+#include "strings.hh"
+
+#include <limits.h>
+#include <gtest/gtest.h>
+#include <rapidcheck/gtest.h>
+
+#include <numeric>
+
+#ifdef _WIN32
+#  define FS_SEP L"\\"
+#  define FS_ROOT L"C:" FS_SEP // Need a mounted one, C drive is likely
+#else
+#  define FS_SEP "/"
+#  define FS_ROOT FS_SEP
+#endif
+
+#ifndef PATH_MAX
+#  define PATH_MAX 4096
+#endif
+
+#ifdef _WIN32
+#  define GET_CWD _wgetcwd
+#else
+#  define GET_CWD getcwd
+#endif
+
+namespace nix {
+
+/* ----------- tests for file-system.hh -------------------------------------*/
+
+/* ----------------------------------------------------------------------------
+ * absPath
+ * --------------------------------------------------------------------------*/
+
+TEST(absPath, doesntChangeRoot)
+{
+    auto p = absPath(std::filesystem::path{FS_ROOT});
+
+    ASSERT_EQ(p, FS_ROOT);
+}
+
+TEST(absPath, turnsEmptyPathIntoCWD)
+{
+    OsChar cwd[PATH_MAX + 1];
+    auto p = absPath(std::filesystem::path{""});
+
+    ASSERT_EQ(p, GET_CWD((OsChar *) &cwd, PATH_MAX));
+}
+
+TEST(absPath, usesOptionalBasePathWhenGiven)
+{
+    OsChar _cwd[PATH_MAX + 1];
+    OsChar * cwd = GET_CWD((OsChar *) &_cwd, PATH_MAX);
+
+    auto p = absPath(std::filesystem::path{""}.string(), std::filesystem::path{cwd}.string());
+
+    ASSERT_EQ(p, std::filesystem::path{cwd}.string());
+}
+
+TEST(absPath, isIdempotent)
+{
+    OsChar _cwd[PATH_MAX + 1];
+    OsChar * cwd = GET_CWD((OsChar *) &_cwd, PATH_MAX);
+    auto p1 = absPath(std::filesystem::path{cwd});
+    auto p2 = absPath(p1);
+
+    ASSERT_EQ(p1, p2);
+}
+
+TEST(absPath, pathIsCanonicalised)
+{
+    auto path = FS_ROOT OS_STR("some/path/with/trailing/dot/.");
+    auto p1 = absPath(std::filesystem::path{path});
+    auto p2 = absPath(p1);
+
+    ASSERT_EQ(p1, FS_ROOT "some" FS_SEP "path" FS_SEP "with" FS_SEP "trailing" FS_SEP "dot");
+    ASSERT_EQ(p1, p2);
+}
+
+/* ----------------------------------------------------------------------------
+ * canonPath
+ * --------------------------------------------------------------------------*/
+
+TEST(canonPath, removesTrailingSlashes)
+{
+    std::filesystem::path path = FS_ROOT "this/is/a/path//";
+    auto p = canonPath(path.string());
+
+    ASSERT_EQ(p, std::filesystem::path{FS_ROOT "this" FS_SEP "is" FS_SEP "a" FS_SEP "path"}.string());
+}
+
+TEST(canonPath, removesDots)
+{
+    std::filesystem::path path = FS_ROOT "this/./is/a/path/./";
+    auto p = canonPath(path.string());
+
+    ASSERT_EQ(p, std::filesystem::path{FS_ROOT "this" FS_SEP "is" FS_SEP "a" FS_SEP "path"}.string());
+}
+
+TEST(canonPath, removesDots2)
+{
+    std::filesystem::path path = FS_ROOT "this/a/../is/a////path/foo/..";
+    auto p = canonPath(path.string());
+
+    ASSERT_EQ(p, std::filesystem::path{FS_ROOT "this" FS_SEP "is" FS_SEP "a" FS_SEP "path"}.string());
+}
+
+TEST(canonPath, requiresAbsolutePath)
+{
+    ASSERT_ANY_THROW(canonPath("."));
+    ASSERT_ANY_THROW(canonPath(".."));
+    ASSERT_ANY_THROW(canonPath("../"));
+    ASSERT_DEATH({ canonPath(""); }, "path != \"\"");
+}
+
+/* ----------------------------------------------------------------------------
+ * dirOf
+ * --------------------------------------------------------------------------*/
+
+TEST(dirOf, returnsEmptyStringForRoot)
+{
+    auto p = dirOf("/");
+
+    ASSERT_EQ(p, "/");
+}
+
+TEST(dirOf, returnsFirstPathComponent)
+{
+    auto p1 = dirOf("/dir/");
+    ASSERT_EQ(p1, "/dir");
+    auto p2 = dirOf("/dir");
+    ASSERT_EQ(p2, "/");
+    auto p3 = dirOf("/dir/..");
+    ASSERT_EQ(p3, "/dir");
+    auto p4 = dirOf("/dir/../");
+    ASSERT_EQ(p4, "/dir/..");
+}
+
+/* ----------------------------------------------------------------------------
+ * baseNameOf
+ * --------------------------------------------------------------------------*/
+
+TEST(baseNameOf, emptyPath)
+{
+    auto p1 = baseNameOf("");
+    ASSERT_EQ(p1, "");
+}
+
+TEST(baseNameOf, pathOnRoot)
+{
+    auto p1 = baseNameOf("/dir");
+    ASSERT_EQ(p1, "dir");
+}
+
+TEST(baseNameOf, relativePath)
+{
+    auto p1 = baseNameOf("dir/foo");
+    ASSERT_EQ(p1, "foo");
+}
+
+TEST(baseNameOf, pathWithTrailingSlashRoot)
+{
+    auto p1 = baseNameOf("/");
+    ASSERT_EQ(p1, "");
+}
+
+TEST(baseNameOf, trailingSlash)
+{
+    auto p1 = baseNameOf("/dir/");
+    ASSERT_EQ(p1, "dir");
+}
+
+TEST(baseNameOf, trailingSlashes)
+{
+    auto p1 = baseNameOf("/dir//");
+    ASSERT_EQ(p1, "dir");
+}
+
+TEST(baseNameOf, absoluteNothingSlashNothing)
+{
+    auto p1 = baseNameOf("//");
+    ASSERT_EQ(p1, "");
+}
+
+/* ----------------------------------------------------------------------------
+ * isInDir
+ * --------------------------------------------------------------------------*/
+
+TEST(isInDir, trivialCase)
+{
+    auto p1 = isInDir("/foo/bar", "/foo");
+    ASSERT_EQ(p1, true);
+}
+
+TEST(isInDir, notInDir)
+{
+    auto p1 = isInDir("/zes/foo/bar", "/foo");
+    ASSERT_EQ(p1, false);
+}
+
+// XXX: hm, bug or feature? :) Looking at the implementation
+// this might be problematic.
+TEST(isInDir, emptyDir)
+{
+    auto p1 = isInDir("/zes/foo/bar", "");
+    ASSERT_EQ(p1, true);
+}
+
+/* ----------------------------------------------------------------------------
+ * isDirOrInDir
+ * --------------------------------------------------------------------------*/
+
+TEST(isDirOrInDir, trueForSameDirectory)
+{
+    ASSERT_EQ(isDirOrInDir("/nix", "/nix"), true);
+    ASSERT_EQ(isDirOrInDir("/", "/"), true);
+}
+
+TEST(isDirOrInDir, trueForEmptyPaths)
+{
+    ASSERT_EQ(isDirOrInDir("", ""), true);
+}
+
+TEST(isDirOrInDir, falseForDisjunctPaths)
+{
+    ASSERT_EQ(isDirOrInDir("/foo", "/bar"), false);
+}
+
+TEST(isDirOrInDir, relativePaths)
+{
+    ASSERT_EQ(isDirOrInDir("/foo/..", "/foo"), true);
+}
+
+// XXX: while it is possible to use "." or ".." in the
+// first argument this doesn't seem to work in the second.
+TEST(isDirOrInDir, DISABLED_shouldWork)
+{
+    ASSERT_EQ(isDirOrInDir("/foo/..", "/foo/."), true);
+}
+
+/* ----------------------------------------------------------------------------
+ * pathExists
+ * --------------------------------------------------------------------------*/
+
+TEST(pathExists, rootExists)
+{
+    ASSERT_TRUE(pathExists(std::filesystem::path{FS_ROOT}.string()));
+}
+
+TEST(pathExists, cwdExists)
+{
+    ASSERT_TRUE(pathExists("."));
+}
+
+TEST(pathExists, bogusPathDoesNotExist)
+{
+    ASSERT_FALSE(pathExists("/schnitzel/darmstadt/pommes"));
+}
+
+/* ----------------------------------------------------------------------------
+ * makeParentCanonical
+ * --------------------------------------------------------------------------*/
+
+TEST(makeParentCanonical, noParent)
+{
+    ASSERT_EQ(makeParentCanonical("file"), absPath(std::filesystem::path("file")));
+}
+
+TEST(makeParentCanonical, root)
+{
+    ASSERT_EQ(makeParentCanonical("/"), "/");
+}
+}

src/libutil/file-system.cc

@@ -685,4 +685,19 @@ void moveFile(const Path & oldName, const Path & newName)
 
 //////////////////////////////////////////////////////////////////////
 
+std::filesystem::path makeParentCanonical(const std::filesystem::path & rawPath)
+{
+    std::filesystem::path path(absPath(rawPath.string()));;
+    try {
+        auto parent = path.parent_path();
+        if (parent == path) {
+            // `path` is a root directory => trivially canonical
+            return parent;
+        }
+        return std::filesystem::canonical(parent) / path.filename();
+    } catch (fs::filesystem_error & e) {
+        throw SysError("canonicalising parent path of '%1%'", path);
+    }
 }
+
+} // namespace nix

src/libutil/file-system.hh

@@ -101,6 +101,23 @@ std::optional<struct stat> maybeLstat(const Path & path);
  */
 bool pathExists(const Path & path);
 
+/**
+ * Canonicalize a path except for the last component.
+ *
+ * This is useful for getting the canonical location of a symlink.
+ *
+ * Consider the case where `foo/l` is a symlink. `canonical("foo/l")` will
+ * resolve the symlink `l` to its target.
+ * `makeParentCanonical("foo/l")` will not resolve the symlink `l` to its target,
+ * but does ensure that the returned parent part of the path, `foo` is resolved
+ * to `canonical("foo")`, and can therefore be retrieved without traversing any
+ * symlinks.
+ *
+ * If a relative path is passed, it will be made absolute, so that the parent
+ * can always be canonicalized.
+ */
+std::filesystem::path makeParentCanonical(const std::filesystem::path & path);
+
 /**
  * A version of pathExists that returns false on a permission error.
  * Useful for inferring default paths across directories that might not

src/libutil/posix-source-accessor.hh

@@ -43,13 +43,25 @@ struct PosixSourceAccessor : virtual SourceAccessor
     std::optional<std::filesystem::path> getPhysicalPath(const CanonPath & path) override;
 
     /**
-     * Create a `PosixSourceAccessor` and `CanonPath` corresponding to
+     * Create a `PosixSourceAccessor` and `SourcePath` corresponding to
      * some native path.
      *
      * 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

src/nix-store/nix-store.cc

@@ -183,9 +183,9 @@ static void opAdd(Strings opFlags, Strings opArgs)
     if (!opFlags.empty()) throw UsageError("unknown flag");
 
     for (auto & i : opArgs) {
-        auto [accessor, canonPath] = PosixSourceAccessor::createAtRoot(i);
+        auto sourcePath = PosixSourceAccessor::createAtRoot(makeParentCanonical(i));
         cout << fmt("%s\n", store->printStorePath(store->addToStore(
-            std::string(baseNameOf(i)), {accessor, canonPath})));
+            std::string(baseNameOf(i)), sourcePath)));
     }
 }
 
@@ -207,10 +207,10 @@ static void opAddFixed(Strings opFlags, Strings opArgs)
     opArgs.pop_front();
 
     for (auto & i : opArgs) {
-        auto [accessor, canonPath] = PosixSourceAccessor::createAtRoot(i);
+        auto sourcePath = PosixSourceAccessor::createAtRoot(makeParentCanonical(i));
         std::cout << fmt("%s\n", store->printStorePath(store->addToStoreSlow(
             baseNameOf(i),
-            {accessor, canonPath},
+            sourcePath,
             method,
             hashAlgo).path));
     }

src/nix/add-to-store.cc

@@ -37,13 +37,13 @@ struct CmdAddToStore : MixDryRun, StoreCommand
     {
         if (!namePart) namePart = baseNameOf(path);
 
-        auto [accessor, path2] = PosixSourceAccessor::createAtRoot(path);
+        auto sourcePath = PosixSourceAccessor::createAtRoot(makeParentCanonical(path));
 
         auto storePath = dryRun
             ? store->computeStorePath(
-                *namePart, {accessor, path2}, caMethod, hashAlgo, {}).first
+                *namePart, sourcePath, caMethod, hashAlgo, {}).first
             : store->addToStoreSlow(
-                *namePart, {accessor, path2}, caMethod, hashAlgo, {}).path;
+                *namePart, sourcePath, caMethod, hashAlgo, {}).path;
 
         logger->cout("%s", store->printStorePath(storePath));
     }

src/nix/hash.cc

@@ -87,18 +87,35 @@ struct CmdHashBase : Command
                     return std::make_unique<HashSink>(hashAlgo);
             };
 
-            auto path2 = PosixSourceAccessor::createAtRoot(path);
+            auto makeSourcePath = [&]() -> SourcePath {
+                return PosixSourceAccessor::createAtRoot(makeParentCanonical(path));
+            };
+
             Hash h { HashAlgorithm::SHA256 }; // throwaway def to appease C++
             switch (mode) {
             case FileIngestionMethod::Flat:
+            {
+                // While usually we could use the some code as for NixArchive,
+                // the Flat method needs to support FIFOs, such as those
+                // produced by bash process substitution, e.g.:
+                //     nix hash --mode flat <(echo hi)
+                // Also symlinks semantics are unambiguous in the flat case,
+                // so we don't need to go low-level, or reject symlink `path`s.
+                auto hashSink = makeSink();
+                readFile(path, *hashSink);
+                h = hashSink->finish().first;
+                break;
+            }
             case FileIngestionMethod::NixArchive:
             {
+                auto sourcePath = makeSourcePath();
                 auto hashSink = makeSink();
-                dumpPath(path2, *hashSink, (FileSerialisationMethod) mode);
+                dumpPath(sourcePath, *hashSink, (FileSerialisationMethod) mode);
                 h = hashSink->finish().first;
                 break;
             }
             case FileIngestionMethod::Git: {
+                auto sourcePath = makeSourcePath();
                 std::function<git::DumpHook> hook;
                 hook = [&](const SourcePath & path) -> git::TreeEntry {
                     auto hashSink = makeSink();
@@ -109,7 +126,7 @@ struct CmdHashBase : Command
                         .hash = hash,
                     };
                 };
-                h = hook(path2).hash;
+                h = hook(sourcePath).hash;
                 break;
             }
             }