From ea95327e72f5781295417b0eae46a5e351bebebd Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 30 Nov 2023 16:16:17 +0100 Subject: [PATCH 01/10] Move restricted/pure-eval access control out of the evaluator and into the accessor --- src/libcmd/installables.cc | 7 +- src/libexpr/eval.cc | 103 +++++---------------- src/libexpr/eval.hh | 25 +++--- src/libexpr/parser.y | 19 +++- src/libexpr/primops.cc | 119 +++++++++++-------------- src/nix-build/nix-build.cc | 7 +- src/nix-instantiate/nix-instantiate.cc | 2 +- tests/functional/restricted.sh | 4 +- 8 files changed, 115 insertions(+), 171 deletions(-) diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc index 6e670efea..6b3c82374 100644 --- a/src/libcmd/installables.cc +++ b/src/libcmd/installables.cc @@ -260,9 +260,10 @@ void SourceExprCommand::completeInstallable(AddCompletions & completions, std::s evalSettings.pureEval = false; auto state = getEvalState(); - Expr *e = state->parseExprFromFile( - resolveExprPath(state->checkSourcePath(lookupFileArg(*state, *file))) - ); + auto e = + state->parseExprFromFile( + resolveExprPath( + lookupFileArg(*state, *file))); Value root; state->eval(e, root); diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 7e68e6f9b..23ac349fe 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -509,7 +509,18 @@ EvalState::EvalState( , sOutputSpecified(symbols.create("outputSpecified")) , repair(NoRepair) , emptyBindings(0) - , rootFS(makeFSInputAccessor(CanonPath::root)) + , rootFS( + makeFSInputAccessor( + CanonPath::root, + evalSettings.restrictEval || evalSettings.pureEval + ? std::optional>(std::set()) + : std::nullopt, + [](const CanonPath & path) -> RestrictedPathError { + auto modeInformation = evalSettings.pureEval + ? "in pure evaluation mode (use '--impure' to override)" + : "in restricted mode"; + throw RestrictedPathError("access to absolute path '%1%' is forbidden %2%", path, modeInformation); + })) , corepkgsFS(makeMemoryInputAccessor()) , internalFS(makeMemoryInputAccessor()) , derivationInternal{corepkgsFS->addFile( @@ -551,28 +562,10 @@ EvalState::EvalState( searchPath.elements.emplace_back(SearchPath::Elem::parse(i)); } - if (evalSettings.restrictEval || evalSettings.pureEval) { - allowedPaths = PathSet(); - - for (auto & i : searchPath.elements) { - auto r = resolveSearchPathPath(i.path); - if (!r) continue; - - auto path = std::move(*r); - - if (store->isInStore(path)) { - try { - StorePathSet closure; - store->computeFSClosure(store->toStorePath(path).first, closure); - for (auto & path : closure) - allowPath(path); - } catch (InvalidPath &) { - allowPath(path); - } - } else - allowPath(path); - } - } + /* Allow access to all paths in the search path. */ + if (rootFS->hasAccessControl()) + for (auto & i : searchPath.elements) + resolveSearchPathPath(i.path, true); corepkgsFS->addFile( CanonPath("fetchurl.nix"), @@ -590,14 +583,12 @@ EvalState::~EvalState() void EvalState::allowPath(const Path & path) { - if (allowedPaths) - allowedPaths->insert(path); + rootFS->allowPath(CanonPath(path)); } void EvalState::allowPath(const StorePath & storePath) { - if (allowedPaths) - allowedPaths->insert(store->toRealPath(storePath)); + rootFS->allowPath(CanonPath(store->toRealPath(storePath))); } void EvalState::allowAndSetStorePathString(const StorePath & storePath, Value & v) @@ -607,54 +598,6 @@ void EvalState::allowAndSetStorePathString(const StorePath & storePath, Value & mkStorePathString(storePath, v); } -SourcePath EvalState::checkSourcePath(const SourcePath & path_) -{ - // Don't check non-rootFS accessors, they're in a different namespace. - if (path_.accessor != ref(rootFS)) return path_; - - if (!allowedPaths) return path_; - - auto i = resolvedPaths.find(path_.path.abs()); - if (i != resolvedPaths.end()) - return i->second; - - bool found = false; - - /* First canonicalize the path without symlinks, so we make sure an - * attacker can't append ../../... to a path that would be in allowedPaths - * and thus leak symlink targets. - */ - Path abspath = canonPath(path_.path.abs()); - - for (auto & i : *allowedPaths) { - if (isDirOrInDir(abspath, i)) { - found = true; - break; - } - } - - if (!found) { - auto modeInformation = evalSettings.pureEval - ? "in pure eval mode (use '--impure' to override)" - : "in restricted mode"; - throw RestrictedPathError("access to absolute path '%1%' is forbidden %2%", abspath, modeInformation); - } - - /* Resolve symlinks. */ - debug("checking access to '%s'", abspath); - SourcePath path = rootPath(CanonPath(canonPath(abspath, true))); - - for (auto & i : *allowedPaths) { - if (isDirOrInDir(path.path.abs(), i)) { - resolvedPaths.insert_or_assign(path_.path.abs(), path); - return path; - } - } - - throw RestrictedPathError("access to canonical path '%1%' is forbidden in restricted mode", path); -} - - void EvalState::checkURI(const std::string & uri) { if (!evalSettings.restrictEval) return; @@ -674,12 +617,12 @@ void EvalState::checkURI(const std::string & uri) /* If the URI is a path, then check it against allowedPaths as well. */ if (hasPrefix(uri, "/")) { - checkSourcePath(rootPath(CanonPath(uri))); + rootFS->checkAllowed(CanonPath(uri)); return; } if (hasPrefix(uri, "file://")) { - checkSourcePath(rootPath(CanonPath(std::string(uri, 7)))); + rootFS->checkAllowed(CanonPath(uri.substr(7))); return; } @@ -1181,10 +1124,8 @@ Value * ExprPath::maybeThunk(EvalState & state, Env & env) } -void EvalState::evalFile(const SourcePath & path_, Value & v, bool mustBeTrivial) +void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial) { - auto path = checkSourcePath(path_); - FileEvalCache::iterator i; if ((i = fileEvalCache.find(path)) != fileEvalCache.end()) { v = i->second; @@ -1205,7 +1146,7 @@ void EvalState::evalFile(const SourcePath & path_, Value & v, bool mustBeTrivial e = j->second; if (!e) - e = parseExprFromFile(checkSourcePath(resolvedPath)); + e = parseExprFromFile(resolvedPath); fileParseCache[resolvedPath] = e; diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 9a92992c1..ee7bdda0d 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -217,12 +217,6 @@ public: */ RepairFlag repair; - /** - * The allowed filesystem paths in restricted or pure evaluation - * mode. - */ - std::optional allowedPaths; - Bindings emptyBindings; /** @@ -396,12 +390,6 @@ public: */ void allowAndSetStorePathString(const StorePath & storePath, Value & v); - /** - * Check whether access to a path is allowed and throw an error if - * not. Otherwise return the canonicalised path. - */ - SourcePath checkSourcePath(const SourcePath & path); - void checkURI(const std::string & uri); /** @@ -445,13 +433,15 @@ public: SourcePath findFile(const SearchPath & searchPath, const std::string_view path, const PosIdx pos = noPos); /** - * Try to resolve a search path value (not the optional key part) + * Try to resolve a search path value (not the optional key part). * * If the specified search path element is a URI, download it. * * If it is not found, return `std::nullopt` */ - std::optional resolveSearchPathPath(const SearchPath::Path & path); + std::optional resolveSearchPathPath( + const SearchPath::Path & elem, + bool initAccessControl = false); /** * Evaluate an expression to normal form @@ -756,6 +746,13 @@ public: */ [[nodiscard]] StringMap realiseContext(const NixStringContext & context); + /* Call the binary path filter predicate used builtins.path etc. */ + bool callPathFilter( + Value * filterFun, + const SourcePath & path, + std::string_view pathArg, + PosIdx pos); + private: /** diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index f6cf1f689..58fc580fc 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -783,7 +783,7 @@ SourcePath EvalState::findFile(const SearchPath & searchPath, const std::string_ } -std::optional EvalState::resolveSearchPathPath(const SearchPath::Path & value0) +std::optional EvalState::resolveSearchPathPath(const SearchPath::Path & value0, bool initAccessControl) { auto & value = value0.s; auto i = searchPathResolved.find(value); @@ -800,7 +800,6 @@ std::optional EvalState::resolveSearchPathPath(const SearchPath::Pa logWarning({ .msg = hintfmt("Nix search path entry '%1%' cannot be downloaded, ignoring", value) }); - res = std::nullopt; } } @@ -814,6 +813,20 @@ std::optional EvalState::resolveSearchPathPath(const SearchPath::Pa else { auto path = absPath(value); + + /* Allow access to paths in the search path. */ + if (initAccessControl) { + allowPath(path); + if (store->isInStore(path)) { + try { + StorePathSet closure; + store->computeFSClosure(store->toStorePath(path).first, closure); + for (auto & p : closure) + allowPath(p); + } catch (InvalidPath &) { } + } + } + if (pathExists(path)) res = { path }; else { @@ -829,7 +842,7 @@ std::optional EvalState::resolveSearchPathPath(const SearchPath::Pa else debug("failed to resolve search path element '%s'", value); - searchPathResolved[value] = res; + searchPathResolved.emplace(value, res); return res; } diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index ebf2549e4..0f7706563 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -15,6 +15,7 @@ #include "value-to-json.hh" #include "value-to-xml.hh" #include "primops.hh" +#include "fs-input-accessor.hh" #include #include @@ -90,8 +91,8 @@ StringMap EvalState::realiseContext(const NixStringContext & context) for (auto & [outputName, outputPath] : outputs) { /* Add the output of this derivations to the allowed paths. */ - if (allowedPaths) { - allowPath(outputPath); + if (rootFS->hasAccessControl()) { + allowPath(store->toRealPath(outputPath)); } /* Get all the output paths corresponding to the placeholders we had */ if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) { @@ -110,27 +111,19 @@ StringMap EvalState::realiseContext(const NixStringContext & context) return res; } -struct RealisePathFlags { - // Whether to check that the path is allowed in pure eval mode - bool checkForPureEval = true; -}; - -static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, const RealisePathFlags flags = {}) +static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v) { NixStringContext context; auto path = state.coerceToPath(noPos, v, context, "while realising the context of a path"); try { - if (!context.empty()) { + if (!context.empty() && path.accessor == state.rootFS) { auto rewrites = state.realiseContext(context); auto realPath = state.toRealPath(rewriteStrings(path.path.abs(), rewrites), context); return {path.accessor, CanonPath(realPath)}; - } - - return flags.checkForPureEval - ? state.checkSourcePath(path) - : path; + } else + return path; } catch (Error & e) { e.addTrace(state.positions[pos], "while realising the context of path '%s'", path); throw; @@ -1493,7 +1486,7 @@ static void prim_storePath(EvalState & state, const PosIdx pos, Value * * args, })); NixStringContext context; - auto path = state.checkSourcePath(state.coerceToPath(pos, *args[0], context, "while evaluating the first argument passed to 'builtins.storePath'")).path; + auto path = state.coerceToPath(pos, *args[0], context, "while evaluating the first argument passed to 'builtins.storePath'").path; /* Resolve symlinks in ‘path’, unless ‘path’ itself is a symlink directly in the store. The latter condition is necessary so e.g. nix-push does the right thing. */ @@ -1535,12 +1528,7 @@ static void prim_pathExists(EvalState & state, const PosIdx pos, Value * * args, { auto & arg = *args[0]; - /* We don’t check the path right now, because we don’t want to - throw if the path isn’t allowed, but just return false (and we - can’t just catch the exception here because we still want to - throw if something in the evaluation of `arg` tries to - access an unauthorized path). */ - auto path = realisePath(state, pos, arg, { .checkForPureEval = false }); + auto path = realisePath(state, pos, arg); /* SourcePath doesn't know about trailing slash. */ auto mustBeDir = arg.type() == nString @@ -1548,14 +1536,9 @@ static void prim_pathExists(EvalState & state, const PosIdx pos, Value * * args, || arg.string_view().ends_with("/.")); try { - auto checked = state.checkSourcePath(path); - auto st = checked.maybeLstat(); + auto st = path.maybeLstat(); auto exists = st && (!mustBeDir || st->type == SourceAccessor::tDirectory); v.mkBool(exists); - } catch (SysError & e) { - /* Don't give away info from errors while canonicalising - ‘path’ in restricted mode. */ - v.mkBool(false); } catch (RestrictedPathError & e) { v.mkBool(false); } @@ -1699,7 +1682,7 @@ static void prim_findFile(EvalState & state, const PosIdx pos, Value * * args, V auto path = state.forceStringNoCtx(*args[1], pos, "while evaluating the second argument passed to builtins.findFile"); - v.mkPath(state.checkSourcePath(state.findFile(searchPath, path, pos))); + v.mkPath(state.findFile(searchPath, path, pos)); } static RegisterPrimOp primop_findFile(PrimOp { @@ -2178,11 +2161,35 @@ static RegisterPrimOp primop_toFile({ .fun = prim_toFile, }); +bool EvalState::callPathFilter( + Value * filterFun, + const SourcePath & path, + std::string_view pathArg, + PosIdx pos) +{ + auto st = path.lstat(); + + /* Call the filter function. The first argument is the path, the + second is a string indicating the type of the file. */ + Value arg1; + arg1.mkString(pathArg); + + Value arg2; + // assert that type is not "unknown" + arg2.mkString(fileTypeToString(st.type)); + + Value * args []{&arg1, &arg2}; + Value res; + callFunction(*filterFun, 2, args, res, pos); + + return forceBool(res, pos, "while evaluating the return value of the path filter function"); +} + static void addPath( EvalState & state, const PosIdx pos, std::string_view name, - Path path, + SourcePath path, Value * filterFun, FileIngestionMethod method, const std::optional expectedHash, @@ -2190,48 +2197,29 @@ static void addPath( const NixStringContext & context) { try { - // FIXME: handle CA derivation outputs (where path needs to - // be rewritten to the actual output). - auto rewrites = state.realiseContext(context); - path = state.toRealPath(rewriteStrings(path, rewrites), context); - StorePathSet refs; - if (state.store->isInStore(path)) { + if (path.accessor == state.rootFS && state.store->isInStore(path.path.abs())) { + // FIXME: handle CA derivation outputs (where path needs to + // be rewritten to the actual output). + auto rewrites = state.realiseContext(context); + path = {state.rootFS, CanonPath(state.toRealPath(rewriteStrings(path.path.abs(), rewrites), context))}; + try { - auto [storePath, subPath] = state.store->toStorePath(path); + auto [storePath, subPath] = state.store->toStorePath(path.path.abs()); // FIXME: we should scanForReferences on the path before adding it refs = state.store->queryPathInfo(storePath)->references; - path = state.store->toRealPath(storePath) + subPath; + path = {state.rootFS, CanonPath(state.store->toRealPath(storePath) + subPath)}; } catch (Error &) { // FIXME: should be InvalidPathError } } - path = evalSettings.pureEval && expectedHash - ? path - : state.checkSourcePath(state.rootPath(CanonPath(path))).path.abs(); - - PathFilter filter = filterFun ? ([&](const Path & path) { - auto st = lstat(path); - - /* Call the filter function. The first argument is the path, - the second is a string indicating the type of the file. */ - Value arg1; - arg1.mkString(path); - - Value arg2; - arg2.mkString( - S_ISREG(st.st_mode) ? "regular" : - S_ISDIR(st.st_mode) ? "directory" : - S_ISLNK(st.st_mode) ? "symlink" : - "unknown" /* not supported, will fail! */); - - Value * args []{&arg1, &arg2}; - Value res; - state.callFunction(*filterFun, 2, args, res, pos); - - return state.forceBool(res, pos, "while evaluating the return value of the path filter function"); - }) : defaultPathFilter; + std::unique_ptr filter; + if (filterFun) + filter = std::make_unique([&](const Path & p) { + auto p2 = CanonPath(p); + return state.callPathFilter(filterFun, {path.accessor, p2}, p2.abs(), pos); + }); std::optional expectedStorePath; if (expectedHash) @@ -2242,7 +2230,7 @@ static void addPath( }); if (!expectedHash || !state.store->isValidPath(*expectedStorePath)) { - auto dstPath = state.rootPath(CanonPath(path)).fetchToStore(state.store, name, method, &filter, state.repair); + auto dstPath = path.fetchToStore(state.store, name, method, filter.get(), state.repair); if (expectedHash && expectedStorePath != dstPath) state.debugThrowLastTrace(Error("store path mismatch in (possibly filtered) path added from '%s'", path)); state.allowAndSetStorePathString(dstPath, v); @@ -2261,7 +2249,8 @@ static void prim_filterSource(EvalState & state, const PosIdx pos, Value * * arg auto path = state.coerceToPath(pos, *args[1], context, "while evaluating the second argument (the path to filter) passed to 'builtins.filterSource'"); state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.filterSource"); - addPath(state, pos, path.baseName(), path.path.abs(), args[0], FileIngestionMethod::Recursive, std::nullopt, v, context); + + addPath(state, pos, path.baseName(), path, args[0], FileIngestionMethod::Recursive, std::nullopt, v, context); } static RegisterPrimOp primop_filterSource({ @@ -2356,7 +2345,7 @@ static void prim_path(EvalState & state, const PosIdx pos, Value * * args, Value if (name.empty()) name = path->baseName(); - addPath(state, pos, name, path->path.abs(), filterFun, method, expectedHash, v, context); + addPath(state, pos, name, *path, filterFun, method, expectedHash, v, context); } static RegisterPrimOp primop_path({ diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 75ce12a8c..e2986bfe0 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -310,8 +310,11 @@ static void main_nix_build(int argc, char * * argv) else /* If we're in a #! script, interpret filenames relative to the script. */ - exprs.push_back(state->parseExprFromFile(resolveExprPath(state->checkSourcePath(lookupFileArg(*state, - inShebang && !packages ? absPath(i, absPath(dirOf(script))) : i))))); + exprs.push_back( + state->parseExprFromFile( + resolveExprPath( + lookupFileArg(*state, + inShebang && !packages ? absPath(i, absPath(dirOf(script))) : i)))); } } diff --git a/src/nix-instantiate/nix-instantiate.cc b/src/nix-instantiate/nix-instantiate.cc index c67409e89..86b9be17d 100644 --- a/src/nix-instantiate/nix-instantiate.cc +++ b/src/nix-instantiate/nix-instantiate.cc @@ -183,7 +183,7 @@ static int main_nix_instantiate(int argc, char * * argv) for (auto & i : files) { Expr * e = fromArgs ? state->parseExprFromString(i, state->rootPath(CanonPath::fromCwd())) - : state->parseExprFromFile(resolveExprPath(state->checkSourcePath(lookupFileArg(*state, i)))); + : state->parseExprFromFile(resolveExprPath(lookupFileArg(*state, i))); processExpr(*state, attrPaths, parseOnly, strict, autoArgs, evalOnly, outputKind, xmlOutputSourceLocation, e); } diff --git a/tests/functional/restricted.sh b/tests/functional/restricted.sh index 197ae7a10..b8deceacc 100644 --- a/tests/functional/restricted.sh +++ b/tests/functional/restricted.sh @@ -14,8 +14,8 @@ nix-instantiate --restrict-eval --eval -E 'builtins.readFile ./simple.nix' -I sr (! nix-instantiate --restrict-eval --eval -E 'builtins.readDir ../../src/nix-channel') nix-instantiate --restrict-eval --eval -E 'builtins.readDir ../../src/nix-channel' -I src=../../src -(! nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in ') -nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in ' -I src=. +(! nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in builtins.readFile ') +nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in builtins.readFile ' -I src=. p=$(nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval --allowed-uris "file://$(pwd)") cmp $p restricted.sh From 305939655a6cd680997981ca6077d4ce7f957984 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 30 Nov 2023 16:28:33 +0100 Subject: [PATCH 02/10] Remove superfluous use of hasAccessControl() --- src/libexpr/primops.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 0f7706563..c442de986 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -91,9 +91,8 @@ StringMap EvalState::realiseContext(const NixStringContext & context) for (auto & [outputName, outputPath] : outputs) { /* Add the output of this derivations to the allowed paths. */ - if (rootFS->hasAccessControl()) { - allowPath(store->toRealPath(outputPath)); - } + allowPath(store->toRealPath(outputPath)); + /* Get all the output paths corresponding to the placeholders we had */ if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) { res.insert_or_assign( From 43d9fb6cf180c421be17b4247f5dd032cf4843f5 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 30 Nov 2023 16:44:54 +0100 Subject: [PATCH 03/10] Remove InputAccessor::root() --- src/libexpr/value.hh | 7 +++---- src/libfetchers/fetchers.cc | 2 +- src/libfetchers/input-accessor.cc | 7 +------ src/libfetchers/input-accessor.hh | 7 +++++-- 4 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index bcff8ae55..72a3a2b32 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -424,10 +424,9 @@ public: SourcePath path() const { assert(internalType == tPath); - return SourcePath { - .accessor = ref(_path.accessor->shared_from_this()), - .path = CanonPath(CanonPath::unchecked_t(), _path.path) - }; + return SourcePath( + ref(_path.accessor->shared_from_this()), + CanonPath(CanonPath::unchecked_t(), _path.path)); } std::string_view string_view() const diff --git a/src/libfetchers/fetchers.cc b/src/libfetchers/fetchers.cc index 60208619e..5fd9e069f 100644 --- a/src/libfetchers/fetchers.cc +++ b/src/libfetchers/fetchers.cc @@ -374,7 +374,7 @@ void InputScheme::clone(const Input & input, const Path & destDir) const std::pair InputScheme::fetch(ref store, const Input & input) { auto [accessor, input2] = getAccessor(store, input); - auto storePath = accessor->root().fetchToStore(store, input2.getName()); + auto storePath = SourcePath(accessor).fetchToStore(store, input2.getName()); return {storePath, input2}; } diff --git a/src/libfetchers/input-accessor.cc b/src/libfetchers/input-accessor.cc index 85dc4609f..f54a5a6fd 100644 --- a/src/libfetchers/input-accessor.cc +++ b/src/libfetchers/input-accessor.cc @@ -53,11 +53,6 @@ StorePath InputAccessor::fetchToStore( return storePath; } -SourcePath InputAccessor::root() -{ - return {ref(shared_from_this()), CanonPath::root}; -} - std::ostream & operator << (std::ostream & str, const SourcePath & path) { str << path.to_string(); @@ -88,7 +83,7 @@ SourcePath SourcePath::parent() const SourcePath SourcePath::resolveSymlinks() const { - auto res = accessor->root(); + auto res = SourcePath(accessor); int linksAllowed = 1024; diff --git a/src/libfetchers/input-accessor.hh b/src/libfetchers/input-accessor.hh index 26d17f064..d5ac238b1 100644 --- a/src/libfetchers/input-accessor.hh +++ b/src/libfetchers/input-accessor.hh @@ -36,8 +36,6 @@ struct InputAccessor : virtual SourceAccessor, std::enable_shared_from_this accessor; CanonPath path; + SourcePath(ref accessor, CanonPath path = CanonPath::root) + : accessor(std::move(accessor)) + , path(std::move(path)) + { } + std::string_view baseName() const; /** From 8cafc754d845529a78595d1196769257ee23ca56 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 30 Nov 2023 21:54:53 +0100 Subject: [PATCH 04/10] Move access control from FSInputAccessor to FilteringInputAccessor --- src/libexpr/eval.cc | 23 +++--- src/libexpr/eval.hh | 3 +- src/libfetchers/filtering-input-accessor.cc | 83 +++++++++++++++++++++ src/libfetchers/filtering-input-accessor.hh | 73 ++++++++++++++++++ src/libfetchers/fs-input-accessor.cc | 77 +++---------------- src/libfetchers/fs-input-accessor.hh | 22 +----- src/libfetchers/git.cc | 6 +- 7 files changed, 191 insertions(+), 96 deletions(-) create mode 100644 src/libfetchers/filtering-input-accessor.cc create mode 100644 src/libfetchers/filtering-input-accessor.hh diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 23ac349fe..841c223cd 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -14,6 +14,7 @@ #include "profiles.hh" #include "print.hh" #include "fs-input-accessor.hh" +#include "filtering-input-accessor.hh" #include "memory-input-accessor.hh" #include "signals.hh" #include "gc-small-vector.hh" @@ -510,17 +511,15 @@ EvalState::EvalState( , repair(NoRepair) , emptyBindings(0) , rootFS( - makeFSInputAccessor( - CanonPath::root, - evalSettings.restrictEval || evalSettings.pureEval - ? std::optional>(std::set()) - : std::nullopt, + evalSettings.restrictEval || evalSettings.pureEval + ? ref(AllowListInputAccessor::create(makeFSInputAccessor(CanonPath::root), {}, [](const CanonPath & path) -> RestrictedPathError { auto modeInformation = evalSettings.pureEval ? "in pure evaluation mode (use '--impure' to override)" : "in restricted mode"; throw RestrictedPathError("access to absolute path '%1%' is forbidden %2%", path, modeInformation); })) + : makeFSInputAccessor(CanonPath::root)) , corepkgsFS(makeMemoryInputAccessor()) , internalFS(makeMemoryInputAccessor()) , derivationInternal{corepkgsFS->addFile( @@ -563,7 +562,7 @@ EvalState::EvalState( } /* Allow access to all paths in the search path. */ - if (rootFS->hasAccessControl()) + if (rootFS.dynamic_pointer_cast()) for (auto & i : searchPath.elements) resolveSearchPathPath(i.path, true); @@ -583,12 +582,14 @@ EvalState::~EvalState() void EvalState::allowPath(const Path & path) { - rootFS->allowPath(CanonPath(path)); + if (auto rootFS2 = rootFS.dynamic_pointer_cast()) + rootFS2->allowPath(CanonPath(path)); } void EvalState::allowPath(const StorePath & storePath) { - rootFS->allowPath(CanonPath(store->toRealPath(storePath))); + if (auto rootFS2 = rootFS.dynamic_pointer_cast()) + rootFS2->allowPath(CanonPath(store->toRealPath(storePath))); } void EvalState::allowAndSetStorePathString(const StorePath & storePath, Value & v) @@ -617,12 +618,14 @@ void EvalState::checkURI(const std::string & uri) /* If the URI is a path, then check it against allowedPaths as well. */ if (hasPrefix(uri, "/")) { - rootFS->checkAllowed(CanonPath(uri)); + if (auto rootFS2 = rootFS.dynamic_pointer_cast()) + rootFS2->checkAccess(CanonPath(uri)); return; } if (hasPrefix(uri, "file://")) { - rootFS->checkAllowed(CanonPath(uri.substr(7))); + if (auto rootFS2 = rootFS.dynamic_pointer_cast()) + rootFS2->checkAccess(CanonPath(uri.substr(7))); return; } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index ee7bdda0d..f3f6d35b9 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -30,7 +30,6 @@ class EvalState; class StorePath; struct SingleDerivedPath; enum RepairFlag : bool; -struct FSInputAccessor; struct MemoryInputAccessor; @@ -222,7 +221,7 @@ public: /** * The accessor for the root filesystem. */ - const ref rootFS; + const ref rootFS; /** * The in-memory filesystem for paths. diff --git a/src/libfetchers/filtering-input-accessor.cc b/src/libfetchers/filtering-input-accessor.cc new file mode 100644 index 000000000..5ae416fd3 --- /dev/null +++ b/src/libfetchers/filtering-input-accessor.cc @@ -0,0 +1,83 @@ +#include "filtering-input-accessor.hh" + +namespace nix { + +std::string FilteringInputAccessor::readFile(const CanonPath & path) +{ + checkAccess(path); + return next->readFile(prefix + path); +} + +bool FilteringInputAccessor::pathExists(const CanonPath & path) +{ + return isAllowed(path) && next->pathExists(prefix + path); +} + +std::optional FilteringInputAccessor::maybeLstat(const CanonPath & path) +{ + checkAccess(path); + return next->maybeLstat(prefix + path); +} + +InputAccessor::DirEntries FilteringInputAccessor::readDirectory(const CanonPath & path) +{ + checkAccess(path); + DirEntries entries; + for (auto & entry : next->readDirectory(prefix + path)) { + if (isAllowed(path + entry.first)) + entries.insert(std::move(entry)); + } + return entries; +} + +std::string FilteringInputAccessor::readLink(const CanonPath & path) +{ + checkAccess(path); + return next->readLink(prefix + path); +} + +std::string FilteringInputAccessor::showPath(const CanonPath & path) +{ + return next->showPath(prefix + path); +} + +void FilteringInputAccessor::checkAccess(const CanonPath & path) +{ + if (!isAllowed(path)) + throw makeNotAllowedError + ? makeNotAllowedError(path) + : RestrictedPathError("access to path '%s' is forbidden", showPath(path)); +} + +struct AllowListInputAccessorImpl : AllowListInputAccessor +{ + std::set allowedPaths; + + AllowListInputAccessorImpl( + ref next, + std::set && allowedPaths, + MakeNotAllowedError && makeNotAllowedError) + : AllowListInputAccessor(SourcePath(next), std::move(makeNotAllowedError)) + , allowedPaths(std::move(allowedPaths)) + { } + + bool isAllowed(const CanonPath & path) override + { + return path.isAllowed(allowedPaths); + } + + void allowPath(CanonPath path) override + { + allowedPaths.insert(std::move(path)); + } +}; + +ref AllowListInputAccessor::create( + ref next, + std::set && allowedPaths, + MakeNotAllowedError && makeNotAllowedError) +{ + return make_ref(next, std::move(allowedPaths), std::move(makeNotAllowedError)); +} + +} diff --git a/src/libfetchers/filtering-input-accessor.hh b/src/libfetchers/filtering-input-accessor.hh new file mode 100644 index 000000000..209d26974 --- /dev/null +++ b/src/libfetchers/filtering-input-accessor.hh @@ -0,0 +1,73 @@ +#pragma once + +#include "input-accessor.hh" + +namespace nix { + +/** + * A function that should throw an exception of type + * `RestrictedPathError` explaining that access to `path` is + * forbidden. + */ +typedef std::function MakeNotAllowedError; + +/** + * An abstract wrapping `InputAccessor` that performs access + * control. Subclasses should override `checkAccess()` to implement an + * access control policy. + */ +struct FilteringInputAccessor : InputAccessor +{ + ref next; + CanonPath prefix; + MakeNotAllowedError makeNotAllowedError; + + FilteringInputAccessor(const SourcePath & src, MakeNotAllowedError && makeNotAllowedError) + : next(src.accessor) + , prefix(src.path) + , makeNotAllowedError(std::move(makeNotAllowedError)) + { } + + std::string readFile(const CanonPath & path) override; + + bool pathExists(const CanonPath & path) override; + + std::optional maybeLstat(const CanonPath & path) override; + + DirEntries readDirectory(const CanonPath & path) override; + + std::string readLink(const CanonPath & path) override; + + std::string showPath(const CanonPath & path) override; + + /** + * Call `makeNotAllowedError` to throw a `RestrictedPathError` + * exception if `isAllowed()` returns `false` for `path`. + */ + void checkAccess(const CanonPath & path); + + /** + * Return `true` iff access to path is allowed. + */ + virtual bool isAllowed(const CanonPath & path) = 0; +}; + +/** + * A wrapping `InputAccessor` that checks paths against an allow-list. + */ +struct AllowListInputAccessor : public FilteringInputAccessor +{ + /** + * Grant access to the specified path. + */ + virtual void allowPath(CanonPath path) = 0; + + static ref create( + ref next, + std::set && allowedPaths, + MakeNotAllowedError && makeNotAllowedError); + + using FilteringInputAccessor::FilteringInputAccessor; +}; + +} diff --git a/src/libfetchers/fs-input-accessor.cc b/src/libfetchers/fs-input-accessor.cc index 2efee932d..c3d8d273c 100644 --- a/src/libfetchers/fs-input-accessor.cc +++ b/src/libfetchers/fs-input-accessor.cc @@ -4,19 +4,12 @@ namespace nix { -struct FSInputAccessorImpl : FSInputAccessor, PosixSourceAccessor +struct FSInputAccessor : InputAccessor, PosixSourceAccessor { CanonPath root; - std::optional> allowedPaths; - MakeNotAllowedError makeNotAllowedError; - FSInputAccessorImpl( - const CanonPath & root, - std::optional> && allowedPaths, - MakeNotAllowedError && makeNotAllowedError) + FSInputAccessor(const CanonPath & root) : root(root) - , allowedPaths(std::move(allowedPaths)) - , makeNotAllowedError(std::move(makeNotAllowedError)) { displayPrefix = root.isRoot() ? "" : root.abs(); } @@ -27,39 +20,30 @@ struct FSInputAccessorImpl : FSInputAccessor, PosixSourceAccessor std::function sizeCallback) override { auto absPath = makeAbsPath(path); - checkAllowed(absPath); PosixSourceAccessor::readFile(absPath, sink, sizeCallback); } bool pathExists(const CanonPath & path) override { - auto absPath = makeAbsPath(path); - return isAllowed(absPath) && PosixSourceAccessor::pathExists(absPath); + return PosixSourceAccessor::pathExists(makeAbsPath(path)); } std::optional maybeLstat(const CanonPath & path) override { - auto absPath = makeAbsPath(path); - checkAllowed(absPath); - return PosixSourceAccessor::maybeLstat(absPath); + return PosixSourceAccessor::maybeLstat(makeAbsPath(path)); } DirEntries readDirectory(const CanonPath & path) override { - auto absPath = makeAbsPath(path); - checkAllowed(absPath); DirEntries res; - for (auto & entry : PosixSourceAccessor::readDirectory(absPath)) - if (isAllowed(absPath + entry.first)) - res.emplace(entry); + for (auto & entry : PosixSourceAccessor::readDirectory(makeAbsPath(path))) + res.emplace(entry); return res; } std::string readLink(const CanonPath & path) override { - auto absPath = makeAbsPath(path); - checkAllowed(absPath); - return PosixSourceAccessor::readLink(absPath); + return PosixSourceAccessor::readLink(makeAbsPath(path)); } CanonPath makeAbsPath(const CanonPath & path) @@ -67,59 +51,22 @@ struct FSInputAccessorImpl : FSInputAccessor, PosixSourceAccessor return root + path; } - void checkAllowed(const CanonPath & absPath) override - { - if (!isAllowed(absPath)) - throw makeNotAllowedError - ? makeNotAllowedError(absPath) - : RestrictedPathError("access to path '%s' is forbidden", absPath); - } - - bool isAllowed(const CanonPath & absPath) - { - if (!absPath.isWithin(root)) - return false; - - if (allowedPaths) { - auto p = absPath.removePrefix(root); - if (!p.isAllowed(*allowedPaths)) - return false; - } - - return true; - } - - void allowPath(CanonPath path) override - { - if (allowedPaths) - allowedPaths->insert(std::move(path)); - } - - bool hasAccessControl() override - { - return (bool) allowedPaths; - } - std::optional getPhysicalPath(const CanonPath & path) override { return makeAbsPath(path); } }; -ref makeFSInputAccessor( - const CanonPath & root, - std::optional> && allowedPaths, - MakeNotAllowedError && makeNotAllowedError) +ref makeFSInputAccessor(const CanonPath & root) { - return make_ref(root, std::move(allowedPaths), std::move(makeNotAllowedError)); + return make_ref(root); } -ref makeStorePathAccessor( +ref makeStorePathAccessor( ref store, - const StorePath & storePath, - MakeNotAllowedError && makeNotAllowedError) + const StorePath & storePath) { - return makeFSInputAccessor(CanonPath(store->toRealPath(storePath)), {}, std::move(makeNotAllowedError)); + return makeFSInputAccessor(CanonPath(store->toRealPath(storePath))); } SourcePath getUnfilteredRootPath(CanonPath path) diff --git a/src/libfetchers/fs-input-accessor.hh b/src/libfetchers/fs-input-accessor.hh index 19a5211c8..ba5af5887 100644 --- a/src/libfetchers/fs-input-accessor.hh +++ b/src/libfetchers/fs-input-accessor.hh @@ -7,26 +7,12 @@ namespace nix { class StorePath; class Store; -struct FSInputAccessor : InputAccessor -{ - virtual void checkAllowed(const CanonPath & absPath) = 0; +ref makeFSInputAccessor( + const CanonPath & root); - virtual void allowPath(CanonPath path) = 0; - - virtual bool hasAccessControl() = 0; -}; - -typedef std::function MakeNotAllowedError; - -ref makeFSInputAccessor( - const CanonPath & root, - std::optional> && allowedPaths = {}, - MakeNotAllowedError && makeNotAllowedError = {}); - -ref makeStorePathAccessor( +ref makeStorePathAccessor( ref store, - const StorePath & storePath, - MakeNotAllowedError && makeNotAllowedError = {}); + const StorePath & storePath); SourcePath getUnfilteredRootPath(CanonPath path); diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 8cd74057c..ff4b1e823 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -9,6 +9,7 @@ #include "processes.hh" #include "git.hh" #include "fs-input-accessor.hh" +#include "filtering-input-accessor.hh" #include "mounted-input-accessor.hh" #include "git-utils.hh" #include "logging.hh" @@ -639,7 +640,10 @@ struct GitInputScheme : InputScheme repoInfo.workdirInfo.files.insert(submodule.path); ref accessor = - makeFSInputAccessor(CanonPath(repoInfo.url), repoInfo.workdirInfo.files, makeNotAllowedError(repoInfo.url)); + AllowListInputAccessor::create( + makeFSInputAccessor(CanonPath(repoInfo.url)), + std::move(repoInfo.workdirInfo.files), + makeNotAllowedError(repoInfo.url)); /* If the repo has submodules, return a mounted input accessor consisting of the accessor for the top-level repo and the From 345f79d01676680f2d4ef8803790896a190c855b Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 5 Dec 2023 15:14:28 +0100 Subject: [PATCH 05/10] Check that we can't follow symlinks outside of the allowed paths --- tests/functional/restricted.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/functional/restricted.sh b/tests/functional/restricted.sh index b8deceacc..cb83c34b1 100644 --- a/tests/functional/restricted.sh +++ b/tests/functional/restricted.sh @@ -39,6 +39,15 @@ nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I $TEST_ROOT - [[ $(nix eval --raw --impure --restrict-eval -I . --expr 'builtins.readFile "${import ./simple.nix}/hello"') == 'Hello World!' ]] +# Check that we can't follow a symlink outside of the allowed paths. +mkdir -p $TEST_ROOT/tunnel.d +ln -sfn .. $TEST_ROOT/tunnel.d/tunnel +echo foo > $TEST_ROOT/bar + +expectStderr 1 nix-instantiate --restrict-eval --eval -E "let __nixPath = [ { prefix = \"foo\"; path = $TEST_ROOT/tunnel.d; } ]; in builtins.readFile " -I $TEST_ROOT/tunnel.d | grepQuiet "forbidden in restricted mode" + +expectStderr 1 nix-instantiate --restrict-eval --eval -E "let __nixPath = [ { prefix = \"foo\"; path = $TEST_ROOT/tunnel.d; } ]; in builtins.readDir " -I $TEST_ROOT/tunnel.d | grepQuiet "forbidden in restricted mode" + # Check whether we can leak symlink information through directory traversal. traverseDir="$(pwd)/restricted-traverse-me" ln -sfn "$(pwd)/restricted-secret" "$(pwd)/restricted-innocent" From 83c067c0fa0cc5a2dca440e5c986afe40b163802 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 5 Dec 2023 23:02:59 +0100 Subject: [PATCH 06/10] PosixSourceAccessor: Don't follow any symlinks All path components must not be symlinks now (so the user needs to call `resolveSymlinks()` when needed). --- src/libexpr/parser.y | 11 +++++----- src/libexpr/primops.cc | 30 ++++++++++++++-------------- src/libutil/posix-source-accessor.cc | 27 +++++++++++++++++++++---- src/libutil/posix-source-accessor.hh | 5 +++++ src/nix-env/nix-env.cc | 6 +++--- src/nix-env/user-env.cc | 2 +- tests/functional/restricted.sh | 7 +++++-- 7 files changed, 58 insertions(+), 30 deletions(-) diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 58fc580fc..16ad8af2e 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -692,16 +692,17 @@ SourcePath resolveExprPath(SourcePath path) /* If `path' is a symlink, follow it. This is so that relative path references work. */ - while (true) { + while (!path.path.isRoot()) { // Basic cycle/depth limit to avoid infinite loops. if (++followCount >= maxFollow) throw Error("too many symbolic links encountered while traversing the path '%s'", path); - if (path.lstat().type != InputAccessor::tSymlink) break; - path = {path.accessor, CanonPath(path.readLink(), path.path.parent().value_or(CanonPath::root))}; + auto p = path.parent().resolveSymlinks() + path.baseName(); + if (p.lstat().type != InputAccessor::tSymlink) break; + path = {path.accessor, CanonPath(p.readLink(), path.path.parent().value_or(CanonPath::root))}; } /* If `path' refers to a directory, append `/default.nix'. */ - if (path.lstat().type == InputAccessor::tDirectory) + if (path.resolveSymlinks().lstat().type == InputAccessor::tDirectory) return path + "default.nix"; return path; @@ -716,7 +717,7 @@ Expr * EvalState::parseExprFromFile(const SourcePath & path) Expr * EvalState::parseExprFromFile(const SourcePath & path, std::shared_ptr & staticEnv) { - auto buffer = path.readFile(); + auto buffer = path.resolveSymlinks().readFile(); // readFile hopefully have left some extra space for terminators buffer.append("\0\0", 2); return parse(buffer.data(), buffer.size(), Pos::Origin(path), path.parent(), staticEnv); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index c442de986..f2d51f8f5 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -110,7 +110,7 @@ StringMap EvalState::realiseContext(const NixStringContext & context) return res; } -static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v) +static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, bool resolveSymlinks = true) { NixStringContext context; @@ -120,9 +120,9 @@ static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v) if (!context.empty() && path.accessor == state.rootFS) { auto rewrites = state.realiseContext(context); auto realPath = state.toRealPath(rewriteStrings(path.path.abs(), rewrites), context); - return {path.accessor, CanonPath(realPath)}; - } else - return path; + path = {path.accessor, CanonPath(realPath)}; + } + return resolveSymlinks ? path.resolveSymlinks() : path; } catch (Error & e) { e.addTrace(state.positions[pos], "while realising the context of path '%s'", path); throw; @@ -162,7 +162,7 @@ static void mkOutputString( argument. */ static void import(EvalState & state, const PosIdx pos, Value & vPath, Value * vScope, Value & v) { - auto path = realisePath(state, pos, vPath); + auto path = realisePath(state, pos, vPath, false); auto path2 = path.path.abs(); // FIXME @@ -1525,16 +1525,16 @@ static RegisterPrimOp primop_storePath({ static void prim_pathExists(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - auto & arg = *args[0]; - - auto path = realisePath(state, pos, arg); - - /* SourcePath doesn't know about trailing slash. */ - auto mustBeDir = arg.type() == nString - && (arg.string_view().ends_with("/") - || arg.string_view().ends_with("/.")); - try { + auto & arg = *args[0]; + + auto path = realisePath(state, pos, arg); + + /* SourcePath doesn't know about trailing slash. */ + auto mustBeDir = arg.type() == nString + && (arg.string_view().ends_with("/") + || arg.string_view().ends_with("/.")); + auto st = path.maybeLstat(); auto exists = st && (!mustBeDir || st->type == SourceAccessor::tDirectory); v.mkBool(exists); @@ -1771,7 +1771,7 @@ static std::string_view fileTypeToString(InputAccessor::Type type) static void prim_readFileType(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - auto path = realisePath(state, pos, *args[0]); + auto path = realisePath(state, pos, *args[0], false); /* Retrieve the directory entry type and stringize it. */ v.mkString(fileTypeToString(path.lstat().type)); } diff --git a/src/libutil/posix-source-accessor.cc b/src/libutil/posix-source-accessor.cc index dc96f84e5..0601e6387 100644 --- a/src/libutil/posix-source-accessor.cc +++ b/src/libutil/posix-source-accessor.cc @@ -8,9 +8,9 @@ void PosixSourceAccessor::readFile( Sink & sink, std::function sizeCallback) { - // FIXME: add O_NOFOLLOW since symlinks should be resolved by the - // caller? - AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC); + assertNoSymlinks(path); + + AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC | O_NOFOLLOW); if (!fd) throw SysError("opening file '%1%'", path); @@ -42,14 +42,16 @@ void PosixSourceAccessor::readFile( bool PosixSourceAccessor::pathExists(const CanonPath & path) { + if (auto parent = path.parent()) assertNoSymlinks(*parent); return nix::pathExists(path.abs()); } std::optional PosixSourceAccessor::maybeLstat(const CanonPath & path) { + if (auto parent = path.parent()) assertNoSymlinks(*parent); struct stat st; if (::lstat(path.c_str(), &st)) { - if (errno == ENOENT) return std::nullopt; + if (errno == ENOENT || errno == ENOTDIR) return std::nullopt; throw SysError("getting status of '%s'", showPath(path)); } mtime = std::max(mtime, st.st_mtime); @@ -66,6 +68,7 @@ std::optional PosixSourceAccessor::maybeLstat(const CanonP SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath & path) { + assertNoSymlinks(path); DirEntries res; for (auto & entry : nix::readDirectory(path.abs())) { std::optional type; @@ -81,6 +84,7 @@ SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath & std::string PosixSourceAccessor::readLink(const CanonPath & path) { + if (auto parent = path.parent()) assertNoSymlinks(*parent); return nix::readLink(path.abs()); } @@ -89,4 +93,19 @@ std::optional PosixSourceAccessor::getPhysicalPath(const CanonPath & return path; } +void PosixSourceAccessor::assertNoSymlinks(CanonPath path) +{ + // FIXME: cache this since it potentially causes a lot of lstat calls. + while (!path.isRoot()) { + struct stat st; + if (::lstat(path.c_str(), &st)) { + if (errno != ENOENT) + throw SysError("getting status of '%s'", showPath(path)); + } + if (S_ISLNK(st.st_mode)) + throw Error("path '%s' is a symlink", showPath(path)); + path.pop(); + } +} + } diff --git a/src/libutil/posix-source-accessor.hh b/src/libutil/posix-source-accessor.hh index a45d96bf8..7189a40e5 100644 --- a/src/libutil/posix-source-accessor.hh +++ b/src/libutil/posix-source-accessor.hh @@ -29,6 +29,11 @@ struct PosixSourceAccessor : virtual SourceAccessor std::string readLink(const CanonPath & path) override; std::optional getPhysicalPath(const CanonPath & path) override; + + /** + * Throw an error if `path` or any of its ancestors are symlinks. + */ + void assertNoSymlinks(CanonPath path); }; } diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index 86126c7ad..e2bbd9775 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -97,7 +97,7 @@ static bool isNixExpr(const SourcePath & path, struct InputAccessor::Stat & st) { return st.type == InputAccessor::tRegular - || (st.type == InputAccessor::tDirectory && (path + "default.nix").pathExists()); + || (st.type == InputAccessor::tDirectory && (path + "default.nix").resolveSymlinks().pathExists()); } @@ -116,11 +116,11 @@ static void getAllExprs(EvalState & state, are implemented using profiles). */ if (i == "manifest.nix") continue; - SourcePath path2 = path + i; + auto path2 = (path + i).resolveSymlinks(); InputAccessor::Stat st; try { - st = path2.resolveSymlinks().lstat(); + st = path2.lstat(); } catch (Error &) { continue; // ignore dangling symlinks in ~/.nix-defexpr } diff --git a/src/nix-env/user-env.cc b/src/nix-env/user-env.cc index 250224e7d..34f6bd005 100644 --- a/src/nix-env/user-env.cc +++ b/src/nix-env/user-env.cc @@ -21,7 +21,7 @@ DrvInfos queryInstalled(EvalState & state, const Path & userEnv) auto manifestFile = userEnv + "/manifest.nix"; if (pathExists(manifestFile)) { Value v; - state.evalFile(state.rootPath(CanonPath(manifestFile)), v); + state.evalFile(state.rootPath(CanonPath(manifestFile)).resolveSymlinks(), v); Bindings & bindings(*state.allocBindings(0)); getDerivations(state, v, "", bindings, elems, false); } diff --git a/tests/functional/restricted.sh b/tests/functional/restricted.sh index cb83c34b1..2d6ab964b 100644 --- a/tests/functional/restricted.sh +++ b/tests/functional/restricted.sh @@ -40,13 +40,16 @@ nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I $TEST_ROOT - [[ $(nix eval --raw --impure --restrict-eval -I . --expr 'builtins.readFile "${import ./simple.nix}/hello"') == 'Hello World!' ]] # Check that we can't follow a symlink outside of the allowed paths. -mkdir -p $TEST_ROOT/tunnel.d +mkdir -p $TEST_ROOT/tunnel.d $TEST_ROOT/foo2 ln -sfn .. $TEST_ROOT/tunnel.d/tunnel echo foo > $TEST_ROOT/bar expectStderr 1 nix-instantiate --restrict-eval --eval -E "let __nixPath = [ { prefix = \"foo\"; path = $TEST_ROOT/tunnel.d; } ]; in builtins.readFile " -I $TEST_ROOT/tunnel.d | grepQuiet "forbidden in restricted mode" -expectStderr 1 nix-instantiate --restrict-eval --eval -E "let __nixPath = [ { prefix = \"foo\"; path = $TEST_ROOT/tunnel.d; } ]; in builtins.readDir " -I $TEST_ROOT/tunnel.d | grepQuiet "forbidden in restricted mode" +expectStderr 1 nix-instantiate --restrict-eval --eval -E "let __nixPath = [ { prefix = \"foo\"; path = $TEST_ROOT/tunnel.d; } ]; in builtins.readDir " -I $TEST_ROOT/tunnel.d | grepQuiet "forbidden in restricted mode" + +# Reading the parents of allowed paths should show only the ancestors of the allowed paths. +[[ $(nix-instantiate --restrict-eval --eval -E "let __nixPath = [ { prefix = \"foo\"; path = $TEST_ROOT/tunnel.d; } ]; in builtins.readDir " -I $TEST_ROOT/tunnel.d) == '{ "tunnel.d" = "directory"; }' ]] # Check whether we can leak symlink information through directory traversal. traverseDir="$(pwd)/restricted-traverse-me" From 504e4fc4576dc6a4cd5c083a3bf7b80dfb0ca220 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 6 Dec 2023 13:45:59 +0100 Subject: [PATCH 07/10] CanonPath: Support std::hash --- src/libfetchers/git-utils.cc | 2 +- src/libutil/canon-path.hh | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index 19eae0e1d..5f2a7a8bc 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -554,7 +554,7 @@ struct GitInputAccessor : InputAccessor return toHash(*git_tree_entry_id(entry)); } - std::map lookupCache; + std::unordered_map lookupCache; /* Recursively look up 'path' relative to the root. */ git_tree_entry * lookup(const CanonPath & path) diff --git a/src/libutil/canon-path.hh b/src/libutil/canon-path.hh index 6d0519f4f..6aff4ec0d 100644 --- a/src/libutil/canon-path.hh +++ b/src/libutil/canon-path.hh @@ -205,8 +205,19 @@ public: * `CanonPath(this.makeRelative(x), this) == path`. */ std::string makeRelative(const CanonPath & path) const; + + friend class std::hash; }; std::ostream & operator << (std::ostream & stream, const CanonPath & path); } + +template<> +struct std::hash +{ + std::size_t operator ()(const nix::CanonPath & s) const noexcept + { + return std::hash{}(s.path); + } +}; From 57246c4c3802920e6167fd540dae2a0abca97f15 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 6 Dec 2023 13:55:07 +0100 Subject: [PATCH 08/10] PosixSourceAccessor: Cache lstat() calls Since we're doing a lot of them in assertNoSymlinks(). --- src/libutil/posix-source-accessor.cc | 56 +++++++++++++++++++--------- src/libutil/posix-source-accessor.hh | 4 ++ 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/src/libutil/posix-source-accessor.cc b/src/libutil/posix-source-accessor.cc index 0601e6387..15ff76e59 100644 --- a/src/libutil/posix-source-accessor.cc +++ b/src/libutil/posix-source-accessor.cc @@ -1,5 +1,8 @@ #include "posix-source-accessor.hh" #include "signals.hh" +#include "sync.hh" + +#include namespace nix { @@ -46,23 +49,45 @@ bool PosixSourceAccessor::pathExists(const CanonPath & path) return nix::pathExists(path.abs()); } +std::optional PosixSourceAccessor::cachedLstat(const CanonPath & path) +{ + static Sync>> _cache; + + { + auto cache(_cache.lock()); + auto i = cache->find(path); + if (i != cache->end()) return i->second; + } + + std::optional st{std::in_place}; + if (::lstat(path.c_str(), &*st)) { + if (errno == ENOENT || errno == ENOTDIR) + st.reset(); + else + throw SysError("getting status of '%s'", showPath(path)); + } + + auto cache(_cache.lock()); + if (cache->size() >= 16384) cache->clear(); + cache->emplace(path, st); + + return st; +} + std::optional PosixSourceAccessor::maybeLstat(const CanonPath & path) { if (auto parent = path.parent()) assertNoSymlinks(*parent); - struct stat st; - if (::lstat(path.c_str(), &st)) { - if (errno == ENOENT || errno == ENOTDIR) return std::nullopt; - throw SysError("getting status of '%s'", showPath(path)); - } - mtime = std::max(mtime, st.st_mtime); + auto st = cachedLstat(path); + if (!st) return std::nullopt; + mtime = std::max(mtime, st->st_mtime); return Stat { .type = - S_ISREG(st.st_mode) ? tRegular : - S_ISDIR(st.st_mode) ? tDirectory : - S_ISLNK(st.st_mode) ? tSymlink : + S_ISREG(st->st_mode) ? tRegular : + S_ISDIR(st->st_mode) ? tDirectory : + S_ISLNK(st->st_mode) ? tSymlink : tMisc, - .fileSize = S_ISREG(st.st_mode) ? std::optional(st.st_size) : std::nullopt, - .isExecutable = S_ISREG(st.st_mode) && st.st_mode & S_IXUSR, + .fileSize = S_ISREG(st->st_mode) ? std::optional(st->st_size) : std::nullopt, + .isExecutable = S_ISREG(st->st_mode) && st->st_mode & S_IXUSR, }; } @@ -95,14 +120,9 @@ std::optional PosixSourceAccessor::getPhysicalPath(const CanonPath & void PosixSourceAccessor::assertNoSymlinks(CanonPath path) { - // FIXME: cache this since it potentially causes a lot of lstat calls. while (!path.isRoot()) { - struct stat st; - if (::lstat(path.c_str(), &st)) { - if (errno != ENOENT) - throw SysError("getting status of '%s'", showPath(path)); - } - if (S_ISLNK(st.st_mode)) + auto st = cachedLstat(path); + if (st && S_ISLNK(st->st_mode)) throw Error("path '%s' is a symlink", showPath(path)); path.pop(); } diff --git a/src/libutil/posix-source-accessor.hh b/src/libutil/posix-source-accessor.hh index 7189a40e5..b2bd39805 100644 --- a/src/libutil/posix-source-accessor.hh +++ b/src/libutil/posix-source-accessor.hh @@ -30,10 +30,14 @@ struct PosixSourceAccessor : virtual SourceAccessor std::optional getPhysicalPath(const CanonPath & path) override; +private: + /** * Throw an error if `path` or any of its ancestors are symlinks. */ void assertNoSymlinks(CanonPath path); + + std::optional cachedLstat(const CanonPath & path); }; } From 53ab5d87c2eef72202bd76eb43e072636bbc72e8 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 6 Dec 2023 14:05:32 +0100 Subject: [PATCH 09/10] Use expectStderr --- tests/functional/restricted.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/functional/restricted.sh b/tests/functional/restricted.sh index 2d6ab964b..3de26eb36 100644 --- a/tests/functional/restricted.sh +++ b/tests/functional/restricted.sh @@ -14,7 +14,7 @@ nix-instantiate --restrict-eval --eval -E 'builtins.readFile ./simple.nix' -I sr (! nix-instantiate --restrict-eval --eval -E 'builtins.readDir ../../src/nix-channel') nix-instantiate --restrict-eval --eval -E 'builtins.readDir ../../src/nix-channel' -I src=../../src -(! nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in builtins.readFile ') +expectStderr 1 nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in builtins.readFile ' | grepQuiet "forbidden in restricted mode" nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in builtins.readFile ' -I src=. p=$(nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval --allowed-uris "file://$(pwd)") From 2bd83225004012af97d2d5977dc1de952f60aa8d Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 6 Dec 2023 14:08:40 +0100 Subject: [PATCH 10/10] Update src/libfetchers/filtering-input-accessor.hh Co-authored-by: Robert Hensing --- src/libfetchers/filtering-input-accessor.hh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libfetchers/filtering-input-accessor.hh b/src/libfetchers/filtering-input-accessor.hh index 209d26974..e1b83c929 100644 --- a/src/libfetchers/filtering-input-accessor.hh +++ b/src/libfetchers/filtering-input-accessor.hh @@ -13,8 +13,8 @@ typedef std::function MakeNotAllowe /** * An abstract wrapping `InputAccessor` that performs access - * control. Subclasses should override `checkAccess()` to implement an - * access control policy. + * control. Subclasses should override `isAllowed()` to implement an + * access control policy. The error message is customized at construction. */ struct FilteringInputAccessor : InputAccessor {