Skip to content

Commit 83c067c

Browse files
edolstraedolstra
committed
PosixSourceAccessor: Don't follow any symlinks
All path components must not be symlinks now (so the user needs to call `resolveSymlinks()` when needed).
1 parent 345f79d commit 83c067c

7 files changed

Lines changed: 56 additions & 28 deletions

File tree

‎src/libexpr/parser.y‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -692,16 +692,17 @@ SourcePath resolveExprPath(SourcePath path)
692692

693693
/* If `path' is a symlink, follow it. This is so that relative
694694
path references work. */
695-
while (true) {
695+
while (!path.path.isRoot()) {
696696
// Basic cycle/depth limit to avoid infinite loops.
697697
if (++followCount >= maxFollow)
698698
throw Error("too many symbolic links encountered while traversing the path '%s'", path);
699-
if (path.lstat().type != InputAccessor::tSymlink) break;
700-
path = {path.accessor, CanonPath(path.readLink(), path.path.parent().value_or(CanonPath::root))};
699+
auto p = path.parent().resolveSymlinks() + path.baseName();
700+
if (p.lstat().type != InputAccessor::tSymlink) break;
701+
path = {path.accessor, CanonPath(p.readLink(), path.path.parent().value_or(CanonPath::root))};
701702
}
702703

703704
/* If `path' refers to a directory, append `/default.nix'. */
704-
if (path.lstat().type == InputAccessor::tDirectory)
705+
if (path.resolveSymlinks().lstat().type == InputAccessor::tDirectory)
705706
return path + "default.nix";
706707

707708
return path;
@@ -716,7 +717,7 @@ Expr * EvalState::parseExprFromFile(const SourcePath & path)
716717

717718
Expr * EvalState::parseExprFromFile(const SourcePath & path, std::shared_ptr<StaticEnv> & staticEnv)
718719
{
719-
auto buffer = path.readFile();
720+
auto buffer = path.resolveSymlinks().readFile();
720721
// readFile hopefully have left some extra space for terminators
721722
buffer.append("\0\0", 2);
722723
return parse(buffer.data(), buffer.size(), Pos::Origin(path), path.parent(), staticEnv);

‎src/libexpr/primops.cc‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ StringMap EvalState::realiseContext(const NixStringContext & context)
110110
return res;
111111
}
112112

113-
static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v)
113+
static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v, bool resolveSymlinks = true)
114114
{
115115
NixStringContext context;
116116

@@ -120,9 +120,9 @@ static SourcePath realisePath(EvalState & state, const PosIdx pos, Value & v)
120120
if (!context.empty() && path.accessor == state.rootFS) {
121121
auto rewrites = state.realiseContext(context);
122122
auto realPath = state.toRealPath(rewriteStrings(path.path.abs(), rewrites), context);
123-
return {path.accessor, CanonPath(realPath)};
124-
} else
125-
return path;
123+
path = {path.accessor, CanonPath(realPath)};
124+
}
125+
return resolveSymlinks ? path.resolveSymlinks() : path;
126126
} catch (Error & e) {
127127
e.addTrace(state.positions[pos], "while realising the context of path '%s'", path);
128128
throw;
@@ -162,7 +162,7 @@ static void mkOutputString(
162162
argument. */
163163
static void import(EvalState & state, const PosIdx pos, Value & vPath, Value * vScope, Value & v)
164164
{
165-
auto path = realisePath(state, pos, vPath);
165+
auto path = realisePath(state, pos, vPath, false);
166166
auto path2 = path.path.abs();
167167

168168
// FIXME
@@ -1525,16 +1525,16 @@ static RegisterPrimOp primop_storePath({
15251525

15261526
static void prim_pathExists(EvalState & state, const PosIdx pos, Value * * args, Value & v)
15271527
{
1528-
auto & arg = *args[0];
1528+
try {
1529+
auto & arg = *args[0];
15291530

1530-
auto path = realisePath(state, pos, arg);
1531+
auto path = realisePath(state, pos, arg);
15311532

1532-
/* SourcePath doesn't know about trailing slash. */
1533-
auto mustBeDir = arg.type() == nString
1534-
&& (arg.string_view().ends_with("/")
1535-
|| arg.string_view().ends_with("/."));
1533+
/* SourcePath doesn't know about trailing slash. */
1534+
auto mustBeDir = arg.type() == nString
1535+
&& (arg.string_view().ends_with("/")
1536+
|| arg.string_view().ends_with("/."));
15361537

1537-
try {
15381538
auto st = path.maybeLstat();
15391539
auto exists = st && (!mustBeDir || st->type == SourceAccessor::tDirectory);
15401540
v.mkBool(exists);
@@ -1771,7 +1771,7 @@ static std::string_view fileTypeToString(InputAccessor::Type type)
17711771

17721772
static void prim_readFileType(EvalState & state, const PosIdx pos, Value * * args, Value & v)
17731773
{
1774-
auto path = realisePath(state, pos, *args[0]);
1774+
auto path = realisePath(state, pos, *args[0], false);
17751775
/* Retrieve the directory entry type and stringize it. */
17761776
v.mkString(fileTypeToString(path.lstat().type));
17771777
}

‎src/libutil/posix-source-accessor.cc‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ void PosixSourceAccessor::readFile(
88
Sink & sink,
99
std::function<void(uint64_t)> sizeCallback)
1010
{
11-
// FIXME: add O_NOFOLLOW since symlinks should be resolved by the
12-
// caller?
13-
AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC);
11+
assertNoSymlinks(path);
12+
13+
AutoCloseFD fd = open(path.c_str(), O_RDONLY | O_CLOEXEC | O_NOFOLLOW);
1414
if (!fd)
1515
throw SysError("opening file '%1%'", path);
1616

@@ -42,14 +42,16 @@ void PosixSourceAccessor::readFile(
4242

4343
bool PosixSourceAccessor::pathExists(const CanonPath & path)
4444
{
45+
if (auto parent = path.parent()) assertNoSymlinks(*parent);
4546
return nix::pathExists(path.abs());
4647
}
4748

4849
std::optional<SourceAccessor::Stat> PosixSourceAccessor::maybeLstat(const CanonPath & path)
4950
{
51+
if (auto parent = path.parent()) assertNoSymlinks(*parent);
5052
struct stat st;
5153
if (::lstat(path.c_str(), &st)) {
52-
if (errno == ENOENT) return std::nullopt;
54+
if (errno == ENOENT || errno == ENOTDIR) return std::nullopt;
5355
throw SysError("getting status of '%s'", showPath(path));
5456
}
5557
mtime = std::max(mtime, st.st_mtime);
@@ -66,6 +68,7 @@ std::optional<SourceAccessor::Stat> PosixSourceAccessor::maybeLstat(const CanonP
6668

6769
SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath & path)
6870
{
71+
assertNoSymlinks(path);
6972
DirEntries res;
7073
for (auto & entry : nix::readDirectory(path.abs())) {
7174
std::optional<Type> type;
@@ -81,6 +84,7 @@ SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath &
8184

8285
std::string PosixSourceAccessor::readLink(const CanonPath & path)
8386
{
87+
if (auto parent = path.parent()) assertNoSymlinks(*parent);
8488
return nix::readLink(path.abs());
8589
}
8690

@@ -89,4 +93,19 @@ std::optional<CanonPath> PosixSourceAccessor::getPhysicalPath(const CanonPath &
8993
return path;
9094
}
9195

96+
void PosixSourceAccessor::assertNoSymlinks(CanonPath path)
97+
{
98+
// FIXME: cache this since it potentially causes a lot of lstat calls.
99+
while (!path.isRoot()) {
100+
struct stat st;
101+
if (::lstat(path.c_str(), &st)) {
102+
if (errno != ENOENT)
103+
throw SysError("getting status of '%s'", showPath(path));
104+
}
105+
if (S_ISLNK(st.st_mode))
106+
throw Error("path '%s' is a symlink", showPath(path));
107+
path.pop();
108+
}
109+
}
110+
92111
}

‎src/libutil/posix-source-accessor.hh‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,11 @@ struct PosixSourceAccessor : virtual SourceAccessor
2929
std::string readLink(const CanonPath & path) override;
3030

3131
std::optional<CanonPath> getPhysicalPath(const CanonPath & path) override;
32+
33+
/**
34+
* Throw an error if `path` or any of its ancestors are symlinks.
35+
*/
36+
void assertNoSymlinks(CanonPath path);
3237
};
3338

3439
}

‎src/nix-env/nix-env.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ static bool isNixExpr(const SourcePath & path, struct InputAccessor::Stat & st)
9797
{
9898
return
9999
st.type == InputAccessor::tRegular
100-
|| (st.type == InputAccessor::tDirectory && (path + "default.nix").pathExists());
100+
|| (st.type == InputAccessor::tDirectory && (path + "default.nix").resolveSymlinks().pathExists());
101101
}
102102

103103

@@ -116,11 +116,11 @@ static void getAllExprs(EvalState & state,
116116
are implemented using profiles). */
117117
if (i == "manifest.nix") continue;
118118

119-
SourcePath path2 = path + i;
119+
auto path2 = (path + i).resolveSymlinks();
120120

121121
InputAccessor::Stat st;
122122
try {
123-
st = path2.resolveSymlinks().lstat();
123+
st = path2.lstat();
124124
} catch (Error &) {
125125
continue; // ignore dangling symlinks in ~/.nix-defexpr
126126
}

‎src/nix-env/user-env.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ DrvInfos queryInstalled(EvalState & state, const Path & userEnv)
2121
auto manifestFile = userEnv + "/manifest.nix";
2222
if (pathExists(manifestFile)) {
2323
Value v;
24-
state.evalFile(state.rootPath(CanonPath(manifestFile)), v);
24+
state.evalFile(state.rootPath(CanonPath(manifestFile)).resolveSymlinks(), v);
2525
Bindings & bindings(*state.allocBindings(0));
2626
getDerivations(state, v, "", bindings, elems, false);
2727
}

‎tests/functional/restricted.sh‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,13 +40,16 @@ nix-instantiate --eval --restrict-eval $TEST_ROOT/restricted.nix -I $TEST_ROOT -
4040
[[ $(nix eval --raw --impure --restrict-eval -I . --expr 'builtins.readFile "${import ./simple.nix}/hello"') == 'Hello World!' ]]
4141

4242
# Check that we can't follow a symlink outside of the allowed paths.
43-
mkdir -p $TEST_ROOT/tunnel.d
43+
mkdir -p $TEST_ROOT/tunnel.d $TEST_ROOT/foo2
4444
ln -sfn .. $TEST_ROOT/tunnel.d/tunnel
4545
echo foo > $TEST_ROOT/bar
4646

4747
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"
4848

49-
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"
49+
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"
50+
51+
# Reading the parents of allowed paths should show only the ancestors of the allowed paths.
52+
[[ $(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"; }' ]]
5053

5154
# Check whether we can leak symlink information through directory traversal.
5255
traverseDir="$(pwd)/restricted-traverse-me"

0 commit comments

Comments
 (0)