From fdba67d4fa5c88fee81a5de211b9b2a2f41be7c7 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 17 May 2022 11:53:02 +0200 Subject: [PATCH] Fix access control --- src/libfetchers/git.cc | 18 ++--------- src/libfetchers/input-accessor.cc | 15 ++------- src/libutil/canon-path.cc | 36 +++++++++++++++++++++ src/libutil/canon-path.hh | 30 ++++++++++++++++- src/libutil/tests/canon-path.cc | 54 +++++++++++++++++++++++++++++++ 5 files changed, 125 insertions(+), 28 deletions(-) diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index 2fe98f135..cc4bc5152 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -682,22 +682,10 @@ struct GitInputScheme : InputScheme auto files = listFiles(repoInfo); + CanonPath repoDir(repoInfo.url); + PathFilter filter = [&](const Path & p) -> bool { - abort(); -#if 0 - assert(hasPrefix(p, repoInfo.url)); - std::string file(p, repoInfo.url.size() + 1); - - auto st = lstat(p); - - if (S_ISDIR(st.st_mode)) { - auto prefix = file + "/"; - auto i = files.lower_bound(prefix); - return i != files.end() && hasPrefix(*i, prefix); - } - - return files.count(file); -#endif + return CanonPath(p).removePrefix(repoDir).isAllowed(files); }; auto storePath = store->addToStore(input.getName(), repoInfo.url, FileIngestionMethod::Recursive, htSHA256, filter); diff --git a/src/libfetchers/input-accessor.cc b/src/libfetchers/input-accessor.cc index 04c3c2618..a64678855 100644 --- a/src/libfetchers/input-accessor.cc +++ b/src/libfetchers/input-accessor.cc @@ -181,18 +181,9 @@ struct FSInputAccessorImpl : FSInputAccessor return false; if (allowedPaths) { - #if 0 - // FIXME: this can be done more efficiently. - auto p = (std::string) absPath.substr(root.size()); - if (p == "") p = "/"; - while (true) { - if (allowedPaths->find(p) != allowedPaths->end()) - break; - if (p == "/") - return false; - p = dirOf(p); - } - #endif + auto p = absPath.removePrefix(root); + if (!p.isAllowed(*allowedPaths)) + return false; } return true; diff --git a/src/libutil/canon-path.cc b/src/libutil/canon-path.cc index e00d5caa8..e854ad950 100644 --- a/src/libutil/canon-path.cc +++ b/src/libutil/canon-path.cc @@ -19,6 +19,13 @@ std::optional CanonPath::parent() const return CanonPath(unchecked_t(), path.substr(0, path.rfind('/'))); } +void CanonPath::pop() +{ + assert(!isRoot()); + auto slash = path.rfind('/'); + path.resize(std::max((size_t) 1, slash)); +} + CanonPath CanonPath::resolveSymlinks() const { return CanonPath(unchecked_t(), canonPath(abs(), true)); @@ -33,6 +40,14 @@ bool CanonPath::isWithin(const CanonPath & parent) const && path[parent.path.size()] != '/')); } +CanonPath CanonPath::removePrefix(const CanonPath & prefix) const +{ + assert(isWithin(prefix)); + if (prefix.isRoot()) return *this; + if (path.size() == prefix.path.size()) return root; + return CanonPath(unchecked_t(), path.substr(prefix.path.size())); +} + void CanonPath::extend(const CanonPath & x) { if (x.isRoot()) return; @@ -63,6 +78,27 @@ CanonPath CanonPath::operator + (std::string_view c) const return res; } +bool CanonPath::isAllowed(const std::set & allowed) const +{ + /* Check if `this` is an exact match or the parent of an + allowed path. */ + auto lb = allowed.lower_bound(*this); + if (lb != allowed.end()) { + if (lb->isWithin(*this)) + return true; + } + + /* Check if a parent of `this` is allowed. */ + auto path = *this; + while (!path.isRoot()) { + path.pop(); + if (allowed.count(path)) + return true; + } + + return false; +} + std::ostream & operator << (std::ostream & stream, const CanonPath & path) { stream << path.abs(); diff --git a/src/libutil/canon-path.hh b/src/libutil/canon-path.hh index 036de37e4..62333b949 100644 --- a/src/libutil/canon-path.hh +++ b/src/libutil/canon-path.hh @@ -4,6 +4,7 @@ #include #include #include +#include namespace nix { @@ -95,6 +96,9 @@ public: std::optional parent() const; + /* Remove the last component. Panics if this path is the root. */ + void pop(); + std::optional dirOf() const { if (isRoot()) return std::nullopt; @@ -113,8 +117,24 @@ public: bool operator != (const CanonPath & x) const { return path != x.path; } + /* Compare paths lexicographically except that path separators + are sorted before any other character. That is, in the sorted order + a directory is always followed directly by its children. For + instance, 'foo' < 'foo/bar' < 'foo!'. */ bool operator < (const CanonPath & x) const - { return path < x.path; } + { + auto i = path.begin(); + auto j = x.path.begin(); + for ( ; i != path.end() && j != x.path.end(); ++i, ++j) { + auto c_i = *i; + if (c_i == '/') c_i = 0; + auto c_j = *j; + if (c_j == '/') c_j = 0; + if (c_i < c_j) return true; + if (c_i > c_j) return false; + } + return i == path.end() && j != x.path.end(); + } CanonPath resolveSymlinks() const; @@ -122,6 +142,8 @@ public: `parent`. */ bool isWithin(const CanonPath & parent) const; + CanonPath removePrefix(const CanonPath & prefix) const; + /* Append another path to this one. */ void extend(const CanonPath & x); @@ -132,6 +154,12 @@ public: void push(std::string_view c); CanonPath operator + (std::string_view c) const; + + /* Check whether access to this path is allowed, which is the case + if 1) `this` is within any of the `allowed` paths; or 2) any of + the `allowed` paths are within `this`. (The latter condition + ensures access to the parents of allowed paths.) */ + bool isAllowed(const std::set & allowed) const; }; std::ostream & operator << (std::ostream & stream, const CanonPath & path); diff --git a/src/libutil/tests/canon-path.cc b/src/libutil/tests/canon-path.cc index b1e343d5f..28fa789a0 100644 --- a/src/libutil/tests/canon-path.cc +++ b/src/libutil/tests/canon-path.cc @@ -38,6 +38,25 @@ namespace nix { } } + TEST(CanonPath, pop) { + CanonPath p("foo/bar/x"); + ASSERT_EQ(p.abs(), "/foo/bar/x"); + p.pop(); + ASSERT_EQ(p.abs(), "/foo/bar"); + p.pop(); + ASSERT_EQ(p.abs(), "/foo"); + p.pop(); + ASSERT_EQ(p.abs(), "/"); + } + + TEST(CanonPath, removePrefix) { + CanonPath p1("foo/bar"); + CanonPath p2("foo/bar/a/b/c"); + ASSERT_EQ(p2.removePrefix(p1).abs(), "/a/b/c"); + ASSERT_EQ(p1.removePrefix(p1).abs(), "/"); + ASSERT_EQ(p1.removePrefix(CanonPath("/")).abs(), "/foo/bar"); + } + TEST(CanonPath, iter) { { CanonPath p("a//foo/bar//"); @@ -95,4 +114,39 @@ namespace nix { ASSERT_TRUE(CanonPath("/").isWithin(CanonPath("/"))); } } + + TEST(CanonPath, sort) { + ASSERT_FALSE(CanonPath("foo") < CanonPath("foo")); + ASSERT_TRUE (CanonPath("foo") < CanonPath("foo/bar")); + ASSERT_TRUE (CanonPath("foo/bar") < CanonPath("foo!")); + ASSERT_FALSE(CanonPath("foo!") < CanonPath("foo")); + ASSERT_TRUE (CanonPath("foo") < CanonPath("foo!")); + } + + TEST(CanonPath, allowed) { + { + std::set allowed { + CanonPath("foo/bar"), + CanonPath("foo!"), + CanonPath("xyzzy"), + CanonPath("a/b/c"), + }; + + ASSERT_TRUE (CanonPath("foo/bar").isAllowed(allowed)); + ASSERT_TRUE (CanonPath("foo/bar/bla").isAllowed(allowed)); + ASSERT_TRUE (CanonPath("foo").isAllowed(allowed)); + ASSERT_FALSE(CanonPath("bar").isAllowed(allowed)); + ASSERT_FALSE(CanonPath("bar/a").isAllowed(allowed)); + ASSERT_TRUE (CanonPath("a").isAllowed(allowed)); + ASSERT_TRUE (CanonPath("a/b").isAllowed(allowed)); + ASSERT_TRUE (CanonPath("a/b/c").isAllowed(allowed)); + ASSERT_TRUE (CanonPath("a/b/c/d").isAllowed(allowed)); + ASSERT_TRUE (CanonPath("a/b/c/d/e").isAllowed(allowed)); + ASSERT_FALSE(CanonPath("a/b/a").isAllowed(allowed)); + ASSERT_FALSE(CanonPath("a/b/d").isAllowed(allowed)); + ASSERT_FALSE(CanonPath("aaa").isAllowed(allowed)); + ASSERT_FALSE(CanonPath("zzz").isAllowed(allowed)); + ASSERT_TRUE (CanonPath("/").isAllowed(allowed)); + } + } }