From af6eeec293f786c75cd8870991f0331cab5374f2 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 13 Dec 2024 18:37:25 +0100 Subject: [PATCH 1/7] Add makeParentCanonical() --- src/libutil/file-system.cc | 15 +++++++++++++++ src/libutil/file-system.hh | 16 ++++++++++++++++ src/libutil/posix-source-accessor.hh | 12 ++++++++++++ 3 files changed, 43 insertions(+) diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 829700336c1..1d2ff170183 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -765,4 +765,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 diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index 3c49181a0ad..d680439cc3e 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -143,6 +143,22 @@ 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 path's parent equals its `canonical`, + * 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 diff --git a/src/libutil/posix-source-accessor.hh b/src/libutil/posix-source-accessor.hh index 40f60bb54b8..ac5da850d25 100644 --- a/src/libutil/posix-source-accessor.hh +++ b/src/libutil/posix-source-accessor.hh @@ -50,6 +50,18 @@ struct PosixSourceAccessor : virtual SourceAccessor * 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 aforemention `canonical`, `makeParentCanonical`, etc. for its callers. + * * See * [`std::filesystem::path::root_path`](https://en.cppreference.com/w/cpp/filesystem/path/root_path) * and From 350b19c94f9ef067934327688a2c3bf56d3e46fb Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 13 Dec 2024 17:21:56 +0100 Subject: [PATCH 2/7] fix: Handle symlinks and FIFOs in `nix hash` where possible Fixes https://github.com/NixOS/nix/issues/11756 Fixes https://github.com/NixOS/nix/issues/11681 --- src/libutil/posix-source-accessor.hh | 2 +- src/nix/hash.cc | 23 ++++++++++++++++++++--- tests/functional/hash-path.sh | 24 ++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/libutil/posix-source-accessor.hh b/src/libutil/posix-source-accessor.hh index ac5da850d25..c351e4ca87c 100644 --- a/src/libutil/posix-source-accessor.hh +++ b/src/libutil/posix-source-accessor.hh @@ -43,7 +43,7 @@ struct PosixSourceAccessor : virtual SourceAccessor std::optional 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 diff --git a/src/nix/hash.cc b/src/nix/hash.cc index 416cd19b317..33060a4be9b 100644 --- a/src/nix/hash.cc +++ b/src/nix/hash.cc @@ -87,18 +87,35 @@ struct CmdHashBase : Command return std::make_unique(hashAlgo); }; - auto path2 = PosixSourceAccessor::createAtRoot(path); + auto makeSourcePath = [&]() -> SourcePath { + return PosixSourceAccessor::createAtRoot(std::filesystem::weakly_canonical(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 = PosixSourceAccessor::createAtRoot(path); std::function 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; } } diff --git a/tests/functional/hash-path.sh b/tests/functional/hash-path.sh index 86d782a9589..74e3195045a 100755 --- a/tests/functional/hash-path.sh +++ b/tests/functional/hash-path.sh @@ -92,3 +92,27 @@ try2 md5 "20f3ffe011d4cfa7d72bfabef7882836" rm "$TEST_ROOT/hash-path/hello" ln -s x "$TEST_ROOT/hash-path/hello" try2 md5 "f78b733a68f5edbdf9413899339eaa4a" + +# Flat mode supports process substitution +h=$(nix hash path --mode flat --type sha256 --base32 <(printf "SMASH THE STATE")) +[[ 0d9n3r2i4m1zgy0wpqbsyabsfzgs952066bfp8gwvcg4mkr4r5g8 == "$h" ]] + +# Flat mode supports process substitution (hash file) +h=$(nix hash file --type sha256 --base32 <(printf "SMASH THE STATE")) +[[ 0d9n3r2i4m1zgy0wpqbsyabsfzgs952066bfp8gwvcg4mkr4r5g8 == "$h" ]] + +# Symlinks in the ancestry are ok and don't affect the result +mkdir -p "$TEST_ROOT/simple" "$TEST_ROOT/try/to/mess/with/it" +echo hi > "$TEST_ROOT/simple/hi" +ln -s "$TEST_ROOT/simple" "$TEST_ROOT/try/to/mess/with/it/simple-link" +h=$(nix hash path --type sha256 --base32 "$TEST_ROOT/simple/hi") +[[ 1xmr8jicvzszfzpz46g37mlpvbzjl2wpwvl2b05psipssyp1sm8h == "$h" ]] +h=$(nix hash path --type sha256 --base32 "$TEST_ROOT/try/to/mess/with/it/simple-link/hi") +[[ 1xmr8jicvzszfzpz46g37mlpvbzjl2wpwvl2b05psipssyp1sm8h == "$h" ]] + +# nix hash --mode nar does not canonicalize a symlink argument. +# Otherwise it can't generate a NAR whose root is a symlink. +# If you want to follow the symlink, pass $(realpath -s ...) instead. +ln -s /non-existent-48cujwe8ndf4as0bne "$TEST_ROOT/symlink-to-nowhere" +h=$(nix hash path --mode nar --type sha256 --base32 "$TEST_ROOT/symlink-to-nowhere") +[[ 1bl5ry3x1fcbwgr5c2x50bn572iixh4j1p6ax5isxly2ddgn8pbp == "$h" ]] # manually verified hash From cd0215eb83f145eaa38e25c931353a983ef57431 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 13 Dec 2024 18:38:47 +0100 Subject: [PATCH 3/7] doc: Document `nix-store --add-fixed` symlink behavior Tested with nix run nix/2.3-maintenance#nix-store -- --add some_symlink nix run nix/2.3-maintenance#nix-store -- --add-fixed sha256 --recursive some_symlink --- doc/manual/source/command-ref/nix-store/add-fixed.md | 3 +++ doc/manual/source/command-ref/nix-store/add.md | 3 +++ 2 files changed, 6 insertions(+) diff --git a/doc/manual/source/command-ref/nix-store/add-fixed.md b/doc/manual/source/command-ref/nix-store/add-fixed.md index bebf15026a6..2ea90a13592 100644 --- a/doc/manual/source/command-ref/nix-store/add-fixed.md +++ b/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}} diff --git a/doc/manual/source/command-ref/nix-store/add.md b/doc/manual/source/command-ref/nix-store/add.md index 87d504cd333..ab474072311 100644 --- a/doc/manual/source/command-ref/nix-store/add.md +++ b/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}} From 3fb1d8912b6503fd817560dac7c6fdd782225eda Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 13 Dec 2024 18:46:47 +0100 Subject: [PATCH 4/7] refactor: Don't re-construct SourcePath unnecessarily --- src/nix-store/nix-store.cc | 8 ++++---- src/nix/add-to-store.cc | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index b731b25afed..659a237f94a 100644 --- a/src/nix-store/nix-store.cc +++ b/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(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(i); std::cout << fmt("%s\n", store->printStorePath(store->addToStoreSlow( baseNameOf(i), - {accessor, canonPath}, + sourcePath, method, hashAlgo).path)); } diff --git a/src/nix/add-to-store.cc b/src/nix/add-to-store.cc index 5c08f761662..45563cb2b40 100644 --- a/src/nix/add-to-store.cc +++ b/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(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)); } From 6e932e1cb8ce155f5f6c9b0e50944dbd8558ee6f Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 13 Dec 2024 18:54:36 +0100 Subject: [PATCH 5/7] fix: Resolve CLI parent symlinks before adding to store Fixes https://github.com/NixOS/nix/issues/11941 --- src/nix-store/nix-store.cc | 4 ++-- src/nix/add-to-store.cc | 2 +- tests/functional/add.sh | 41 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index 659a237f94a..99bb2c72601 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -183,7 +183,7 @@ static void opAdd(Strings opFlags, Strings opArgs) if (!opFlags.empty()) throw UsageError("unknown flag"); for (auto & i : opArgs) { - auto sourcePath = PosixSourceAccessor::createAtRoot(i); + auto sourcePath = PosixSourceAccessor::createAtRoot(makeParentCanonical(i)); cout << fmt("%s\n", store->printStorePath(store->addToStore( std::string(baseNameOf(i)), sourcePath))); } @@ -207,7 +207,7 @@ static void opAddFixed(Strings opFlags, Strings opArgs) opArgs.pop_front(); for (auto & i : opArgs) { - auto sourcePath = PosixSourceAccessor::createAtRoot(i); + auto sourcePath = PosixSourceAccessor::createAtRoot(makeParentCanonical(i)); std::cout << fmt("%s\n", store->printStorePath(store->addToStoreSlow( baseNameOf(i), sourcePath, diff --git a/src/nix/add-to-store.cc b/src/nix/add-to-store.cc index 45563cb2b40..7f15de374eb 100644 --- a/src/nix/add-to-store.cc +++ b/src/nix/add-to-store.cc @@ -37,7 +37,7 @@ struct CmdAddToStore : MixDryRun, StoreCommand { if (!namePart) namePart = baseNameOf(path); - auto sourcePath = PosixSourceAccessor::createAtRoot(path); + auto sourcePath = PosixSourceAccessor::createAtRoot(makeParentCanonical(path)); auto storePath = dryRun ? store->computeStorePath( diff --git a/tests/functional/add.sh b/tests/functional/add.sh index 3b37ee7d47f..0e6868d8f56 100755 --- a/tests/functional/add.sh +++ b/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 From 0c9be5622c9f35c9940e3d89f2200fa3628e0540 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 16 Dec 2024 09:53:36 +0100 Subject: [PATCH 6/7] test: Add hydraJobs.tests.functional_symlinked-home --- tests/nixos/default.nix | 2 ++ tests/nixos/functional/symlinked-home.nix | 25 +++++++++++++++++++++++ 2 files changed, 27 insertions(+) create mode 100644 tests/nixos/functional/symlinked-home.nix diff --git a/tests/nixos/default.nix b/tests/nixos/default.nix index ff1220f35d0..eade124d18d 100644 --- a/tests/nixos/default.nix +++ b/tests/nixos/default.nix @@ -157,6 +157,8 @@ in functional_root = runNixOSTestFor "x86_64-linux" ./functional/as-root.nix; + functional_symlinked-home = runNixOSTestFor "x86_64-linux" ./functional/symlinked-home.nix; + user-sandboxing = runNixOSTestFor "x86_64-linux" ./user-sandboxing; s3-binary-cache-store = runNixOSTestFor "x86_64-linux" ./s3-binary-cache-store.nix; diff --git a/tests/nixos/functional/symlinked-home.nix b/tests/nixos/functional/symlinked-home.nix new file mode 100644 index 00000000000..ac41c535f9f --- /dev/null +++ b/tests/nixos/functional/symlinked-home.nix @@ -0,0 +1,25 @@ +{ ... }: +{ + name = "functional-tests-on-nixos_user_symlinked-home"; + + imports = [ ./common.nix ]; + + nodes.machine = { + users.users.alice = { isNormalUser = true; }; + }; + + testScript = '' + machine.wait_for_unit("multi-user.target") + with subtest("prepare symlinked home"): + machine.succeed(""" + ( + set -x + mv /home/alice /home/alice.real + ln -s alice.real /home/alice + ) 1>&2 + """) + machine.succeed(""" + su --login --command "run-test-suite" alice >&2 + """) + ''; +} From 12d4c8d81aefa2e6cb3e8a399afbcf9918bb1da9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Sun, 15 Dec 2024 19:03:14 +0100 Subject: [PATCH 7/7] makeParentCanonical: test case where parent is empty --- src/libutil-tests/file-system.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/libutil-tests/file-system.cc b/src/libutil-tests/file-system.cc index 7ef804f3403..2c10d486986 100644 --- a/src/libutil-tests/file-system.cc +++ b/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("/"), "/"); +} }