From 9ddd0f2af8fd95e1380027a70d0aa650ea2fd5e4 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 27 Jan 2024 11:18:03 +0100 Subject: [PATCH 1/5] Revert "StorePath: reject names starting with '.'" This reverts commit 24bda0c7b381e1a017023c6f7cb9661fae8560bd. --- src/libstore/path-regex.hh | 2 +- src/libstore/path.cc | 2 -- tests/unit/libstore-support/tests/path.cc | 8 ++------ tests/unit/libstore/path.cc | 1 - 4 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/libstore/path-regex.hh b/src/libstore/path-regex.hh index a44e6a2eb..4f8dc4c1f 100644 --- a/src/libstore/path-regex.hh +++ b/src/libstore/path-regex.hh @@ -3,6 +3,6 @@ namespace nix { -static constexpr std::string_view nameRegexStr = R"([0-9a-zA-Z\+\-_\?=][0-9a-zA-Z\+\-\._\?=]*)"; +static constexpr std::string_view nameRegexStr = R"([0-9a-zA-Z\+\-\._\?=]+)"; } diff --git a/src/libstore/path.cc b/src/libstore/path.cc index a15a78545..4361b3194 100644 --- a/src/libstore/path.cc +++ b/src/libstore/path.cc @@ -9,8 +9,6 @@ static void checkName(std::string_view path, std::string_view name) if (name.size() > StorePath::MaxPathLen) throw BadStorePath("store path '%s' has a name longer than %d characters", path, StorePath::MaxPathLen); - if (name[0] == '.') - throw BadStorePath("store path '%s' starts with illegal character '.'", path); // See nameRegexStr for the definition for (auto c : name) if (!((c >= '0' && c <= '9') diff --git a/tests/unit/libstore-support/tests/path.cc b/tests/unit/libstore-support/tests/path.cc index e5f169e94..bbe43bad4 100644 --- a/tests/unit/libstore-support/tests/path.cc +++ b/tests/unit/libstore-support/tests/path.cc @@ -46,12 +46,8 @@ Gen Arbitrary::arbitrary() pre += '-'; break; case 64: - // names aren't permitted to start with a period, - // so just fall through to the next case here - if (c != 0) { - pre += '.'; - break; - } + pre += '.'; + break; case 65: pre += '_'; break; diff --git a/tests/unit/libstore/path.cc b/tests/unit/libstore/path.cc index 30631b5fd..5485ab8bb 100644 --- a/tests/unit/libstore/path.cc +++ b/tests/unit/libstore/path.cc @@ -39,7 +39,6 @@ TEST_DONT_PARSE(double_star, "**") TEST_DONT_PARSE(star_first, "*,foo") TEST_DONT_PARSE(star_second, "foo,*") TEST_DONT_PARSE(bang, "foo!o") -TEST_DONT_PARSE(dotfile, ".gitignore") #undef TEST_DONT_PARSE From b13e6a76b4f289c6db69ffaa7bd35b7e44f2a391 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 27 Jan 2024 11:19:05 +0100 Subject: [PATCH 2/5] parseStorePath: Support leading period --- doc/manual/rl-next/leading-period.md | 10 ++++++++++ tests/unit/libstore/path.cc | 1 + 2 files changed, 11 insertions(+) create mode 100644 doc/manual/rl-next/leading-period.md diff --git a/doc/manual/rl-next/leading-period.md b/doc/manual/rl-next/leading-period.md new file mode 100644 index 000000000..e9a32a74a --- /dev/null +++ b/doc/manual/rl-next/leading-period.md @@ -0,0 +1,10 @@ +--- +synopsis: Store paths are allowed to start with `.` +issues: 912 +prs: 9867 9091 9095 9120 9121 9122 9130 9219 9224 +--- + +Leading periods were allowed by accident in Nix 2.4. The Nix team has considered this to be a bug, but this behavior has since been relied on by users, leading to unnecessary difficulties. +From now on, leading periods are officially, definitively supported. + +Nix versions that denied leading periods are documented [in the issue](https://github.com/NixOS/nix/issues/912#issuecomment-1919583286). diff --git a/tests/unit/libstore/path.cc b/tests/unit/libstore/path.cc index 5485ab8bb..f7b69d5f9 100644 --- a/tests/unit/libstore/path.cc +++ b/tests/unit/libstore/path.cc @@ -62,6 +62,7 @@ TEST_DO_PARSE(underscore, "foo_bar") TEST_DO_PARSE(period, "foo.txt") TEST_DO_PARSE(question_mark, "foo?why") TEST_DO_PARSE(equals_sign, "foo=foo") +TEST_DO_PARSE(dotfile, ".gitignore") #undef TEST_DO_PARSE From 69bbd5852af9b2f0b794162bd1debcdf64fc6648 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 30 Jan 2024 18:18:27 +0100 Subject: [PATCH 3/5] test: Generate distinct path names Gen::just is the constant generator. Don't just return that! --- tests/unit/libstore-support/tests/path.cc | 72 ++++++++++++----------- 1 file changed, 37 insertions(+), 35 deletions(-) diff --git a/tests/unit/libstore-support/tests/path.cc b/tests/unit/libstore-support/tests/path.cc index bbe43bad4..8ddda8027 100644 --- a/tests/unit/libstore-support/tests/path.cc +++ b/tests/unit/libstore-support/tests/path.cc @@ -1,3 +1,4 @@ +#include #include #include @@ -20,59 +21,60 @@ void showValue(const StorePath & p, std::ostream & os) namespace rc { using namespace nix; -Gen Arbitrary::arbitrary() +Gen storePathChar() { - auto len = *gen::inRange( - 1, - StorePath::MaxPathLen - StorePath::HashLen); - - std::string pre; - pre.reserve(len); - - for (size_t c = 0; c < len; ++c) { - switch (auto i = *gen::inRange(0, 10 + 2 * 26 + 6)) { + return rc::gen::apply([](uint8_t i) -> char { + switch (i) { case 0 ... 9: - pre += '0' + i; + return '0' + i; case 10 ... 35: - pre += 'A' + (i - 10); - break; + return 'A' + (i - 10); case 36 ... 61: - pre += 'a' + (i - 36); - break; + return 'a' + (i - 36); case 62: - pre += '+'; - break; + return '+'; case 63: - pre += '-'; - break; + return '-'; case 64: - pre += '.'; - break; + return '.'; case 65: - pre += '_'; - break; + return '_'; case 66: - pre += '?'; - break; + return '?'; case 67: - pre += '='; - break; + return '='; default: assert(false); } - } + }, + gen::inRange(0, 10 + 2 * 26 + 6)); +} - return gen::just(StorePathName { - .name = std::move(pre), - }); +Gen Arbitrary::arbitrary() +{ + return gen::construct( + gen::suchThat( + gen::container(storePathChar()), + [](const std::string & s) { + return + !( s == "" + || s == "." + || s == ".." + || s.starts_with(".-") + || s.starts_with("..-") + ); + } + ) + ); } Gen Arbitrary::arbitrary() { - return gen::just(StorePath { - *gen::arbitrary(), - (*gen::arbitrary()).name, - }); + return + gen::construct( + gen::arbitrary(), + gen::apply([](StorePathName n){ return n.name; }, gen::arbitrary()) + ); } } // namespace rc From 8406da28773f050e00a006e4812e3ecbf919a2a9 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 30 Jan 2024 18:31:28 +0100 Subject: [PATCH 4/5] test: Generate distinct hashes Gen::just is the constant generator. Don't just return that! --- tests/unit/libutil-support/tests/hash.cc | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/unit/libutil-support/tests/hash.cc b/tests/unit/libutil-support/tests/hash.cc index 50889cd33..51b9663b4 100644 --- a/tests/unit/libutil-support/tests/hash.cc +++ b/tests/unit/libutil-support/tests/hash.cc @@ -11,10 +11,17 @@ using namespace nix; Gen Arbitrary::arbitrary() { - Hash hash(HashAlgorithm::SHA1); - for (size_t i = 0; i < hash.hashSize; ++i) - hash.hash[i] = *gen::arbitrary(); - return gen::just(hash); + Hash prototype(HashAlgorithm::SHA1); + return + gen::apply( + [](const std::vector & v) { + Hash hash(HashAlgorithm::SHA1); + assert(v.size() == hash.hashSize); + std::copy(v.begin(), v.end(), hash.hash); + return hash; + }, + gen::container>(prototype.hashSize, gen::arbitrary()) + ); } } From f1b4663805a9dbcb1ace64ec110092d17c9155e0 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 30 Jan 2024 18:37:23 +0100 Subject: [PATCH 5/5] Disallow store path names that are . or .. (plus opt. -) As discussed in the maintainer meeting on 2024-01-29. Mainly this is to avoid a situation where the name is parsed and treated as a file name, mostly to protect users. .-* and ..-* are also considered invalid because they might strip on that separator to remove versions. Doesn't really work, but that's what we decided, and I won't argue with it, because .-* probably doesn't seem to have a real world application anyway. We do still permit a 1-character name that's just "-", which still poses a similar risk in such a situation. We can't start disallowing trailing -, because a non-zero number of users will need it and we've seen how annoying and painful such a change is. What matters most is preventing a situation where . or .. can be injected, and to just get this done. --- doc/manual/rl-next/leading-period.md | 2 +- src/libstore/path-regex.hh | 7 ++- src/libstore/path.cc | 13 ++++++ tests/unit/libstore/path.cc | 68 ++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 2 deletions(-) diff --git a/doc/manual/rl-next/leading-period.md b/doc/manual/rl-next/leading-period.md index e9a32a74a..ef7c2326f 100644 --- a/doc/manual/rl-next/leading-period.md +++ b/doc/manual/rl-next/leading-period.md @@ -5,6 +5,6 @@ prs: 9867 9091 9095 9120 9121 9122 9130 9219 9224 --- Leading periods were allowed by accident in Nix 2.4. The Nix team has considered this to be a bug, but this behavior has since been relied on by users, leading to unnecessary difficulties. -From now on, leading periods are officially, definitively supported. +From now on, leading periods are officially, definitively supported. The names `.` and `..` are disallowed, as well as those starting with `.-` or `..-`. Nix versions that denied leading periods are documented [in the issue](https://github.com/NixOS/nix/issues/912#issuecomment-1919583286). diff --git a/src/libstore/path-regex.hh b/src/libstore/path-regex.hh index 4f8dc4c1f..56c2cfc1d 100644 --- a/src/libstore/path-regex.hh +++ b/src/libstore/path-regex.hh @@ -3,6 +3,11 @@ namespace nix { -static constexpr std::string_view nameRegexStr = R"([0-9a-zA-Z\+\-\._\?=]+)"; + +static constexpr std::string_view nameRegexStr = + // This uses a negative lookahead: (?!\.\.?(-|$)) + // - deny ".", "..", or those strings followed by '-' + // - when it's not those, start again at the start of the input and apply the next regex, which is [0-9a-zA-Z\+\-\._\?=]+ + R"((?!\.\.?(-|$))[0-9a-zA-Z\+\-\._\?=]+)"; } diff --git a/src/libstore/path.cc b/src/libstore/path.cc index 4361b3194..5db4b974c 100644 --- a/src/libstore/path.cc +++ b/src/libstore/path.cc @@ -10,6 +10,19 @@ static void checkName(std::string_view path, std::string_view name) throw BadStorePath("store path '%s' has a name longer than %d characters", path, StorePath::MaxPathLen); // See nameRegexStr for the definition + if (name[0] == '.') { + // check against "." and "..", followed by end or dash + if (name.size() == 1) + throw BadStorePath("store path '%s' has invalid name '%s'", path, name); + if (name[1] == '-') + throw BadStorePath("store path '%s' has invalid name '%s': first dash-separated component must not be '%s'", path, name, "."); + if (name[1] == '.') { + if (name.size() == 2) + throw BadStorePath("store path '%s' has invalid name '%s'", path, name); + if (name[2] == '-') + throw BadStorePath("store path '%s' has invalid name '%s': first dash-separated component must not be '%s'", path, name, ".."); + } + } for (auto c : name) if (!((c >= '0' && c <= '9') || (c >= 'a' && c <= 'z') diff --git a/tests/unit/libstore/path.cc b/tests/unit/libstore/path.cc index f7b69d5f9..213b6e95f 100644 --- a/tests/unit/libstore/path.cc +++ b/tests/unit/libstore/path.cc @@ -39,6 +39,12 @@ TEST_DONT_PARSE(double_star, "**") TEST_DONT_PARSE(star_first, "*,foo") TEST_DONT_PARSE(star_second, "foo,*") TEST_DONT_PARSE(bang, "foo!o") +TEST_DONT_PARSE(dot, ".") +TEST_DONT_PARSE(dot_dot, "..") +TEST_DONT_PARSE(dot_dot_dash, "..-1") +TEST_DONT_PARSE(dot_dash, ".-1") +TEST_DONT_PARSE(dot_dot_dash_a, "..-a") +TEST_DONT_PARSE(dot_dash_a, ".-a") #undef TEST_DONT_PARSE @@ -63,6 +69,10 @@ TEST_DO_PARSE(period, "foo.txt") TEST_DO_PARSE(question_mark, "foo?why") TEST_DO_PARSE(equals_sign, "foo=foo") TEST_DO_PARSE(dotfile, ".gitignore") +TEST_DO_PARSE(triple_dot_a, "...a") +TEST_DO_PARSE(triple_dot_1, "...1") +TEST_DO_PARSE(triple_dot_dash, "...-") +TEST_DO_PARSE(triple_dot, "...") #undef TEST_DO_PARSE @@ -84,6 +94,64 @@ RC_GTEST_FIXTURE_PROP( RC_ASSERT(p == store->parseStorePath(store->printStorePath(p))); } + +RC_GTEST_FIXTURE_PROP( + StorePathTest, + prop_check_regex_eq_parse, + ()) +{ + static auto nameFuzzer = + rc::gen::container( + rc::gen::oneOf( + // alphanum, repeated to weigh heavier + rc::gen::oneOf( + rc::gen::inRange('0', '9'), + rc::gen::inRange('a', 'z'), + rc::gen::inRange('A', 'Z') + ), + // valid symbols + rc::gen::oneOf( + rc::gen::just('+'), + rc::gen::just('-'), + rc::gen::just('.'), + rc::gen::just('_'), + rc::gen::just('?'), + rc::gen::just('=') + ), + // symbols for scary .- and ..- cases, repeated for weight + rc::gen::just('.'), rc::gen::just('.'), + rc::gen::just('.'), rc::gen::just('.'), + rc::gen::just('-'), rc::gen::just('-'), + // ascii symbol ranges + rc::gen::oneOf( + rc::gen::inRange(' ', '/'), + rc::gen::inRange(':', '@'), + rc::gen::inRange('[', '`'), + rc::gen::inRange('{', '~') + ), + // typical whitespace + rc::gen::oneOf( + rc::gen::just(' '), + rc::gen::just('\t'), + rc::gen::just('\n'), + rc::gen::just('\r') + ), + // some chance of control codes, non-ascii or other garbage we missed + rc::gen::inRange('\0', '\xff') + )); + + auto name = *nameFuzzer; + + std::string path = store->storeDir + "/575s52sh487i0ylmbs9pvi606ljdszr0-" + name; + bool parsed = false; + try { + store->parseStorePath(path); + parsed = true; + } catch (const BadStorePath &) { + } + RC_ASSERT(parsed == std::regex_match(std::string { name }, nameRegex)); +} + #endif }