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}} 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("/"), "/"); +} } 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..7fdaba8111e 100644 --- a/src/libutil/file-system.hh +++ b/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 diff --git a/src/libutil/posix-source-accessor.hh b/src/libutil/posix-source-accessor.hh index 40f60bb54b8..5d491e633ce 100644 --- a/src/libutil/posix-source-accessor.hh +++ b/src/libutil/posix-source-accessor.hh @@ -43,13 +43,25 @@ 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 * 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 diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index b731b25afed..99bb2c72601 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(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)); } diff --git a/src/nix/add-to-store.cc b/src/nix/add-to-store.cc index 5c08f761662..7f15de374eb 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(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)); } diff --git a/src/nix/hash.cc b/src/nix/hash.cc index 416cd19b317..eac421d1260 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(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 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/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 diff --git a/tests/functional/hash-path.sh b/tests/functional/hash-path.sh index 86d782a9589..4894ae39146 100755 --- a/tests/functional/hash-path.sh +++ b/tests/functional/hash-path.sh @@ -92,3 +92,32 @@ 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 +if [[ -e /bin ]]; then + ln -s /bin "$TEST_ROOT/symlink-to-bin" + h=$(nix hash path --mode nar --type sha256 --base32 "$TEST_ROOT/symlink-to-bin") + [[ 0z2mdmkd43l0ijdxfbj1y8vzli15yh9b09n3a3rrygmjshbyypsw == "$h" ]] # manually verified hash +fi 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..57c45d5d592 --- /dev/null +++ b/tests/nixos/functional/symlinked-home.nix @@ -0,0 +1,36 @@ +/** + This test runs the functional tests on a NixOS system where the home directory + is symlinked to another location. + + The purpose of this test is to find cases where Nix uses low-level operations + that don't support symlinks on paths that include them. + + It is not a substitute for more intricate, use case-specific tests, but helps + catch common issues. +*/ +# TODO: add symlinked tmpdir +{ ... }: +{ + 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 + """) + ''; +}