From eba85e23670c57448aaf29d2fedec356e4cd2fe7 Mon Sep 17 00:00:00 2001 From: Nick Van den Broeck Date: Wed, 1 May 2019 10:34:23 +0200 Subject: [PATCH 1/8] WIP: still need to adapt flakeref parsing --- src/libexpr/primops/flake.cc | 22 ++++++++++++---------- src/libexpr/primops/flakeref.cc | 1 + src/libexpr/primops/flakeref.hh | 11 ++++++----- 3 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/libexpr/primops/flake.cc b/src/libexpr/primops/flake.cc index 88a0293e3..e806ef6c7 100644 --- a/src/libexpr/primops/flake.cc +++ b/src/libexpr/primops/flake.cc @@ -194,11 +194,8 @@ static FlakeRef lookupFlake(EvalState & state, const FlakeRef & flakeRef, const return flakeRef; } -static FlakeSourceInfo fetchFlake(EvalState & state, const FlakeRef flakeRef, bool impureIsAllowed = false) +static FlakeSourceInfo fetchFlake(EvalState & state, const FlakeRef fRef, bool impureIsAllowed = false) { - FlakeRef fRef = lookupFlake(state, flakeRef, - impureIsAllowed ? state.getFlakeRegistries() : std::vector>()); - if (evalSettings.pureEval && !impureIsAllowed && !fRef.isImmutable()) throw Error("requested to fetch mutable flake '%s' in pure mode", fRef); @@ -266,26 +263,31 @@ static FlakeSourceInfo fetchFlake(EvalState & state, const FlakeRef flakeRef, bo // This will return the flake which corresponds to a given FlakeRef. The lookupFlake is done within this function. Flake getFlake(EvalState & state, const FlakeRef & flakeRef, bool impureIsAllowed = false) { - FlakeSourceInfo sourceInfo = fetchFlake(state, flakeRef, impureIsAllowed); + FlakeRef resolvedRef = lookupFlake(state, flakeRef, + impureIsAllowed ? state.getFlakeRegistries() : std::vector>()); + + FlakeSourceInfo sourceInfo = fetchFlake(state, resolvedRef, impureIsAllowed); debug("got flake source '%s' with revision %s", sourceInfo.storePath, sourceInfo.rev.value_or(Hash(htSHA1)).to_string(Base16, false)); + resolvedRef = sourceInfo.flakeRef; // `resolvedRef` is now immutable + state.store->assertStorePath(sourceInfo.storePath); if (state.allowedPaths) state.allowedPaths->insert(sourceInfo.storePath); - Flake flake(flakeRef, std::move(sourceInfo)); - if (std::get_if(&flakeRef.data)) { + Flake flake(resolvedRef, std::move(sourceInfo)); + if (std::get_if(&resolvedRef.data)) { // FIXME: ehm? if (flake.sourceInfo.rev) - flake.ref = FlakeRef(flakeRef.baseRef().to_string() + flake.ref = FlakeRef(resolvedRef.baseRef().to_string() + "/" + flake.sourceInfo.rev->to_string(Base16, false)); } - Path flakeFile = sourceInfo.storePath + "/flake.nix"; + Path flakeFile = sourceInfo.storePath + resolvedRef.subdir + "/flake.nix"; if (!pathExists(flakeFile)) - throw Error("source tree referenced by '%s' does not contain a 'flake.nix' file", flakeRef); + throw Error("source tree referenced by '%s' does not contain a 'flake.nix' file", resolvedRef); Value vInfo; state.evalFile(flakeFile, vInfo); // FIXME: symlink attack diff --git a/src/libexpr/primops/flakeref.cc b/src/libexpr/primops/flakeref.cc index b91bbee2a..022535515 100644 --- a/src/libexpr/primops/flakeref.cc +++ b/src/libexpr/primops/flakeref.cc @@ -139,6 +139,7 @@ std::string FlakeRef::to_string() const string += (ref ? "/" + *ref : "") + (rev ? "/" + rev->to_string(Base16, false) : ""); + if (subdir != "") string += "?dir=" + subdir; return string; } diff --git a/src/libexpr/primops/flakeref.hh b/src/libexpr/primops/flakeref.hh index 51fdc3b70..299094634 100644 --- a/src/libexpr/primops/flakeref.hh +++ b/src/libexpr/primops/flakeref.hh @@ -69,7 +69,7 @@ namespace nix { https://example.org/my/repo.git https://example.org/my/repo.git?ref=release-1.2.3 https://example.org/my/repo.git?rev=e72daba8250068216d79d2aeef40d4d95aff6666 - git://github.com/edolstra/dwarffs.git\?ref=flake\&rev=2efca4bc9da70fb001b26c3dc858c6397d3c4817 + git://github.com/edolstra/dwarffs.git?ref=flake&rev=2efca4bc9da70fb001b26c3dc858c6397d3c4817 * /path.git(\?attr(&attr)*)? @@ -144,17 +144,18 @@ struct FlakeRef std::optional ref; std::optional rev; + Path subdir = ""; // This is a relative path pointing at the flake.nix file's directory, relative to the git root. bool operator<(const FlakeRef & flakeRef) const { - return std::make_tuple(this->data, ref, rev) < - std::make_tuple(flakeRef.data, flakeRef.ref, flakeRef.rev); + return std::make_tuple(data, ref, rev, subdir) < + std::make_tuple(flakeRef.data, flakeRef.ref, flakeRef.rev, subdir); } bool operator==(const FlakeRef & flakeRef) const { - return std::make_tuple(this->data, ref, rev) == - std::make_tuple(flakeRef.data, flakeRef.ref, flakeRef.rev); + return std::make_tuple(data, ref, rev, subdir) == + std::make_tuple(flakeRef.data, flakeRef.ref, flakeRef.rev, flakeRef.subdir); } // Parse a flake URI. From 00db8d4549b1e87db45395e157dec3f729bdc071 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 1 May 2019 14:24:22 +0200 Subject: [PATCH 2/8] Support 'dir' parameters in https and ssh flake URIs --- src/libexpr/primops/flakeref.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libexpr/primops/flakeref.cc b/src/libexpr/primops/flakeref.cc index 022535515..66eab4db6 100644 --- a/src/libexpr/primops/flakeref.cc +++ b/src/libexpr/primops/flakeref.cc @@ -30,7 +30,9 @@ const static std::string schemeRegex = "(?:http|https|ssh|git|file)"; const static std::string authorityRegex = "[a-zA-Z0-9._~-]*"; const static std::string segmentRegex = "[a-zA-Z0-9._~-]+"; const static std::string pathRegex = "/?" + segmentRegex + "(?:/" + segmentRegex + ")*"; -const static std::string paramRegex = "[a-z]+=[a-zA-Z0-9._-]*"; +// FIXME: support escaping in query string. +// Note: '/' is not a valid query parameter, but so what... +const static std::string paramRegex = "[a-z]+=[/a-zA-Z0-9._-]*"; FlakeRef::FlakeRef(const std::string & uri, bool allowRelative) { @@ -97,6 +99,9 @@ FlakeRef::FlakeRef(const std::string & uri, bool allowRelative) if (!std::regex_match(value, refRegex2)) throw Error("invalid Git ref '%s'", value); ref = value; + } else if (name == "dir") { + // FIXME: validate value; should not contain relative paths + subdir = value; } else // FIXME: should probably pass through unknown parameters throw Error("invalid Git flake reference parameter '%s', in '%s'", name, uri); From 43408d3cd6e254c1c69eb9cf9de4de042b986ab6 Mon Sep 17 00:00:00 2001 From: Nick Van den Broeck Date: Wed, 1 May 2019 16:24:33 +0200 Subject: [PATCH 3/8] flake.lock now uses flakeRef.subdir --- src/libexpr/primops/flake.cc | 21 ++++++++++++--------- src/libexpr/primops/flake.hh | 2 +- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/libexpr/primops/flake.cc b/src/libexpr/primops/flake.cc index e806ef6c7..41b3f6d28 100644 --- a/src/libexpr/primops/flake.cc +++ b/src/libexpr/primops/flake.cc @@ -285,7 +285,7 @@ Flake getFlake(EvalState & state, const FlakeRef & flakeRef, bool impureIsAllowe + "/" + flake.sourceInfo.rev->to_string(Base16, false)); } - Path flakeFile = sourceInfo.storePath + resolvedRef.subdir + "/flake.nix"; + Path flakeFile = sourceInfo.storePath + "/" + resolvedRef.subdir + "/flake.nix"; if (!pathExists(flakeFile)) throw Error("source tree referenced by '%s' does not contain a 'flake.nix' file", resolvedRef); @@ -367,7 +367,7 @@ ResolvedFlake resolveFlake(EvalState & state, const FlakeRef & topRef, LockFile lockFile; if (isTopFlake) - lockFile = readLockFile(flake.sourceInfo.storePath + "/flake.lock"); // FIXME: symlink attack + lockFile = readLockFile(flake.sourceInfo.storePath + "/" + flake.resolvedRef.subdir + "/flake.lock"); // FIXME: symlink attack ResolvedFlake deps(flake); @@ -407,16 +407,19 @@ static LockFile makeLockFile(EvalState & evalState, FlakeRef & flakeRef) return lockFile; } -void updateLockFile(EvalState & state, const Path & path) +void updateLockFile(EvalState & state, const FlakeUri & flakeUri) { // FIXME: We are writing the lockfile to the store here! Very bad practice! - FlakeRef flakeRef = FlakeRef(path); - auto lockFile = makeLockFile(state, flakeRef); - writeLockFile(lockFile, path + "/flake.lock"); + FlakeRef flakeRef = FlakeRef(flakeUri); + if (auto refData = std::get_if(flakeRef)) { + auto lockFile = makeLockFile(state, flakeRef); + writeLockFile(lockFile, refData->path + "/" + flakeRef.subdir + "/flake.lock"); - // Hack: Make sure that flake.lock is visible to Git. Otherwise, - // exportGit will fail to copy it to the Nix store. - runProgram("git", true, { "-C", path, "add", "flake.lock" }); + // Hack: Make sure that flake.lock is visible to Git. Otherwise, + // exportGit will fail to copy it to the Nix store. + runProgram("git", true, { "-C", refData->path, "add", flakeRef.subDir + "/flake.lock" }); + } else + throw Error("flakeUri %s can't be updated because it is not a path", flakeUri); } void callFlake(EvalState & state, const ResolvedFlake & resFlake, Value & v) diff --git a/src/libexpr/primops/flake.hh b/src/libexpr/primops/flake.hh index 8e9af5843..6329c36ec 100644 --- a/src/libexpr/primops/flake.hh +++ b/src/libexpr/primops/flake.hh @@ -90,7 +90,7 @@ struct ResolvedFlake ResolvedFlake resolveFlake(EvalState &, const FlakeRef &, RegistryAccess registryAccess, bool isTopFlake = true); -void updateLockFile(EvalState &, const Path & path); +void updateLockFile(EvalState &, const FlakeUri &); void gitCloneFlake (std::string flakeUri, EvalState &, Registries, Path); } From ab9e47284a46f1a933d84c9c66f8fb4ba6c4ba34 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 1 May 2019 18:07:36 +0200 Subject: [PATCH 4/8] Improve error message --- src/libexpr/primops/flake.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/primops/flake.cc b/src/libexpr/primops/flake.cc index 41b3f6d28..293e5ad0b 100644 --- a/src/libexpr/primops/flake.cc +++ b/src/libexpr/primops/flake.cc @@ -287,7 +287,7 @@ Flake getFlake(EvalState & state, const FlakeRef & flakeRef, bool impureIsAllowe Path flakeFile = sourceInfo.storePath + "/" + resolvedRef.subdir + "/flake.nix"; if (!pathExists(flakeFile)) - throw Error("source tree referenced by '%s' does not contain a 'flake.nix' file", resolvedRef); + throw Error("source tree referenced by '%s' does not contain a '%s/flake.nix' file", resolvedRef, resolvedRef.subdir); Value vInfo; state.evalFile(flakeFile, vInfo); // FIXME: symlink attack From fa88f7152070d4c886b512de00691da709bc7229 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 1 May 2019 20:23:39 +0200 Subject: [PATCH 5/8] Validate 'dir=' parameters We reject any path element starting with a '.' (mostly to reject '.' and '..'). --- src/libexpr/primops/flakeref.cc | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/libexpr/primops/flakeref.cc b/src/libexpr/primops/flakeref.cc index 66eab4db6..56ba58d09 100644 --- a/src/libexpr/primops/flakeref.cc +++ b/src/libexpr/primops/flakeref.cc @@ -34,6 +34,11 @@ const static std::string pathRegex = "/?" + segmentRegex + "(?:/" + segmentRegex // Note: '/' is not a valid query parameter, but so what... const static std::string paramRegex = "[a-z]+=[/a-zA-Z0-9._-]*"; +// 'dir' path elements cannot start with a '.'. We also reject +// potentially dangerous characters like ';'. +const static std::string subDirElemRegex = "(?:[a-zA-Z0-9_-]+[a-zA-Z0-9._-]*)"; +const static std::string subDirRegex = subDirElemRegex + "(?:/" + subDirElemRegex + ")*"; + FlakeRef::FlakeRef(const std::string & uri, bool allowRelative) { // FIXME: could combine this into one regex. @@ -55,6 +60,8 @@ FlakeRef::FlakeRef(const std::string & uri, bool allowRelative) static std::regex refRegex2(refRegex, std::regex::ECMAScript); + static std::regex subDirRegex2(subDirRegex, std::regex::ECMAScript); + std::cmatch match; if (std::regex_match(uri.c_str(), match, flakeRegex)) { IsAlias d; @@ -100,7 +107,8 @@ FlakeRef::FlakeRef(const std::string & uri, bool allowRelative) throw Error("invalid Git ref '%s'", value); ref = value; } else if (name == "dir") { - // FIXME: validate value; should not contain relative paths + if (!std::regex_match(value, subDirRegex2)) + throw Error("flake '%s' has invalid subdirectory '%s'", uri, value); subdir = value; } else // FIXME: should probably pass through unknown parameters @@ -124,6 +132,7 @@ FlakeRef::FlakeRef(const std::string & uri, bool allowRelative) std::string FlakeRef::to_string() const { std::string string; + if (auto refData = std::get_if(&data)) string = refData->alias; @@ -142,9 +151,12 @@ std::string FlakeRef::to_string() const else abort(); + // FIXME: need to use ?rev etc. for IsGit URIs. string += (ref ? "/" + *ref : "") + (rev ? "/" + rev->to_string(Base16, false) : ""); + if (subdir != "") string += "?dir=" + subdir; + return string; } From a37436d7929f37fb390837419d166a81559abb3e Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 1 May 2019 20:38:41 +0200 Subject: [PATCH 6/8] Accept empty directories --- src/libexpr/primops/flake.cc | 5 ++++- src/libexpr/primops/flakeref.cc | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libexpr/primops/flake.cc b/src/libexpr/primops/flake.cc index 293e5ad0b..de692f167 100644 --- a/src/libexpr/primops/flake.cc +++ b/src/libexpr/primops/flake.cc @@ -285,7 +285,10 @@ Flake getFlake(EvalState & state, const FlakeRef & flakeRef, bool impureIsAllowe + "/" + flake.sourceInfo.rev->to_string(Base16, false)); } - Path flakeFile = sourceInfo.storePath + "/" + resolvedRef.subdir + "/flake.nix"; + // Guard against symlink attacks. + auto flakeFile = canonPath(sourceInfo.storePath + "/" + resolvedRef.subdir + "/flake.nix"); + if (!isInDir(flakeFile, sourceInfo.storePath)) + throw Error("flake file '%s' escapes from '%s'", resolvedRef, sourceInfo.storePath); if (!pathExists(flakeFile)) throw Error("source tree referenced by '%s' does not contain a '%s/flake.nix' file", resolvedRef, resolvedRef.subdir); diff --git a/src/libexpr/primops/flakeref.cc b/src/libexpr/primops/flakeref.cc index 56ba58d09..b7a20a170 100644 --- a/src/libexpr/primops/flakeref.cc +++ b/src/libexpr/primops/flakeref.cc @@ -107,7 +107,7 @@ FlakeRef::FlakeRef(const std::string & uri, bool allowRelative) throw Error("invalid Git ref '%s'", value); ref = value; } else if (name == "dir") { - if (!std::regex_match(value, subDirRegex2)) + if (value != "" && !std::regex_match(value, subDirRegex2)) throw Error("flake '%s' has invalid subdirectory '%s'", uri, value); subdir = value; } else From 5d6e8c008ba8716f16ddfad954663d9e732f8556 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 1 May 2019 20:43:16 +0200 Subject: [PATCH 7/8] Allow 'dir' parameter in github: URIs E.g. 'github:edolstra/dwarffs/flake?dir=foo/bar'. --- src/libexpr/primops/flakeref.cc | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/libexpr/primops/flakeref.cc b/src/libexpr/primops/flakeref.cc index b7a20a170..141d61c0d 100644 --- a/src/libexpr/primops/flakeref.cc +++ b/src/libexpr/primops/flakeref.cc @@ -33,12 +33,14 @@ const static std::string pathRegex = "/?" + segmentRegex + "(?:/" + segmentRegex // FIXME: support escaping in query string. // Note: '/' is not a valid query parameter, but so what... const static std::string paramRegex = "[a-z]+=[/a-zA-Z0-9._-]*"; +const static std::string paramsRegex = "(?:[?](" + paramRegex + "(?:&" + paramRegex + ")*))"; // 'dir' path elements cannot start with a '.'. We also reject // potentially dangerous characters like ';'. const static std::string subDirElemRegex = "(?:[a-zA-Z0-9_-]+[a-zA-Z0-9._-]*)"; const static std::string subDirRegex = subDirElemRegex + "(?:/" + subDirElemRegex + ")*"; + FlakeRef::FlakeRef(const std::string & uri, bool allowRelative) { // FIXME: could combine this into one regex. @@ -48,14 +50,15 @@ FlakeRef::FlakeRef(const std::string & uri, bool allowRelative) std::regex::ECMAScript); static std::regex githubRegex( - "github:(" + ownerRegex + ")/(" + repoRegex + ")(?:/" + revOrRefRegex + ")?", + "github:(" + ownerRegex + ")/(" + repoRegex + ")(?:/" + revOrRefRegex + ")?" + + paramsRegex + "?", std::regex::ECMAScript); static std::regex uriRegex( "((" + schemeRegex + "):" + "(?://(" + authorityRegex + "))?" + "(" + pathRegex + "))" + - "(?:[?](" + paramRegex + "(?:&" + paramRegex + ")*))?", + paramsRegex + "?", std::regex::ECMAScript); static std::regex refRegex2(refRegex, std::regex::ECMAScript); @@ -85,6 +88,18 @@ FlakeRef::FlakeRef(const std::string & uri, bool allowRelative) else if (match[4].matched) { ref = match[4]; } + for (auto & param : tokenizeString(match[5], "&")) { + auto n = param.find('='); + assert(n != param.npos); + std::string name(param, 0, n); + std::string value(param, n + 1); + if (name == "dir") { + if (value != "" && !std::regex_match(value, subDirRegex2)) + throw Error("flake '%s' has invalid subdirectory '%s'", uri, value); + subdir = value; + } else + throw Error("invalid Git flake reference parameter '%s', in '%s'", name, uri); + } data = d; } From e0d4aa75fc8957347215f3c52366f47f1e3d6d6e Mon Sep 17 00:00:00 2001 From: Nick Van den Broeck Date: Thu, 2 May 2019 08:40:00 +0200 Subject: [PATCH 8/8] Fixed compile errors --- src/libexpr/primops/flake.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libexpr/primops/flake.cc b/src/libexpr/primops/flake.cc index de692f167..0d9ef36ba 100644 --- a/src/libexpr/primops/flake.cc +++ b/src/libexpr/primops/flake.cc @@ -370,7 +370,7 @@ ResolvedFlake resolveFlake(EvalState & state, const FlakeRef & topRef, LockFile lockFile; if (isTopFlake) - lockFile = readLockFile(flake.sourceInfo.storePath + "/" + flake.resolvedRef.subdir + "/flake.lock"); // FIXME: symlink attack + lockFile = readLockFile(flake.sourceInfo.storePath + "/" + flake.ref.subdir + "/flake.lock"); // FIXME: symlink attack ResolvedFlake deps(flake); @@ -414,13 +414,13 @@ void updateLockFile(EvalState & state, const FlakeUri & flakeUri) { // FIXME: We are writing the lockfile to the store here! Very bad practice! FlakeRef flakeRef = FlakeRef(flakeUri); - if (auto refData = std::get_if(flakeRef)) { + if (auto refData = std::get_if(&flakeRef.data)) { auto lockFile = makeLockFile(state, flakeRef); writeLockFile(lockFile, refData->path + "/" + flakeRef.subdir + "/flake.lock"); // Hack: Make sure that flake.lock is visible to Git. Otherwise, // exportGit will fail to copy it to the Nix store. - runProgram("git", true, { "-C", refData->path, "add", flakeRef.subDir + "/flake.lock" }); + runProgram("git", true, { "-C", refData->path, "add", flakeRef.subdir + "/flake.lock" }); } else throw Error("flakeUri %s can't be updated because it is not a path", flakeUri); }