From f1b4663805a9dbcb1ace64ec110092d17c9155e0 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 30 Jan 2024 18:37:23 +0100 Subject: [PATCH] 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 }