Merge pull request #9497 from edolstra/move-access-control

Move restricted/pure-eval access control out of the evaluator and into the accessor

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(
+            auto e =
-                resolveExprPath(state->checkSourcePath(lookupFileArg(*state, *file)))
+                state->parseExprFromFile(
-                );
+                    resolveExprPath(
+                        lookupFileArg(*state, *file)));
 
             Value root;
             state->eval(e, root);

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"
@@ -509,7 +510,16 @@ EvalState::EvalState(
     , sOutputSpecified(symbols.create("outputSpecified"))
     , repair(NoRepair)
     , emptyBindings(0)
-    , rootFS(makeFSInputAccessor(CanonPath::root))
+    , rootFS(
+        evalSettings.restrictEval || evalSettings.pureEval
+        ? ref<InputAccessor>(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(
@@ -551,28 +561,10 @@ EvalState::EvalState(
             searchPath.elements.emplace_back(SearchPath::Elem::parse(i));
     }
 
-    if (evalSettings.restrictEval || evalSettings.pureEval) {
+    /* Allow access to all paths in the search path. */
-        allowedPaths = PathSet();
+    if (rootFS.dynamic_pointer_cast<AllowListInputAccessor>())
-
+        for (auto & i : searchPath.elements)
-        for (auto & i : searchPath.elements) {
+            resolveSearchPathPath(i.path, true);
-            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);
-        }
-    }
 
     corepkgsFS->addFile(
         CanonPath("fetchurl.nix"),
@@ -590,14 +582,14 @@ EvalState::~EvalState()
 
 void EvalState::allowPath(const Path & path)
 {
-    if (allowedPaths)
+    if (auto rootFS2 = rootFS.dynamic_pointer_cast<AllowListInputAccessor>())
-        allowedPaths->insert(path);
+        rootFS2->allowPath(CanonPath(path));
 }
 
 void EvalState::allowPath(const StorePath & storePath)
 {
-    if (allowedPaths)
+    if (auto rootFS2 = rootFS.dynamic_pointer_cast<AllowListInputAccessor>())
-        allowedPaths->insert(store->toRealPath(storePath));
+        rootFS2->allowPath(CanonPath(store->toRealPath(storePath)));
 }
 
 void EvalState::allowAndSetStorePathString(const StorePath & storePath, Value & v)
@@ -607,54 +599,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<InputAccessor>(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 +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, "/")) {
-        checkSourcePath(rootPath(CanonPath(uri)));
+        if (auto rootFS2 = rootFS.dynamic_pointer_cast<AllowListInputAccessor>())
+            rootFS2->checkAccess(CanonPath(uri));
         return;
     }
 
     if (hasPrefix(uri, "file://")) {
-        checkSourcePath(rootPath(CanonPath(std::string(uri, 7))));
+        if (auto rootFS2 = rootFS.dynamic_pointer_cast<AllowListInputAccessor>())
+            rootFS2->checkAccess(CanonPath(uri.substr(7)));
         return;
     }
 
@@ -1181,10 +1127,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 +1149,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;
 

src/libexpr/eval.hh

@@ -30,7 +30,6 @@ class EvalState;
 class StorePath;
 struct SingleDerivedPath;
 enum RepairFlag : bool;
-struct FSInputAccessor;
 struct MemoryInputAccessor;
 
 
@@ -217,18 +216,12 @@ public:
      */
     RepairFlag repair;
 
-    /**
-     * The allowed filesystem paths in restricted or pure evaluation
-     * mode.
-     */
-    std::optional<PathSet> allowedPaths;
-
     Bindings emptyBindings;
 
     /**
      * The accessor for the root filesystem.
      */
-    const ref<FSInputAccessor> rootFS;
+    const ref<InputAccessor> rootFS;
 
     /**
      * The in-memory filesystem for <nix/...> paths.
@@ -396,12 +389,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 +432,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<std::string> resolveSearchPathPath(const SearchPath::Path & path);
+    std::optional<std::string> resolveSearchPathPath(
+        const SearchPath::Path & elem,
+        bool initAccessControl = false);
 
     /**
      * Evaluate an expression to normal form
@@ -756,6 +745,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:
 
     /**

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;
+        auto p = path.parent().resolveSymlinks() + path.baseName();
-        path = {path.accessor, CanonPath(path.readLink(), path.path.parent().value_or(CanonPath::root))};
+        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> & 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);
@@ -783,7 +784,7 @@ SourcePath EvalState::findFile(const SearchPath & searchPath, const std::string_
 }
 
 
-std::optional<std::string> EvalState::resolveSearchPathPath(const SearchPath::Path & value0)
+std::optional<std::string> EvalState::resolveSearchPathPath(const SearchPath::Path & value0, bool initAccessControl)
 {
     auto & value = value0.s;
     auto i = searchPathResolved.find(value);
@@ -800,7 +801,6 @@ std::optional<std::string> EvalState::resolveSearchPathPath(const SearchPath::Pa
             logWarning({
                 .msg = hintfmt("Nix search path entry '%1%' cannot be downloaded, ignoring", value)
             });
-            res = std::nullopt;
         }
     }
 
@@ -814,6 +814,20 @@ std::optional<std::string> 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 +843,7 @@ std::optional<std::string> EvalState::resolveSearchPathPath(const SearchPath::Pa
     else
         debug("failed to resolve search path element '%s'", value);
 
-    searchPathResolved[value] = res;
+    searchPathResolved.emplace(value, res);
     return res;
 }
 

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 <boost/container/small_vector.hpp>
 #include <nlohmann/json.hpp>
@@ -90,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 (allowedPaths) {
+            allowPath(store->toRealPath(outputPath));
-                allowPath(outputPath);
+
-            }
             /* Get all the output paths corresponding to the placeholders we had */
             if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations)) {
                 res.insert_or_assign(
@@ -110,27 +110,19 @@ StringMap EvalState::realiseContext(const NixStringContext & context)
     return res;
 }
 
-struct RealisePathFlags {
+static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, bool resolveSymlinks = true)
-    // 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 = {})
 {
     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)};
+            path = {path.accessor, CanonPath(realPath)};
         }
