diff --git a/flake.nix b/flake.nix index c2461a2ebc7..8283ccf970d 100644 --- a/flake.nix +++ b/flake.nix @@ -403,7 +403,7 @@ devShells = let - makeShell = import ./packaging/dev-shell.nix { inherit inputs lib devFlake; }; + makeShell = import ./packaging/dev-shell.nix { inherit lib devFlake; }; prefixAttrs = prefix: lib.concatMapAttrs (k: v: { "${prefix}-${k}" = v; }); in forAllSystems ( diff --git a/packaging/dev-shell.nix b/packaging/dev-shell.nix index 8e1bb89368a..1b6c37f354d 100644 --- a/packaging/dev-shell.nix +++ b/packaging/dev-shell.nix @@ -1,6 +1,5 @@ { lib, - inputs, devFlake, }: @@ -117,7 +116,7 @@ pkgs.nixComponents.nix-util.overrideAttrs ( pkgs.buildPackages.changelog-d modular.pre-commit.settings.package (pkgs.writeScriptBin "pre-commit-hooks-install" modular.pre-commit.settings.installationScript) - inputs.nixfmt.packages.${pkgs.hostPlatform.system}.default + pkgs.buildPackages.nixfmt-rfc-style ] # TODO: Remove the darwin check once # https://github.com/NixOS/nixpkgs/pull/291814 is available diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 6a45f24b82a..6a907e57e10 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -263,7 +263,25 @@ EvalState::EvalState( auto storeFS = makeMountedSourceAccessor( { {CanonPath::root, makeEmptySourceAccessor()}, - {CanonPath(store->storeDir), makeFSSourceAccessor(realStoreDir)} + /* In the pure eval case, we can simply require + valid paths. However, in the *impure* eval + case this gets in the way of the union + mechanism, because an invalid access in the + upper layer will *not* be caught by the union + source accessor, but instead abort the entire + lookup. + + This happens when the store dir in the + ambient file system has a path (e.g. because + another Nix store there), but the relocated + store does not. + + TODO make the various source accessors doing + access control all throw the same time of + exception, and make union source accessor + catch it, so we don't need to do this hack. + */ + {CanonPath(store->storeDir), store->getFSAccessor(settings.pureEval)}, }); accessor = settings.pureEval ? storeFS diff --git a/src/libfetchers/store-path-accessor.cc b/src/libfetchers/store-path-accessor.cc index 528bf2a4f51..9d3b7c4ec09 100644 --- a/src/libfetchers/store-path-accessor.cc +++ b/src/libfetchers/store-path-accessor.cc @@ -5,11 +5,7 @@ namespace nix { ref makeStorePathAccessor(ref store, const StorePath & storePath) { - // FIXME: should use `store->getFSAccessor()` - auto root = std::filesystem::path{store->toRealPath(storePath)}; - auto accessor = makeFSSourceAccessor(root); - accessor->setPathDisplay(root.string()); - return accessor; + return projectSubdirSourceAccessor(store->getFSAccessor(), storePath.to_string()); } } diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index b765fc2a002..cb6ce028e29 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -568,7 +568,7 @@ bool Worker::pathContentsGood(const StorePath & path) res = false; else { auto current = hashPath( - {store.getFSAccessor(), CanonPath(store.printStorePath(path))}, + {store.getFSAccessor(), CanonPath(path.to_string())}, FileIngestionMethod::NixArchive, info->narHash.algo).first; Hash nullHash(HashAlgorithm::SHA256); res = info->narHash == nullHash || info->narHash == current; diff --git a/src/libstore/dummy-store.cc b/src/libstore/dummy-store.cc index c1e871e9384..9bacfb8b1de 100644 --- a/src/libstore/dummy-store.cc +++ b/src/libstore/dummy-store.cc @@ -83,7 +83,9 @@ struct DummyStore : public virtual DummyStoreConfig, public virtual Store { callback(nullptr); } virtual ref getFSAccessor(bool requireValidPath) override - { unsupported("getFSAccessor"); } + { + return makeEmptySourceAccessor(); + } }; static RegisterStoreImplementation regDummyStore; diff --git a/src/libstore/local-fs-store.cc b/src/libstore/local-fs-store.cc index 5449b20eb3b..f63585678e3 100644 --- a/src/libstore/local-fs-store.cc +++ b/src/libstore/local-fs-store.cc @@ -33,30 +33,37 @@ struct LocalStoreAccessor : PosixSourceAccessor bool requireValidPath; LocalStoreAccessor(ref store, bool requireValidPath) - : store(store) + : PosixSourceAccessor(std::filesystem::path{store->realStoreDir.get()}) + , store(store) , requireValidPath(requireValidPath) - { } + { + // Not `realStoreDir`, because this is where the store objects "morally" reside. + ownLocationForSymlinkResolution = store->storeDir; + } + - CanonPath toRealPath(const CanonPath & path) + void requireStoreObject(const CanonPath & path) { - auto [storePath, rest] = store->toStorePath(path.abs()); + auto [storePath, rest] = store->toStorePath(store->storeDir + path.abs()); if (requireValidPath && !store->isValidPath(storePath)) throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath)); - return CanonPath(store->getRealStoreDir()) / storePath.to_string() / CanonPath(rest); } std::optional maybeLstat(const CanonPath & path) override { - /* Handle the case where `path` is (a parent of) the store. */ - if (isDirOrInDir(store->storeDir, path.abs())) + /* Also allow `path` to point to the entire store, which is + needed for resolving symlinks. */ + if (path.isRoot()) return Stat{ .type = tDirectory }; - return PosixSourceAccessor::maybeLstat(toRealPath(path)); + requireStoreObject(path); + return PosixSourceAccessor::maybeLstat(path); } DirEntries readDirectory(const CanonPath & path) override { - return PosixSourceAccessor::readDirectory(toRealPath(path)); + requireStoreObject(path); + return PosixSourceAccessor::readDirectory(path); } void readFile( @@ -64,12 +71,14 @@ struct LocalStoreAccessor : PosixSourceAccessor Sink & sink, std::function sizeCallback) override { - return PosixSourceAccessor::readFile(toRealPath(path), sink, sizeCallback); + requireStoreObject(path); + return PosixSourceAccessor::readFile(path, sink, sizeCallback); } std::string readLink(const CanonPath & path) override { - return PosixSourceAccessor::readLink(toRealPath(path)); + requireStoreObject(path); + return PosixSourceAccessor::readLink(path); } }; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 67d5a1dcb7d..c5e8ff49a5a 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1104,7 +1104,7 @@ void LocalStore::addToStore(const ValidPathInfo & info, Source & source, auto & specified = *info.ca; auto actualHash = ({ auto accessor = getFSAccessor(false); - CanonPath path { printStorePath(info.path) }; + CanonPath path { info.path.to_string() }; Hash h { HashAlgorithm::SHA256 }; // throwaway def to appease C++ auto fim = specified.method.getFileIngestionMethod(); switch (fim) { diff --git a/src/libstore/remote-fs-accessor.cc b/src/libstore/remote-fs-accessor.cc index 7e360b5fef1..616c8443a14 100644 --- a/src/libstore/remote-fs-accessor.cc +++ b/src/libstore/remote-fs-accessor.cc @@ -15,6 +15,9 @@ RemoteFSAccessor::RemoteFSAccessor(ref store, bool requireValidPath, cons { if (cacheDir != "") createDirs(cacheDir); + + // Not `realStoreDir`, because this is where the store objects "morally" reside. + ownLocationForSymlinkResolution = store->storeDir; } Path RemoteFSAccessor::makeCacheFile(std::string_view hashPart, const std::string & ext) @@ -51,7 +54,7 @@ ref RemoteFSAccessor::addToCache(std::string_view hashPart, std: std::pair, CanonPath> RemoteFSAccessor::fetch(const CanonPath & path) { - auto [storePath, restPath_] = store->toStorePath(path.abs()); + auto [storePath, restPath_] = store->toStorePath(store->storeDir + path.abs()); auto restPath = CanonPath(restPath_); if (requireValidPath && !store->isValidPath(storePath)) diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 236622eae37..204f22eb58d 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1232,7 +1232,7 @@ static Derivation readDerivationCommon(Store & store, const StorePath & drvPath, auto accessor = store.getFSAccessor(requireValidPath); try { return parseDerivation(store, - accessor->readFile(CanonPath(store.printStorePath(drvPath))), + accessor->readFile(CanonPath(drvPath.to_string())), Derivation::nameFromPath(drvPath)); } catch (FormatError & e) { throw Error("error parsing derivation '%s': %s", store.printStorePath(drvPath), e.msg()); diff --git a/src/libutil/meson.build b/src/libutil/meson.build index df459f0e57f..35757b0646c 100644 --- a/src/libutil/meson.build +++ b/src/libutil/meson.build @@ -162,6 +162,7 @@ sources = files( 'signature/signer.cc', 'source-accessor.cc', 'source-path.cc', + 'subdir-source-accessor.cc', 'strings.cc', 'suggestions.cc', 'tarfile.cc', diff --git a/src/libutil/source-accessor.cc b/src/libutil/source-accessor.cc index 78f038cf377..2f5054b4967 100644 --- a/src/libutil/source-accessor.cc +++ b/src/libutil/source-accessor.cc @@ -114,9 +114,29 @@ CanonPath SourceAccessor::resolveSymlinks( if (!linksAllowed--) throw Error("infinite symlink recursion in path '%s'", showPath(path)); auto target = readLink(res); - res.pop(); - if (isAbsolute(target)) + if (isAbsolute(target)) { + auto prefixLen = ownLocationForSymlinkResolution.size(); + /* TODO need to support windows paths without making + Unix vulnerability. The problem is we don't know whether + the target (which is *not* a `CanonPath`) is a + Windows or Unix path. Because of remote stores to + other OSes, and abstract stores (which should + have host-OS-agnostic designs), we cannot simply + use whether we are running on Unix or Windows for + this. */ + if (!(hasPrefix(target, ownLocationForSymlinkResolution) && (target.size() == prefixLen || target[prefixLen] == '/'))) { + throw Error( + "cannot resolve '%s': " + "symlink target '%s' points outside source accessor that is considered to reside at '%s'", + showPath(path), + target, + ownLocationForSymlinkResolution); + } + target = std::move(target).substr(prefixLen); res = CanonPath::root; + } else { + res.pop(); + } todo.splice(todo.begin(), tokenizeString>(target, "/")); } } diff --git a/src/libutil/source-accessor.hh b/src/libutil/source-accessor.hh index 79ae092ac18..c49551d3d5c 100644 --- a/src/libutil/source-accessor.hh +++ b/src/libutil/source-accessor.hh @@ -176,6 +176,39 @@ struct SourceAccessor : std::enable_shared_from_this const CanonPath & path, SymlinkResolution mode = SymlinkResolution::Full); + /** + * When `resolveSymlinks` encounters an absolute target, that path + * may or may not denote a local inside the current source accessor + * --- there is no general answer to the question, it depends on + * what the current source accessor "represents". + * + * For now, we use this variable to decide: if the path has this + * path as a prefix, then the suffix points inside this store + * prefix. If the path does not have path as prefix, than the + * symlink escapes the source acccessor. + * + * Note, that we do *not* want this to become a general "location of + * this source accessor" variable: That would further violate the + * "don't know your own name principle. Indeed, the same source + * accessor may be used in multiple different contexts, at different + * locations, and so its "location" is a property of the caller + * rather than the thing itself. + * + * @todo Based on the above observations, we should instead get rid + * of this field, and separate out symlink resolving (other than the + * low-level, interpretation-*agnostic* `readLink`) from the source + * accessor itself. + * + * Until we do the above proper fix, the best we can do is *only* + * use this for `resolveSymlinks`. + * + * This variable does not include a trailing directory separator. + * Rather a directory separate must come after the prefix. For + * example, the empty string means the root path, i.e. any absolute + * path points within the source accessor. + */ + std::string ownLocationForSymlinkResolution = ""; + /** * A string that uniquely represents the contents of this * accessor. This is used for caching lookups (see `fetchToStore()`). @@ -222,4 +255,10 @@ ref makeMountedSourceAccessor(std::map makeUnionSourceAccessor(std::vector> && accessors); +/** + * Creates a new source accessor which is confined to the subdirectory + * of the given source accessor. + */ +ref projectSubdirSourceAccessor(ref, CanonPath subdirectory); + } diff --git a/src/libutil/subdir-source-accessor.cc b/src/libutil/subdir-source-accessor.cc new file mode 100644 index 00000000000..f033fcd1ad3 --- /dev/null +++ b/src/libutil/subdir-source-accessor.cc @@ -0,0 +1,56 @@ +#include "source-accessor.hh" + +namespace nix { + +struct SubdirSourceAccessor : SourceAccessor +{ + ref parent; + + CanonPath subdirectory; + + SubdirSourceAccessor(ref && parent, CanonPath && subdirectory) + : parent(std::move(parent)) + , subdirectory(std::move(subdirectory)) + { + displayPrefix.clear(); + + ownLocationForSymlinkResolution = this->parent->ownLocationForSymlinkResolution + subdirectory.abs(); + } + + std::string readFile(const CanonPath & path) override + { + return parent->readFile(subdirectory / path); + } + + bool pathExists(const CanonPath & path) override + { + return parent->pathExists(subdirectory / path); + } + + std::optional maybeLstat(const CanonPath & path) override + { + return parent->maybeLstat(subdirectory / path); + } + + DirEntries readDirectory(const CanonPath & path) override + { + return parent->readDirectory(subdirectory / path); + } + + std::string readLink(const CanonPath & path) override + { + return parent->readLink(subdirectory / path); + } + + std::string showPath(const CanonPath & path) override + { + return displayPrefix + parent->showPath(subdirectory / path) + displaySuffix; + } +}; + +ref projectSubdirSourceAccessor(ref parent, CanonPath subdirectory) +{ + return make_ref(std::move(parent), std::move(subdirectory)); +} + +} diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index d182b1eee57..d34de0276fe 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -563,7 +563,7 @@ static void registerValidity(bool reregister, bool hashGiven, bool canonicalise) #endif if (!hashGiven) { HashResult hash = hashPath( - {store->getFSAccessor(false), CanonPath { store->printStorePath(info->path) }}, + {store->getFSAccessor(false), CanonPath { info->path.to_string() }}, FileSerialisationMethod::NixArchive, HashAlgorithm::SHA256); info->narHash = hash.first; info->narSize = hash.second; diff --git a/src/nix/cat.cc b/src/nix/cat.cc index e0179c3486c..1dd8c550980 100644 --- a/src/nix/cat.cc +++ b/src/nix/cat.cc @@ -7,21 +7,21 @@ using namespace nix; struct MixCat : virtual Args { - std::string path; - - void cat(ref accessor) + void cat(ref accessor, CanonPath path) { - auto st = accessor->lstat(CanonPath(path)); + auto st = accessor->lstat(path); if (st.type != SourceAccessor::Type::tRegular) - throw Error("path '%1%' is not a regular file", path); + throw Error("path '%1%' is not a regular file", path.abs()); stopProgressBar(); - writeFull(getStandardOutput(), accessor->readFile(CanonPath(path))); + writeFull(getStandardOutput(), accessor->readFile(path)); } }; struct CmdCatStore : StoreCommand, MixCat { + std::string path; + CmdCatStore() { expectArgs({ @@ -45,7 +45,8 @@ struct CmdCatStore : StoreCommand, MixCat void run(ref store) override { - cat(store->getFSAccessor()); + auto [storePath, rest] = store->toStorePath(path); + cat(store->getFSAccessor(), CanonPath{storePath.to_string()} / CanonPath{rest}); } }; @@ -53,6 +54,8 @@ struct CmdCatNar : StoreCommand, MixCat { Path narPath; + std::string path; + CmdCatNar() { expectArgs({ @@ -77,7 +80,7 @@ struct CmdCatNar : StoreCommand, MixCat void run(ref store) override { - cat(makeNarAccessor(readFile(narPath))); + cat(makeNarAccessor(readFile(narPath)), CanonPath{path}); } }; diff --git a/src/nix/env.cc b/src/nix/env.cc index 832320320ae..0938744f6b8 100644 --- a/src/nix/env.cc +++ b/src/nix/env.cc @@ -87,8 +87,8 @@ struct CmdShell : InstallablesCommand, MixEnvironment if (true) pathAdditions.push_back(store->printStorePath(path) + "/bin"); - auto propPath = accessor->resolveSymlinks( - CanonPath(store->printStorePath(path)) / "nix-support" / "propagated-user-env-packages"); + auto propPath = + accessor->resolveSymlinks(CanonPath(path.to_string()) / "nix-support" / "propagated-user-env-packages"); if (auto st = accessor->maybeLstat(propPath); st && st->type == SourceAccessor::tRegular) { for (auto & p : tokenizeString(accessor->readFile(propPath))) todo.push(store->parseStorePath(p)); diff --git a/src/nix/ls.cc b/src/nix/ls.cc index 63f97f2d3b6..e7f17485464 100644 --- a/src/nix/ls.cc +++ b/src/nix/ls.cc @@ -8,8 +8,6 @@ using namespace nix; struct MixLs : virtual Args, MixJSON { - std::string path; - bool recursive = false; bool verbose = false; bool showDirectory = false; @@ -38,7 +36,7 @@ struct MixLs : virtual Args, MixJSON }); } - void listText(ref accessor) + void listText(ref accessor, CanonPath path) { std::function doPath; @@ -77,26 +75,27 @@ struct MixLs : virtual Args, MixJSON showFile(curPath, relPath); }; - auto path2 = CanonPath(path); - auto st = accessor->lstat(path2); - doPath(st, path2, - st.type == SourceAccessor::Type::tDirectory ? "." : path2.baseName().value_or(""), + auto st = accessor->lstat(path); + doPath(st, path, + st.type == SourceAccessor::Type::tDirectory ? "." : path.baseName().value_or(""), showDirectory); } - void list(ref accessor) + void list(ref accessor, CanonPath path) { if (json) { if (showDirectory) throw UsageError("'--directory' is useless with '--json'"); - logger->cout("%s", listNar(accessor, CanonPath(path), recursive)); + logger->cout("%s", listNar(accessor, path, recursive)); } else - listText(accessor); + listText(accessor, std::move(path)); } }; struct CmdLsStore : StoreCommand, MixLs { + std::string path; + CmdLsStore() { expectArgs({ @@ -120,7 +119,8 @@ struct CmdLsStore : StoreCommand, MixLs void run(ref store) override { - list(store->getFSAccessor()); + auto [storePath, rest] = store->toStorePath(path); + list(store->getFSAccessor(), CanonPath{storePath.to_string()} / CanonPath{rest}); } }; @@ -128,6 +128,8 @@ struct CmdLsNar : Command, MixLs { Path narPath; + std::string path; + CmdLsNar() { expectArgs({ @@ -152,7 +154,7 @@ struct CmdLsNar : Command, MixLs void run() override { - list(makeNarAccessor(readFile(narPath))); + list(makeNarAccessor(readFile(narPath)), CanonPath{path}); } }; diff --git a/src/nix/why-depends.cc b/src/nix/why-depends.cc index e299585ff88..9c6289e1137 100644 --- a/src/nix/why-depends.cc +++ b/src/nix/why-depends.cc @@ -175,7 +175,7 @@ struct CmdWhyDepends : SourceExprCommand, MixOperateOnOptions struct BailOut { }; printNode = [&](Node & node, const std::string & firstPad, const std::string & tailPad) { - CanonPath pathS(store->printStorePath(node.path)); + CanonPath pathS(node.path.to_string()); assert(node.dist != inf); if (precise) { diff --git a/tests/functional/shell.sh b/tests/functional/shell.sh index 51032ff1b75..bcb3c51630b 100755 --- a/tests/functional/shell.sh +++ b/tests/functional/shell.sh @@ -21,7 +21,7 @@ nix shell -f shell-hello.nix 'hello^*' -c hello2 | grep 'Hello2' nix shell -f shell-hello.nix hello-symlink -c hello | grep 'Hello World' # Test that symlinks outside of the store don't work. -expect 1 nix shell -f shell-hello.nix forbidden-symlink -c hello 2>&1 | grepQuiet "is not in the Nix store" +expect 1 nix shell -f shell-hello.nix forbidden-symlink -c hello 2>&1 | grepQuiet "points outside source accessor" # Test that we're not setting any more environment variables than necessary. # For instance, we might set an environment variable temporarily to affect some