From 5cbb9c5406f3058fcc9f99692490fbc5a4f57876 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 21 Sep 2021 13:19:26 +0200 Subject: [PATCH 1/3] path fetcher: Fix relative path check --- src/libfetchers/path.cc | 13 +++++++++---- tests/flakes.sh | 4 ++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/libfetchers/path.cc b/src/libfetchers/path.cc index b6fcdac9e..b35c04cfc 100644 --- a/src/libfetchers/path.cc +++ b/src/libfetchers/path.cc @@ -85,18 +85,23 @@ 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) && !isInDir(absPath, parent)) + throw BadStorePath("relative path '%s' [%s] points outside of its parent's store path '%s'", path, absPath, parent); } 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 <&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 From 06557299b30f53c6d4a1a2e3068f36d9ef3df904 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 21 Sep 2021 13:45:11 +0200 Subject: [PATCH 2/3] Allow relative paths anywhere into the parent's store path --- src/libfetchers/path.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libfetchers/path.cc b/src/libfetchers/path.cc index b35c04cfc..fb5702c4c 100644 --- a/src/libfetchers/path.cc +++ b/src/libfetchers/path.cc @@ -95,8 +95,11 @@ struct PathInputScheme : InputScheme absPath = nix::absPath(path, parent); // for security, ensure that if the parent is a store path, it's inside it - if (store->isInStore(parent) && !isInDir(absPath, parent)) - throw BadStorePath("relative path '%s' [%s] points outside of its parent's store path '%s'", path, absPath, 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; From 60cc975d22c193cd70cc4e8d3fb5643728aec418 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 21 Sep 2021 14:07:16 +0200 Subject: [PATCH 3/3] Set input parent at construction time --- src/libexpr/flake/flake.cc | 27 +++++++++++++++------------ src/libexpr/flake/flakeref.cc | 6 +++++- 2 files changed, 20 insertions(+), 13 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 parseFlakeInputs( - EvalState & state, Value * value, const Pos & pos); + EvalState & state, Value * value, const Pos & pos, + const std::optional & 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 & 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 parseFlakeInputs( - EvalState & state, Value * value, const Pos & pos) + EvalState & state, Value * value, const Pos & pos, + const std::optional & baseDir) { std::map inputs; @@ -175,7 +178,8 @@ static std::map 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 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); } }