-
+        return resolveSymlinks ? path.resolveSymlinks() : path;
-        return flags.checkForPureEval
-            ? state.checkSourcePath(path)
-            : path;
     } catch (Error & e) {
         e.addTrace(state.positions[pos], "while realising the context of path '%s'", path);
         throw;
@@ -170,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
@@ -1493,7 +1485,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. */
@@ -1533,29 +1525,19 @@ static RegisterPrimOp primop_storePath({
 
 static void prim_pathExists(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
+    try {
         auto & arg = *args[0];
 
-    /* We don’t check the path right now, because we don’t want to
+        auto path = realisePath(state, pos, arg);
-       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 });
 
         /* SourcePath doesn't know about trailing slash. */
         auto mustBeDir = arg.type() == nString
             && (arg.string_view().ends_with("/")
                 || arg.string_view().ends_with("/."));
 
-    try {
+        auto st = path.maybeLstat();
-        auto checked = state.checkSourcePath(path);
-        auto st = checked.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 +1681,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 {
@@ -1789,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));
 }
@@ -2178,11 +2160,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<Hash> expectedHash,
@@ -2190,48 +2196,29 @@ static void addPath(
     const NixStringContext & context)
 {
     try {
+        StorePathSet refs;
+
+        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.toRealPath(rewriteStrings(path, rewrites), context);
+            path = {state.rootFS, CanonPath(state.toRealPath(rewriteStrings(path.path.abs(), rewrites), context))};
 
-        StorePathSet refs;
-
-        if (state.store->isInStore(path)) {
             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
+        std::unique_ptr<PathFilter> filter;
-            ? path
+        if (filterFun)
-            : state.checkSourcePath(state.rootPath(CanonPath(path))).path.abs();
+            filter = std::make_unique<PathFilter>([&](const Path & p) {
-
+                auto p2 = CanonPath(p);
-        PathFilter filter = filterFun ? ([&](const Path & path) {
+                return state.callPathFilter(filterFun, {path.accessor, p2}, p2.abs(), pos);
-            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::optional<StorePath> expectedStorePath;
         if (expectedHash)
@@ -2242,7 +2229,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 +2248,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 +2344,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({

src/libexpr/value.hh

@@ -423,10 +423,9 @@ public:
     SourcePath path() const
     {
         assert(internalType == tPath);
-        return SourcePath {
+        return SourcePath(
-            .accessor = ref(_path.accessor->shared_from_this()),
+            ref(_path.accessor->shared_from_this()),
-            .path = CanonPath(CanonPath::unchecked_t(), _path.path)
+            CanonPath(CanonPath::unchecked_t(), _path.path));
-        };
     }
 
     std::string_view string_view() const

src/libfetchers/fetchers.cc

@@ -374,7 +374,7 @@ void InputScheme::clone(const Input & input, const Path & destDir) const
 std::pair<StorePath, Input> InputScheme::fetch(ref<Store> 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};
 }
 

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<InputAccessor::Stat> 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<CanonPath> allowedPaths;
+
+    AllowListInputAccessorImpl(
+        ref<InputAccessor> next,
+        std::set<CanonPath> && 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> AllowListInputAccessor::create(
+    ref<InputAccessor> next,
+    std::set<CanonPath> && allowedPaths,
+    MakeNotAllowedError && makeNotAllowedError)
+{
+    return make_ref<AllowListInputAccessorImpl>(next, std::move(allowedPaths), std::move(makeNotAllowedError));
+}
+
+}

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<RestrictedPathError(const CanonPath & path)> MakeNotAllowedError;
+
+/**
+ * An abstract wrapping `InputAccessor` that performs access
+ * control. Subclasses should override `isAllowed()` to implement an
+ * access control policy. The error message is customized at construction.
+ */
+struct FilteringInputAccessor : InputAccessor
+{
+    ref<InputAccessor> 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<Stat> 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<AllowListInputAccessor> create(
+        ref<InputAccessor> next,
+        std::set<CanonPath> && allowedPaths,
+        MakeNotAllowedError && makeNotAllowedError);
+
+    using FilteringInputAccessor::FilteringInputAccessor;
+};
+
+}

src/libfetchers/fs-input-accessor.cc

@@ -4,19 +4,12 @@
 
 namespace nix {
 
-struct FSInputAccessorImpl : FSInputAccessor, PosixSourceAccessor
+struct FSInputAccessor : InputAccessor, PosixSourceAccessor
 {
     CanonPath root;
-    std::optional<std::set<CanonPath>> allowedPaths;
-    MakeNotAllowedError makeNotAllowedError;
 
-    FSInputAccessorImpl(
+    FSInputAccessor(const CanonPath & root)
-        const CanonPath & root,
-        std::optional<std::set<CanonPath>> && allowedPaths,
-        MakeNotAllowedError && makeNotAllowedError)
         : 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<void(uint64_t)> sizeCallback) override
     {
         auto absPath = makeAbsPath(path);
-        checkAllowed(absPath);
         PosixSourceAccessor::readFile(absPath, sink, sizeCallback);
     }
 
     bool pathExists(const CanonPath & path) override
     {
-        auto absPath = makeAbsPath(path);
+        return PosixSourceAccessor::pathExists(makeAbsPath(path));
-        return isAllowed(absPath) && PosixSourceAccessor::pathExists(absPath);
     }
 
     std::optional<Stat> maybeLstat(const CanonPath & path) override
     {
-        auto absPath = makeAbsPath(path);
+        return PosixSourceAccessor::maybeLstat(makeAbsPath(path));
-        checkAllowed(absPath);
-        return PosixSourceAccessor::maybeLstat(absPath);
     }
 
     DirEntries readDirectory(const CanonPath & path) override
     {
-        auto absPath = makeAbsPath(path);
-        checkAllowed(absPath);
         DirEntries res;
-        for (auto & entry : PosixSourceAccessor::readDirectory(absPath))
+        for (auto & entry : PosixSourceAccessor::readDirectory(makeAbsPath(path)))
-            if (isAllowed(absPath + entry.first))
             res.emplace(entry);
         return res;
     }
 
     std::string readLink(const CanonPath & path) override
     {
-        auto absPath = makeAbsPath(path);
+        return PosixSourceAccessor::readLink(makeAbsPath(path));
-        checkAllowed(absPath);
-        return PosixSourceAccessor::readLink(absPath);
     }
 
     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<CanonPath> getPhysicalPath(const CanonPath & path) override
     {
         return makeAbsPath(path);
     }
 };
 
-ref<FSInputAccessor> makeFSInputAccessor(
+ref<InputAccessor> makeFSInputAccessor(const CanonPath & root)
-    const CanonPath & root,
-    std::optional<std::set<CanonPath>> && allowedPaths,
-    MakeNotAllowedError && makeNotAllowedError)
 {
-    return make_ref<FSInputAccessorImpl>(root, std::move(allowedPaths), std::move(makeNotAllowedError));
+    return make_ref<FSInputAccessor>(root);
 }
 
-ref<FSInputAccessor> makeStorePathAccessor(
+ref<InputAccessor> makeStorePathAccessor(
     ref<Store> store,
-    const StorePath & storePath,
+    const StorePath & storePath)
-    MakeNotAllowedError && makeNotAllowedError)
 {
-    return makeFSInputAccessor(CanonPath(store->toRealPath(storePath)), {}, std::move(makeNotAllowedError));
+    return makeFSInputAccessor(CanonPath(store->toRealPath(storePath)));
 }
 
 SourcePath getUnfilteredRootPath(CanonPath path)

src/libfetchers/fs-input-accessor.hh

@@ -7,26 +7,12 @@ namespace nix {
 class StorePath;
 class Store;
 
-struct FSInputAccessor : InputAccessor
+ref<InputAccessor> makeFSInputAccessor(
-{
+    const CanonPath & root);
-    virtual void checkAllowed(const CanonPath & absPath) = 0;
 
-    virtual void allowPath(CanonPath path) = 0;
+ref<InputAccessor> makeStorePathAccessor(
-
-    virtual bool hasAccessControl() = 0;
-};
-
-typedef std::function<RestrictedPathError(const CanonPath & path)> MakeNotAllowedError;
-
-ref<FSInputAccessor> makeFSInputAccessor(
-    const CanonPath & root,
-    std::optional<std::set<CanonPath>> && allowedPaths = {},
-    MakeNotAllowedError && makeNotAllowedError = {});
-
-ref<FSInputAccessor> makeStorePathAccessor(
     ref<Store> store,
-    const StorePath & storePath,
+    const StorePath & storePath);
-    MakeNotAllowedError && makeNotAllowedError = {});
 
 SourcePath getUnfilteredRootPath(CanonPath path);
 

src/libfetchers/git-utils.cc

@@ -554,7 +554,7 @@ struct GitInputAccessor : InputAccessor
         return toHash(*git_tree_entry_id(entry));
     }
 
-    std::map<CanonPath, TreeEntry> lookupCache;
+    std::unordered_map<CanonPath, TreeEntry> lookupCache;
 
     /* Recursively look up 'path' relative to the root. */
     git_tree_entry * lookup(const CanonPath & path)

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<InputAccessor> 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

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;
 

src/libfetchers/input-accessor.hh

@@ -36,8 +36,6 @@ struct InputAccessor : virtual SourceAccessor, std::enable_shared_from_this<Inpu
         FileIngestionMethod method = FileIngestionMethod::Recursive,
         PathFilter * filter = nullptr,
         RepairFlag repair = NoRepair);
-
-    SourcePath root();
 };
 
 /**
@@ -51,6 +49,11 @@ struct SourcePath
     ref<InputAccessor> accessor;
     CanonPath path;
 
+    SourcePath(ref<InputAccessor> accessor, CanonPath path = CanonPath::root)
+        : accessor(std::move(accessor))
+        , path(std::move(path))
+    { }
+
     std::string_view baseName() const;
 
     /**

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<CanonPath>;
 };
 
 std::ostream & operator << (std::ostream & stream, const CanonPath & path);
 
 }
+
+template<>
+struct std::hash<nix::CanonPath>
+{
+    std::size_t operator ()(const nix::CanonPath & s) const noexcept
+    {
+        return std::hash<std::string>{}(s.path);
+    }
+};

src/libutil/posix-source-accessor.cc

@@ -1,5 +1,8 @@
 #include "posix-source-accessor.hh"
 #include "signals.hh"
+#include "sync.hh"
+
+#include <unordered_map>
 
 namespace nix {
 
@@ -8,9 +11,9 @@ void PosixSourceAccessor::readFile(
     Sink & sink,
     std::function<void(uint64_t)> sizeCallback)
 {
-    // FIXME: add O_NOFOLLOW since symlinks should be resolved by the
+    assertNoSymlinks(path);
-    // caller?
+
-    AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC);
+    AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC | O_NOFOLLOW);
     if (!fd)
         throw SysError("opening file '%1%'", path);
 
@@ -42,30 +45,55 @@ void PosixSourceAccessor::readFile(
 
 bool PosixSourceAccessor::pathExists(const CanonPath & path)
 {
+    if (auto parent = path.parent()) assertNoSymlinks(*parent);
     return nix::pathExists(path.abs());
 }
 
+std::optional<struct stat> PosixSourceAccessor::cachedLstat(const CanonPath & path)
+{
+    static Sync<std::unordered_map<CanonPath, std::optional<struct stat>>> _cache;
+
+    {
+        auto cache(_cache.lock());
+        auto i = cache->find(path);
+        if (i != cache->end()) return i->second;
+    }
+
+    std::optional<struct stat> 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<SourceAccessor::Stat> PosixSourceAccessor::maybeLstat(const CanonPath & path)
 {
-    struct stat st;
+    if (auto parent = path.parent()) assertNoSymlinks(*parent);
-    if (::lstat(path.c_str(), &st)) {
+    auto st = cachedLstat(path);
-        if (errno == ENOENT) return std::nullopt;
+    if (!st) return std::nullopt;
-        throw SysError("getting status of '%s'", showPath(path));
+    mtime = std::max(mtime, st->st_mtime);
-    }
-    mtime = std::max(mtime, st.st_mtime);
     return Stat {
         .type =
-            S_ISREG(st.st_mode) ? tRegular :
+            S_ISREG(st->st_mode) ? tRegular :
-            S_ISDIR(st.st_mode) ? tDirectory :
+            S_ISDIR(st->st_mode) ? tDirectory :
-            S_ISLNK(st.st_mode) ? tSymlink :
+            S_ISLNK(st->st_mode) ? tSymlink :
             tMisc,
-        .fileSize = S_ISREG(st.st_mode) ? std::optional<uint64_t>(st.st_size) : std::nullopt,
+        .fileSize = S_ISREG(st->st_mode) ? std::optional<uint64_t>(st->st_size) : std::nullopt,
-        .isExecutable = S_ISREG(st.st_mode) && st.st_mode & S_IXUSR,
+        .isExecutable = S_ISREG(st->st_mode) && st->st_mode & S_IXUSR,
     };
 }
 
 SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath & path)
 {
+    assertNoSymlinks(path);
     DirEntries res;
     for (auto & entry : nix::readDirectory(path.abs())) {
         std::optional<Type> type;
@@ -81,6 +109,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 +118,14 @@ std::optional<CanonPath> PosixSourceAccessor::getPhysicalPath(const CanonPath &
     return path;
 }
 
+void PosixSourceAccessor::assertNoSymlinks(CanonPath path)
+{
+    while (!path.isRoot()) {
+        auto st = cachedLstat(path);
+        if (st && S_ISLNK(st->st_mode))
+            throw Error("path '%s' is a symlink", showPath(path));
+        path.pop();
+    }
+}
+
 }

src/libutil/posix-source-accessor.hh

@@ -29,6 +29,15 @@ struct PosixSourceAccessor : virtual SourceAccessor
     std::string readLink(const CanonPath & path) override;
 
     std::optional<CanonPath> getPhysicalPath(const CanonPath & path) override;
+
+private:
+
+    /**
+     * Throw an error if `path` or any of its ancestors are symlinks.
+     */
+    void assertNoSymlinks(CanonPath path);
+
+    std::optional<struct stat> cachedLstat(const CanonPath & path);
 };
 
 }

src/nix-build/nix-build.cc

@@ -311,8 +311,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,
+                    exprs.push_back(
-                                        inShebang && !packages ? absPath(i, absPath(dirOf(script))) : i)))));
+                        state->parseExprFromFile(
+                            resolveExprPath(
+                                lookupFileArg(*state,
+                                    inShebang && !packages ? absPath(i, absPath(dirOf(script))) : i))));
             }
         }
 

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
         }

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);
     }

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);
         }

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 <foo>')
+expectStderr 1 nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in builtins.readFile <foo/simple.nix>' | grepQuiet "forbidden in restricted mode"
-nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in <foo>' -I src=.
+nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in builtins.readFile <foo/simple.nix>' -I src=.
 
 p=$(nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval --allowed-uris "file://$(pwd)")
 cmp $p restricted.sh
@@ -39,6 +39,18 @@ 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 $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 <foo/tunnel/bar>" -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 <foo/tunnel/foo2>" -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 <foo/tunnel>" -I $TEST_ROOT/tunnel.d) == '{ "tunnel.d" = "directory"; }' ]]
+
 # Check whether we can leak symlink information through directory traversal.
 traverseDir="$(pwd)/restricted-traverse-me"
 ln -sfn "$(pwd)/restricted-secret" "$(pwd)/restricted-innocent"