From 8cafc754d845529a78595d1196769257ee23ca56 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 30 Nov 2023 21:54:53 +0100 Subject: [PATCH] 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