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

@@ -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> & 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);

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,6 +1525,7 @@ static RegisterPrimOp primop_storePath({
 
 static void prim_pathExists(EvalState & state, const PosIdx pos, Value * * args, Value & v)
 {
+    try {
         auto & arg = *args[0];
 
         auto path = realisePath(state, pos, arg);
@@ -1534,7 +1535,6 @@ static void prim_pathExists(EvalState & state, const PosIdx pos, Value * * args,
             && (arg.string_view().ends_with("/")
                 || arg.string_view().ends_with("/."));
 
-    try {
         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));
 }

src/libutil/posix-source-accessor.cc

@@ -8,9 +8,9 @@ void PosixSourceAccessor::readFile(
     Sink & sink,
     std::function<void(uint64_t)> 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<SourceAccessor::Stat> 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<SourceAccessor::Stat> 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> 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<CanonPath> 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();
+    }
+}
+
 }

src/libutil/posix-source-accessor.hh

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

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

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