Merge pull request #12046 from roberth/cli-symlink-fixes

CLI symlink fixes

Author: Mic92

doc/manual/source/command-ref/nix-store/add-fixed.md

@@ -21,6 +21,9 @@ This operation has the following options:
   Use recursive instead of flat hashing mode, used when adding
   directories to the store.
 
+  *paths* that refer to symlinks are not dereferenced, but added to the store
+  as symlinks with the same target.
+
 {{#include ./opt-common.md}}
 
 {{#include ../opt-common.md}}

doc/manual/source/command-ref/nix-store/add.md

@@ -11,6 +11,9 @@
 The operation `--add` adds the specified paths to the Nix store. It
 prints the resulting paths in the Nix store on standard output.
 
+*paths* that refer to symlinks are not dereferenced, but added to the store
+as symlinks with the same target.
+
 {{#include ./opt-common.md}}
 
 {{#include ../opt-common.md}}

src/libutil-tests/file-system.cc

@@ -261,4 +261,18 @@ 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

@@ -766,4 +766,19 @@ bool isExecutableFileAmbient(const fs::path & exe) {
         ) == 0;
 }
 
+std::filesystem::path makeParentCanonical(const std::filesystem::path & rawPath)
+{
+    std::filesystem::path path(absPath(rawPath));;
+    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

@@ -143,6 +143,23 @@ inline bool symlink_exists(const std::filesystem::path & path) {
 
 } // namespace fs
 
+/**
+ * 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;
             }
             }

tests/functional/add.sh

@@ -29,6 +29,47 @@ echo "$hash2"
 
 test "$hash1" = "sha256:$hash2"
 
+# The contents can be accessed through a symlink, and this symlink has no effect on the hash
+# https://github.com/NixOS/nix/issues/11941
+test_issue_11941() {
+    local expected actual
+    mkdir -p "$TEST_ROOT/foo/bar" && ln -s "$TEST_ROOT/foo" "$TEST_ROOT/foo-link"
+
+    # legacy
+    expected=$(nix-store --add-fixed --recursive sha256 "$TEST_ROOT/foo/bar")
+    actual=$(nix-store --add-fixed --recursive sha256 "$TEST_ROOT/foo-link/bar")
+    [[ "$expected" == "$actual" ]]
+    actual=$(nix-store --add "$TEST_ROOT/foo-link/bar")
+    [[ "$expected" == "$actual" ]]
+
+    # nix store add
+    actual=$(nix store add --hash-algo sha256 --mode nar "$TEST_ROOT/foo/bar")
+    [[ "$expected" == "$actual" ]]
+
+    # cleanup
+    rm -r "$TEST_ROOT/foo" "$TEST_ROOT/foo-link"
+}
+test_issue_11941
+
+# A symlink is added to the store as a symlink, not as a copy of the target
+test_add_symlink() {
+    ln -s /bin "$TEST_ROOT/my-bin"
+
+    # legacy
+    path=$(nix-store --add-fixed --recursive sha256 "$TEST_ROOT/my-bin")
+    [[ "$(readlink "$path")" == /bin ]]
+    path=$(nix-store --add "$TEST_ROOT/my-bin")
+    [[ "$(readlink "$path")" == /bin ]]
+
+    # nix store add
+    path=$(nix store add --hash-algo sha256 --mode nar "$TEST_ROOT/my-bin")
+    [[ "$(readlink "$path")" == /bin ]]
+
+    # cleanup
+    rm "$TEST_ROOT/my-bin"
+}
+test_add_symlink
+
 #### New style commands
 
 clearStoreIfPossible