Merge pull request #5279 from edolstra/restrict-path-inputs

Fix relative path input handling

4 files changed, 34 insertions, 19 deletions

diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc
index 6893f157e..492b47115 100644
--- a/src/libexpr/flake/flake.cc
+++ b/src/libexpr/flake/flake.cc
@@ -89,10 +89,12 @@ static void expectType(EvalState & state, ValueType type,
 }
 
 static std::map<FlakeId, FlakeInput> parseFlakeInputs(
-    EvalState & state, Value * value, const Pos & pos);
+    EvalState & state, Value * value, const Pos & pos,
+    const std::optional<Path> & baseDir);
 
 static FlakeInput parseFlakeInput(EvalState & state,
-    const std::string & inputName, Value * value, const Pos & pos)
+    const std::string & inputName, Value * value, const Pos & pos,
+    const std::optional<Path> & baseDir)
 {
     expectType(state, nAttrs, *value, pos);
 
@@ -116,7 +118,7 @@ static FlakeInput parseFlakeInput(EvalState & state,
                 expectType(state, nBool, *attr.value, *attr.pos);
                 input.isFlake = attr.value->boolean;
             } else if (attr.name == sInputs) {
-                input.overrides = parseFlakeInputs(state, attr.value, *attr.pos);
+                input.overrides = parseFlakeInputs(state, attr.value, *attr.pos, baseDir);
             } else if (attr.name == sFollows) {
                 expectType(state, nString, *attr.value, *attr.pos);
                 input.follows = parseInputPath(attr.value->string.s);
@@ -154,7 +156,7 @@ static FlakeInput parseFlakeInput(EvalState & state,
         if (!attrs.empty())
             throw Error("unexpected flake input attribute '%s', at %s", attrs.begin()->first, pos);
         if (url)
-            input.ref = parseFlakeRef(*url, {}, true);
+            input.ref = parseFlakeRef(*url, baseDir, true);
     }
 
     if (!input.follows && !input.ref)
@@ -164,7 +166,8 @@ static FlakeInput parseFlakeInput(EvalState & state,
 }
 
 static std::map<FlakeId, FlakeInput> parseFlakeInputs(
-    EvalState & state, Value * value, const Pos & pos)
+    EvalState & state, Value * value, const Pos & pos,
+    const std::optional<Path> & baseDir)
 {
     std::map<FlakeId, FlakeInput> inputs;
 
@@ -175,7 +178,8 @@ static std::map<FlakeId, FlakeInput> parseFlakeInputs(
             parseFlakeInput(state,
                 inputAttr.name,
                 inputAttr.value,
-                *inputAttr.pos));
+                *inputAttr.pos,
+                baseDir));
     }
 
     return inputs;
@@ -191,7 +195,8 @@ static Flake getFlake(
         state, originalRef, allowLookup, flakeCache);
 
     // Guard against symlink attacks.
-    auto flakeFile = canonPath(sourceInfo.actualPath + "/" + lockedRef.subdir + "/flake.nix");
+    auto flakeDir = canonPath(sourceInfo.actualPath + "/" + lockedRef.subdir);
+    auto flakeFile = canonPath(flakeDir + "/flake.nix");
     if (!isInDir(flakeFile, sourceInfo.actualPath))
         throw Error("'flake.nix' file of flake '%s' escapes from '%s'",
             lockedRef, state.store->printStorePath(sourceInfo.storePath));
@@ -219,7 +224,7 @@ static Flake getFlake(
     auto sInputs = state.symbols.create("inputs");
 
     if (auto inputs = vInfo.attrs->get(sInputs))
-        flake.inputs = parseFlakeInputs(state, inputs->value, *inputs->pos);
+        flake.inputs = parseFlakeInputs(state, inputs->value, *inputs->pos, flakeDir);
 
     auto sOutputs = state.symbols.create("outputs");
 
@@ -488,10 +493,8 @@ LockedFlake lockFlake(
 
                             // If this input is a path, recurse it down.
                             // This allows us to resolve path inputs relative to the current flake.
-                            if (localRef.input.getType() == "path") {
-                                localRef.input.parent = parentPath;
-                                localPath = canonPath(parentPath + "/" + *input.ref->input.getSourcePath());
-                            }
+                            if (localRef.input.getType() == "path")
+                                localPath = absPath(*input.ref->input.getSourcePath(), parentPath);
 
                             auto inputFlake = getFlake(state, localRef, useRegistries, flakeCache);
 
diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc
index 833e8a776..29128d789 100644
--- a/src/libexpr/flake/flakeref.cc
+++ b/src/libexpr/flake/flakeref.cc
@@ -172,8 +172,12 @@ std::pair<FlakeRef, std::string> parseFlakeRefWithFragment(
         auto parsedURL = parseURL(url);
         std::string fragment;
         std::swap(fragment, parsedURL.fragment);
+
+        auto input = Input::fromURL(parsedURL);
+        input.parent = baseDir;
+
         return std::make_pair(
-            FlakeRef(Input::fromURL(parsedURL), get(parsedURL.query, "dir").value_or("")),
+            FlakeRef(std::move(input), get(parsedURL.query, "dir").value_or("")),
             fragment);
     }
 }
diff --git a/src/libfetchers/path.cc b/src/libfetchers/path.cc
index b6fcdac9e..fb5702c4c 100644
--- a/src/libfetchers/path.cc
+++ b/src/libfetchers/path.cc
@@ -85,18 +85,26 @@ struct PathInputScheme : InputScheme
         std::string absPath;
         auto path = getStrAttr(input.attrs, "path");
 
-        if (path[0] != '/' && input.parent) {
+        if (path[0] != '/') {
+            if (!input.parent)
+                throw Error("cannot fetch input '%s' because it uses a relative path", input.to_string());
+
             auto parent = canonPath(*input.parent);
 
             // the path isn't relative, prefix it
-            absPath = canonPath(parent + "/" + path);
+            absPath = nix::absPath(path, parent);
 
             // for security, ensure that if the parent is a store path, it's inside it
-            if (!parent.rfind(store->storeDir, 0) && absPath.rfind(store->storeDir, 0))
-                throw BadStorePath("relative path '%s' points outside of its parent's store path %s, this is a security violation", path, parent);
+            if (store->isInStore(parent)) {
+                auto storePath = store->printStorePath(store->toStorePath(parent).first);
+                if (!isInDir(absPath, storePath))
+                    throw BadStorePath("relative path '%s' points outside of its parent's store path '%s'", path, storePath);
+            }
         } else
             absPath = path;
 
+        Activity act(*logger, lvlTalkative, actUnknown, fmt("copying '%s'", absPath));
+
         // FIXME: check whether access to 'path' is allowed.
         auto storePath = store->maybeParseStorePath(absPath);
 
diff --git a/tests/flakes.sh b/tests/flakes.sh
index 2ede7f72c..26cdf27b7 100644
--- a/tests/flakes.sh
+++ b/tests/flakes.sh
@@ -766,7 +766,7 @@ cat > $flakeFollowsA/flake.nix <<EOF
 {
     description = "Flake A";
     inputs = {
-        B.url = "path:./../../flakeB";
+        B.url = "path:../flakeB";
     };
     outputs = { ... }: {};
 }
@@ -774,7 +774,7 @@ EOF
 
 git -C $flakeFollowsA add flake.nix
 
-nix flake lock $flakeFollowsA 2>&1 | grep 'this is a security violation'
+nix flake lock $flakeFollowsA 2>&1 | grep 'points outside'
 
 # Test flake in store does not evaluate
 rm -rf $badFlakeDir