From 83c067c0fa0cc5a2dca440e5c986afe40b163802 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 5 Dec 2023 23:02:59 +0100 Subject: [PATCH] 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"