Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

pathExists: Return false on "/nix/store" in pure mode #10505

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
12 changes: 7 additions & 5 deletions src/libexpr/eval.cc
Original file line number Diff line number Diff line change
Expand Up @@ -395,12 +395,14 @@ EvalState::EvalState(
, emptyBindings(0)
, rootFS(
evalSettings.restrictEval || evalSettings.pureEval
? ref<InputAccessor>(AllowListInputAccessor::create(makeFSInputAccessor(), {},
? ref<InputAccessor>(AllowListInputAccessor::create(makeFSInputAccessor(), {}, {},
[](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);
throw RestrictedPathError(
std::string("access to absolute path '%1%' is forbidden ") +
(evalSettings.pureEval
? "in pure evaluation mode (use '--impure' to override)"
: "in restricted mode"),
path);
}))
: makeFSInputAccessor())
, corepkgsFS(makeMemoryInputAccessor())
Expand Down
34 changes: 27 additions & 7 deletions src/libfetchers/filtering-input-accessor.cc
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "filtering-input-accessor.hh"

#include <unordered_set>

namespace nix {

std::string FilteringInputAccessor::readFile(const CanonPath & path)
Expand All @@ -13,10 +15,15 @@ bool FilteringInputAccessor::pathExists(const CanonPath & path)
return isAllowed(path) && next->pathExists(prefix / path);
}

std::optional<InputAccessor::Stat> FilteringInputAccessor::maybeLstat(const CanonPath & path)
SourceAccessor::Stat FilteringInputAccessor::lstat(const CanonPath & path)
{
checkAccess(path);
return next->maybeLstat(prefix / path);
return next->lstat(prefix / path);
}

std::optional<InputAccessor::Stat> FilteringInputAccessor::maybeLstat(const CanonPath & path)
{
return isAllowed(path) ? next->maybeLstat(prefix / path) : std::nullopt;
}

InputAccessor::DirEntries FilteringInputAccessor::readDirectory(const CanonPath & path)
Expand Down Expand Up @@ -51,33 +58,46 @@ void FilteringInputAccessor::checkAccess(const CanonPath & path)

struct AllowListInputAccessorImpl : AllowListInputAccessor
{
std::set<CanonPath> allowedPrefixes;
std::unordered_set<CanonPath> allowedPrefixes;
std::unordered_set<CanonPath> allowedPaths;

AllowListInputAccessorImpl(
ref<InputAccessor> next,
std::set<CanonPath> && allowedPrefixes,
std::unordered_set<CanonPath> && allowedPrefixes,
std::unordered_set<CanonPath> && allowedPaths,
MakeNotAllowedError && makeNotAllowedError)
: AllowListInputAccessor(SourcePath(next), std::move(makeNotAllowedError))
, allowedPrefixes(std::move(allowedPrefixes))
, allowedPaths(std::move(allowedPaths))
{ }

bool isAllowed(const CanonPath & path) override
{
return path.isAllowed(allowedPrefixes);
return allowedPaths.count(path) || path.isAllowed(allowedPrefixes);
}

void allowPrefix(CanonPath prefix) override
{
allowedPrefixes.insert(std::move(prefix));
}

void allowPath(CanonPath path) override
{
allowedPaths.insert(std::move(path));
}
};

ref<AllowListInputAccessor> AllowListInputAccessor::create(
ref<InputAccessor> next,
std::set<CanonPath> && allowedPrefixes,
std::unordered_set<CanonPath> && allowedPrefixes,
std::unordered_set<CanonPath> && allowedPaths,
MakeNotAllowedError && makeNotAllowedError)
{
return make_ref<AllowListInputAccessorImpl>(next, std::move(allowedPrefixes), std::move(makeNotAllowedError));
return make_ref<AllowListInputAccessorImpl>(
next,
std::move(allowedPrefixes),
std::move(allowedPaths),
std::move(makeNotAllowedError));
}

bool CachingFilteringInputAccessor::isAllowed(const CanonPath & path)
Expand Down
13 changes: 11 additions & 2 deletions src/libfetchers/filtering-input-accessor.hh
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ struct FilteringInputAccessor : InputAccessor

bool pathExists(const CanonPath & path) override;

Stat lstat(const CanonPath & path) override;

std::optional<Stat> maybeLstat(const CanonPath & path) override;

DirEntries readDirectory(const CanonPath & path) override;
Expand Down Expand Up @@ -60,13 +62,20 @@ struct FilteringInputAccessor : InputAccessor
struct AllowListInputAccessor : public FilteringInputAccessor
{
/**
* Grant access to the specified prefix.
* Grant access to the specified prefix, i.e. the path *and* its
* children.
*/
virtual void allowPrefix(CanonPath prefix) = 0;

/**
* Grant access to a path but not its children.
*/
virtual void allowPath(CanonPath path) = 0;

static ref<AllowListInputAccessor> create(
ref<InputAccessor> next,
std::set<CanonPath> && allowedPrefixes,
std::unordered_set<CanonPath> && allowedPrefixes,
std::unordered_set<CanonPath> && allowedPaths,
MakeNotAllowedError && makeNotAllowedError);

using FilteringInputAccessor::FilteringInputAccessor;
Expand Down
28 changes: 17 additions & 11 deletions src/libfetchers/git-utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -943,17 +943,23 @@ ref<InputAccessor> GitRepoImpl::getAccessor(const Hash & rev, bool exportIgnore)
ref<InputAccessor> GitRepoImpl::getAccessor(const WorkdirInfo & wd, bool exportIgnore, MakeNotAllowedError makeNotAllowedError)
{
auto self = ref<GitRepoImpl>(shared_from_this());
/* In case of an empty workdir, return an empty in-memory tree. We
cannot use AllowListInputAccessor because it would return an
error for the root (and we can't add the root to the allow-list
since that would allow access to all its children). */
ref<InputAccessor> fileAccessor =
wd.files.empty()
? makeEmptyInputAccessor()
: AllowListInputAccessor::create(
makeFSInputAccessor(path),
std::set<CanonPath> { wd.files },
std::move(makeNotAllowedError)).cast<InputAccessor>();

/* Grant access to the parent directories of every accessible
file. The root is always accessible. */
std::unordered_set<CanonPath> files{CanonPath::root};
for (auto path : wd.files) {
while (!path.isRoot()) {
if (!files.insert(path).second) break;
path.pop();
}
}
Comment on lines +949 to +955
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a generic solution for getting the closure of path parents. Would be nice to factor out.


auto fileAccessor = AllowListInputAccessor::create(
makeFSInputAccessor(path),
{},
std::move(files),
std::move(makeNotAllowedError));

if (exportIgnore)
return make_ref<GitExportIgnoreInputAccessor>(self, fileAccessor, std::nullopt);
else
Expand Down
19 changes: 5 additions & 14 deletions src/libutil/canon-path.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,25 +89,16 @@ CanonPath CanonPath::operator / (std::string_view c) const
return res;
}

bool CanonPath::isAllowed(const std::set<CanonPath> & allowed) const
bool CanonPath::isAllowed(const std::unordered_set<CanonPath> & allowed) const
{
/* Check if `this` is an exact match or the parent of an
allowed path. */
auto lb = allowed.lower_bound(*this);
if (lb != allowed.end()) {
if (lb->isWithin(*this))
return true;
}

/* Check if a parent of `this` is allowed. */
auto path = *this;
while (!path.isRoot()) {
path.pop();
while (true) {
if (allowed.count(path))
return true;
if (path.isRoot())
return false;
path.pop();
}

return false;
}

std::ostream & operator << (std::ostream & stream, const CanonPath & path)
Expand Down
10 changes: 4 additions & 6 deletions src/libutil/canon-path.hh
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#include <optional>
#include <cassert>
#include <iostream>
#include <set>
#include <unordered_set>
#include <vector>

namespace nix {
Expand Down Expand Up @@ -210,12 +210,10 @@ public:
CanonPath operator / (std::string_view c) const;

/**
* Check whether access to this path is allowed, which is the case
* if 1) `this` is within any of the `allowed` paths; or 2) any of
* the `allowed` paths are within `this`. (The latter condition
* ensures access to the parents of allowed paths.)
* Check whether access to this path is allowed, i.e. `this` is
* within any of the `allowed` paths.
*/
bool isAllowed(const std::set<CanonPath> & allowed) const;
bool isAllowed(const std::unordered_set<CanonPath> & allowed) const;

/**
* Return a representation `x` of `path` relative to `this`, i.e.
Expand Down
2 changes: 1 addition & 1 deletion src/libutil/source-accessor.hh
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ struct SourceAccessor
std::optional<uint64_t> narOffset;
};

Stat lstat(const CanonPath & path);
virtual Stat lstat(const CanonPath & path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC if allowed paths is closed under taking parent dirs, then we don't need this, because this is just done for sake of resolving symlinks?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary to get the correct error message for inaccessible paths, i.e. forbidden in restricted mode rather than does not exist.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense,

I was thinking more the other thing, having lstatMaybe not allow /nix and /nix/store again. And the interaction with the other PR more broadly.


virtual std::optional<Stat> maybeLstat(const CanonPath & path) = 0;

Expand Down
4 changes: 4 additions & 0 deletions tests/functional/flakes/flakes.sh
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ cat > "$flake2Dir/flake.nix" <<EOF

outputs = { self, flake1 }: rec {
packages.$system.bar = flake1.packages.$system.foo;
foo = builtins.pathExists (self + "/..");
};
}
EOF
Expand Down Expand Up @@ -251,6 +252,9 @@ nix build -o "$TEST_ROOT/result" "$flake2Dir#bar" --commit-lock-file
[[ -e "$flake2Dir/flake.lock" ]]
[[ -z $(git -C "$flake2Dir" diff main || echo failed) ]]

# Test that pathExist on the parent of a flake returns false.
[[ $(nix eval "$flake2Dir#foo") = false ]]

# Rerunning the build should not change the lockfile.
nix build -o "$TEST_ROOT/result" "$flake2Dir#bar"
[[ -z $(git -C "$flake2Dir" diff main || echo failed) ]]
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/repl.sh
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ foo + baz
' "3" \
./flake ./flake\#bar --experimental-features 'flakes'

# Test the `:reload` mechansim with flakes:
# Test the `:reload` mechanism with flakes:
# - Eval `./flake#changingThing`
# - Modify the flake
# - Re-eval it
Expand Down
30 changes: 14 additions & 16 deletions tests/functional/restricted.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@ source common.sh
clearStore

nix-instantiate --restrict-eval --eval -E '1 + 2'
(! nix-instantiate --eval --restrict-eval ./restricted.nix)
(! nix-instantiate --eval --restrict-eval <(echo '1 + 2'))
expectStderr 1 nix-instantiate --eval --restrict-eval ./restricted.nix | grepQuiet "forbidden in restricted mode"
expectStderr 1 nix-instantiate --eval --restrict-eval <(echo '1 + 2') | grepQuiet "forbidden in restricted mode"
nix-instantiate --restrict-eval ./simple.nix -I src=.
nix-instantiate --restrict-eval ./simple.nix -I src1=simple.nix -I src2=config.nix -I src3=./simple.builder.sh

(! nix-instantiate --restrict-eval --eval -E 'builtins.readFile ./simple.nix')
expectStderr 1 nix-instantiate --restrict-eval --eval -E 'builtins.readFile ./simple.nix' | grepQuiet "forbidden in restricted mode"
nix-instantiate --restrict-eval --eval -E 'builtins.readFile ./simple.nix' -I src=../..

(! nix-instantiate --restrict-eval --eval -E 'builtins.readDir ../../src/nix-channel')
expectStderr 1 nix-instantiate --restrict-eval --eval -E 'builtins.readDir ../../src/nix-channel' | grepQuiet "forbidden in restricted mode"
nix-instantiate --restrict-eval --eval -E 'builtins.readDir ../../src/nix-channel' -I src=../../src

expectStderr 1 nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; path = ./.; } ]; in builtins.readFile <foo/simple.nix>' | grepQuiet "forbidden in restricted mode"
Expand All @@ -20,21 +20,21 @@ nix-instantiate --restrict-eval --eval -E 'let __nixPath = [ { prefix = "foo"; p
p=$(nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval --allowed-uris "file://$(pwd)")
cmp $p restricted.sh

(! nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval)
expectStderr 1 nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval | grepQuiet "forbidden in restricted mode"

(! nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval --allowed-uris "file://$(pwd)/restricted.sh/")
expectStderr 1 nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval --allowed-uris "file://$(pwd)/restricted.sh/" | grepQuiet "forbidden in restricted mode"

nix eval --raw --expr "builtins.fetchurl file://$(pwd)/restricted.sh" --impure --restrict-eval --allowed-uris "file://$(pwd)/restricted.sh"

(! nix eval --raw --expr "builtins.fetchurl https://github.com/NixOS/patchelf/archive/master.tar.gz" --impure --restrict-eval)
(! nix eval --raw --expr "builtins.fetchTarball https://github.com/NixOS/patchelf/archive/master.tar.gz" --impure --restrict-eval)
(! nix eval --raw --expr "fetchGit git://github.com/NixOS/patchelf.git" --impure --restrict-eval)
expectStderr 1 nix eval --raw --expr "builtins.fetchurl https://github.com/NixOS/patchelf/archive/master.tar.gz" --impure --restrict-eval | grepQuiet "forbidden in restricted mode"
expectStderr 1 nix eval --raw --expr "builtins.fetchTarball https://github.com/NixOS/patchelf/archive/master.tar.gz" --impure --restrict-eval | grepQuiet "forbidden in restricted mode"
expectStderr 1 nix eval --raw --expr "fetchGit git://github.com/NixOS/patchelf.git" --impure --restrict-eval | grepQuiet "forbidden in restricted mode"

ln -sfn $(pwd)/restricted.nix $TEST_ROOT/restricted.nix
[[ $(nix-instantiate --eval $TEST_ROOT/restricted.nix) == 3 ]]
(! nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix)
(! nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I $TEST_ROOT)
(! nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I .)
expectStderr 1 nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix | grepQuiet "forbidden in restricted mode"
expectStderr 1 nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I $TEST_ROOT | grepQuiet "forbidden in restricted mode"
expectStderr 1 nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I . | grepQuiet "forbidden in restricted mode"
nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I $TEST_ROOT -I .

[[ $(nix eval --raw --impure --restrict-eval -I . --expr 'builtins.readFile "${import ./simple.nix}/hello"') == 'Hello World!' ]]
Expand All @@ -48,8 +48,8 @@ expectStderr 1 nix-instantiate --restrict-eval --eval -E "let __nixPath = [ { pr

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"; }' ]]
# Reading the parents of allowed paths is forbidden.
expectStderr 1 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 | grepQuiet "forbidden in restricted mode"

# Check whether we can leak symlink information through directory traversal.
traverseDir="$(pwd)/restricted-traverse-me"
Expand All @@ -61,5 +61,3 @@ output="$(nix eval --raw --restrict-eval -I "$traverseDir" \
2>&1 || :)"
echo "$output" | grep "is forbidden"
echo "$output" | grepInverse -F restricted-secret

expectStderr 1 nix-instantiate --restrict-eval true ./dependencies.nix | grepQuiet "forbidden in restricted mode"
10 changes: 5 additions & 5 deletions tests/unit/libutil/canon-path.cc
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ namespace nix {
}

TEST(CanonPath, allowed) {
std::set<CanonPath> allowed {
std::unordered_set<CanonPath> allowed {
CanonPath("foo/bar"),
CanonPath("foo!"),
CanonPath("xyzzy"),
Expand All @@ -152,19 +152,19 @@ namespace nix {

ASSERT_TRUE (CanonPath("foo/bar").isAllowed(allowed));
ASSERT_TRUE (CanonPath("foo/bar/bla").isAllowed(allowed));
ASSERT_TRUE (CanonPath("foo").isAllowed(allowed));
ASSERT_FALSE(CanonPath("foo").isAllowed(allowed));
ASSERT_FALSE(CanonPath("bar").isAllowed(allowed));
ASSERT_FALSE(CanonPath("bar/a").isAllowed(allowed));
ASSERT_TRUE (CanonPath("a").isAllowed(allowed));
ASSERT_TRUE (CanonPath("a/b").isAllowed(allowed));
ASSERT_FALSE(CanonPath("a").isAllowed(allowed));
ASSERT_FALSE(CanonPath("a/b").isAllowed(allowed));
ASSERT_TRUE (CanonPath("a/b/c").isAllowed(allowed));
ASSERT_TRUE (CanonPath("a/b/c/d").isAllowed(allowed));
ASSERT_TRUE (CanonPath("a/b/c/d/e").isAllowed(allowed));
ASSERT_FALSE(CanonPath("a/b/a").isAllowed(allowed));
ASSERT_FALSE(CanonPath("a/b/d").isAllowed(allowed));
ASSERT_FALSE(CanonPath("aaa").isAllowed(allowed));
ASSERT_FALSE(CanonPath("zzz").isAllowed(allowed));
ASSERT_TRUE (CanonPath("/").isAllowed(allowed));
ASSERT_FALSE(CanonPath("/").isAllowed(allowed));
}

TEST(CanonPath, makeRelative) {
Expand Down