commit 4423e81d1efb2f4794f745ca853c548650c299b0
parent 5ec230addfc7ba95aa8cbb863df704e8e93240ec
Author: cfillion <cfillion@users.noreply.github.com>
Date: Fri, 12 Feb 2016 00:40:19 -0500
harden against directory traversal vulnerability
Diffstat:
10 files changed, 170 insertions(+), 25 deletions(-)
diff --git a/src/package.cpp b/src/package.cpp
@@ -79,7 +79,7 @@ const Version *Package::lastVersion() const
return *m_versions.rbegin();
}
-Path Package::targetPath() const
+Path Package::makeTargetPath(const string &file) const
{
Path path;
@@ -90,7 +90,9 @@ Path Package::targetPath() const
case ScriptType:
path.append("Scripts");
path.append(m_category->index()->name());
- path.append(m_category->name());
+
+ // only allow directory traversal up to the index name
+ path += Path(m_category->name()) + file;
break;
default:
throw reapack_error("unsupported package type");
diff --git a/src/package.hpp b/src/package.hpp
@@ -48,7 +48,7 @@ public:
const Version *version(size_t index) const;
const Version *lastVersion() const;
- Path targetPath() const;
+ Path makeTargetPath(const std::string &file = {}) const;
private:
Category *m_category;
diff --git a/src/path.cpp b/src/path.cpp
@@ -18,6 +18,7 @@
#include "path.hpp"
#include <algorithm>
+#include <boost/range/adaptor/reversed.hpp>
#include <vector>
using namespace std;
@@ -28,6 +29,9 @@ static const char SEPARATOR = '/';
static const char SEPARATOR = '\\';
#endif
+static const string DOT = ".";
+static const string DOTDOT = "..";
+
Path Path::s_root;
static vector<string> Split(const string &input)
@@ -46,7 +50,7 @@ static vector<string> Split(const string &input)
const string part = input.substr(last, pos - last);
- if(!part.empty())
+ if(!part.empty() && part != ".")
list.push_back(part);
last = pos + 1;
@@ -60,22 +64,48 @@ Path::Path(const std::string &path)
append(path);
}
-void Path::prepend(const string &parts)
+void Path::prepend(const string &str)
{
- if(parts.empty())
+ if(str.empty())
return;
- for(const string &part : Split(parts))
- m_parts.push_front(part);
+ bool skip = false;
+
+ const vector<string> &parts = Split(str);
+
+ for(const string &part : parts | boost::adaptors::reversed) {
+ if(part == DOTDOT)
+ skip = true;
+ else if(!skip)
+ m_parts.push_front(part);
+ else
+ skip = false;
+ }
}
-void Path::append(const string &parts)
+void Path::append(const string &parts, const bool traversal)
{
if(parts.empty())
return;
- for(const string &part : Split(parts))
- m_parts.push_back(part);
+ for(const string &part : Split(parts)) {
+ if(part == DOTDOT) {
+ if(traversal)
+ removeLast();
+ }
+ else
+ m_parts.push_back(part);
+ }
+}
+
+void Path::prepend(const Path &o)
+{
+ m_parts.insert(m_parts.begin(), o.m_parts.begin(), o.m_parts.end());
+}
+
+void Path::append(const Path &o)
+{
+ m_parts.insert(m_parts.end(), o.m_parts.begin(), o.m_parts.end());
}
void Path::clear()
@@ -166,11 +196,23 @@ Path Path::operator+(const string &part) const
Path Path::operator+(const Path &o) const
{
Path path(*this);
- path.m_parts.insert(path.m_parts.end(), o.m_parts.begin(), o.m_parts.end());
+ path += o;
return path;
}
+const Path &Path::operator+=(const string &parts)
+{
+ append(parts);
+ return *this;
+}
+
+const Path &Path::operator+=(const Path &o)
+{
+ append(o);
+ return *this;
+}
+
const string &Path::at(const size_t index) const
{
auto it = m_parts.begin();
diff --git a/src/path.hpp b/src/path.hpp
@@ -33,7 +33,9 @@ public:
Path(const std::string &path = std::string());
void prepend(const std::string &part);
- void append(const std::string &part);
+ void append(const std::string &part, bool traversal = true);
+ void prepend(const Path &other);
+ void append(const Path &other);
void removeLast();
void clear();
@@ -51,6 +53,8 @@ public:
bool operator<(const Path &) const;
Path operator+(const std::string &) const;
Path operator+(const Path &) const;
+ const Path &operator+=(const std::string &);
+ const Path &operator+=(const Path &);
std::string &operator[](size_t);
const std::string &operator[](size_t) const;
diff --git a/src/remote.cpp b/src/remote.cpp
@@ -30,12 +30,14 @@ static char DATA_DELIMITER = '|';
static bool ValidateName(const string &name)
{
- static const regex pattern("^[^~#%&*{}\\\\:<>?/+|\"]+$");
+ static const regex validPattern("[^~#%&*{}\\\\:<>?/+|\"]+");
+ static const regex invalidPattern("\\.+");
- smatch match;
- regex_match(name, match, pattern);
+ smatch match, invalid;
+ regex_match(name, match, validPattern);
+ regex_match(name, invalid, invalidPattern);
- return !match.empty();
+ return !match.empty() && invalid.empty();
}
static bool ValidateUrl(const string &url)
diff --git a/src/source.cpp b/src/source.cpp
@@ -86,5 +86,5 @@ Path Source::targetPath() const
if(!pkg)
throw reapack_error("no package associated with the source");
- return pkg->targetPath() + file();
+ return pkg->makeTargetPath(file());
}
diff --git a/test/package.cpp b/test/package.cpp
@@ -81,14 +81,14 @@ TEST_CASE("add owned version", M) {
}
}
-TEST_CASE("unknown target path", M) {
+TEST_CASE("target path for unknown package type", M) {
RemoteIndex ri("name");
Category cat("name", &ri);
Package pack(Package::UnknownType, "a", &cat);
try {
- pack.targetPath();
+ pack.makeTargetPath({});
FAIL();
}
catch(const reapack_error &e) {
@@ -107,14 +107,27 @@ TEST_CASE("script target path", M) {
expected.append("Remote Name");
expected.append("Category Name");
- REQUIRE(pack.targetPath() == expected);
+ REQUIRE(pack.makeTargetPath() == expected);
+}
+
+TEST_CASE("script target path: directory traversal", M) {
+ RemoteIndex ri("Remote Name");
+ Category cat("../..", &ri);
+
+ Package pack(Package::ScriptType, "file.name", &cat);
+
+ Path expected;
+ expected.append("Scripts");
+ expected.append("Remote Name");
+
+ REQUIRE(pack.makeTargetPath() == expected);
}
TEST_CASE("script target path without category", M) {
Package pack(Package::ScriptType, "file.name");
try {
- pack.targetPath();
+ pack.makeTargetPath();
FAIL();
}
catch(const reapack_error &e) {
diff --git a/test/path.cpp b/test/path.cpp
@@ -62,7 +62,7 @@ TEST_CASE("concatenate paths", M) {
Path c;
c.append("hello");
- c.append("world");
+ c += "world";
REQUIRE(a + b == c);
REQUIRE(a + "world" == c);
@@ -123,13 +123,17 @@ TEST_CASE("split input", M) {
a.prepend("hello/world");
REQUIRE(a.size() == 2);
+ REQUIRE(a[0] == "hello");
+ REQUIRE(a[1] == "world");
}
SECTION("append") {
Path a;
a.append("hello/world");
-
REQUIRE(a.size() == 2);
+
+ a += "chunky/bacon";
+ REQUIRE(a.size() == 4);
}
SECTION("skip empty parts") {
@@ -193,3 +197,46 @@ TEST_CASE("first and last path component", M) {
REQUIRE(path.first() == "hello");
REQUIRE(path.last() == "bacon");
}
+
+TEST_CASE("directory traversal", M) {
+ SECTION("append (enabled)") {
+ REQUIRE(Path("a/./b") == Path("a/b"));
+ REQUIRE(Path("a/../b") == Path("b"));
+ REQUIRE(Path("../a") == Path("a"));
+ }
+
+ SECTION("append (disabled)") {
+ Path a;
+ a.append("a/../b", false);
+ REQUIRE(a == Path("a/b"));
+ }
+
+ SECTION("prepend") {
+ Path a;
+ a.prepend("a/../b/c/../d");
+ REQUIRE(a == Path("b/d"));
+
+ Path b;
+ b.prepend("../a");
+ REQUIRE(b == Path("a"));
+ }
+
+ SECTION("concatenate") {
+ // concatenating a std::string to a path, directory traversal is applied
+ const Path a = Path("a/b/c") + "../../../d/e/f";
+ REQUIRE(a == Path("d/e/f"));
+
+ // here, the directory traversal components are lost before concatenating
+ const Path b = Path("a/b/c") + Path("../../../d/e/f");
+ REQUIRE(b == Path("a/b/c/d/e/f"));
+ }
+}
+
+TEST_CASE("append and prepend full paths") {
+ Path a;
+ a += Path("c/d");
+ a.append(Path("e/f"));
+ a.prepend(Path("a/b"));
+
+ REQUIRE(a == Path("a/b/c/d/e/f"));
+}
diff --git a/test/remote.cpp b/test/remote.cpp
@@ -31,7 +31,7 @@ TEST_CASE("construct invalid remote", M) {
SECTION("invalid name") {
try {
- Remote remote("/", "url");
+ Remote remote("a/", "url");
FAIL();
}
catch(const reapack_error &e) {
@@ -39,6 +39,24 @@ TEST_CASE("construct invalid remote", M) {
}
}
+ SECTION("directory traversal in name") {
+ try {
+ Remote remote("..", "url");
+ FAIL("dotdot was allowed");
+ }
+ catch(const reapack_error &e) {
+ REQUIRE(string(e.what()) == "invalid name");
+ }
+
+ try {
+ Remote remote(".", "url");
+ FAIL("single dot was allowed");
+ }
+ catch(const reapack_error &e) {
+ REQUIRE(string(e.what()) == "invalid name");
+ }
+ }
+
SECTION("empty url") {
try {
Remote remote("name", {});
diff --git a/test/source.cpp b/test/source.cpp
@@ -129,6 +129,23 @@ TEST_CASE("source target path", M) {
REQUIRE(source.targetPath() == expected);
}
+TEST_CASE("source target path with parent directory traversal", M) {
+ RemoteIndex ri("RemoteIndex Name");
+ Category cat("Category Name", &ri);
+ Package pack(Package::ScriptType, "package name", &cat);
+ Version ver("1.0", &pack);
+
+ const Source source(Source::GenericPlatform, "../../../file.name", "url", &ver);
+
+ Path expected;
+ expected.append("Scripts");
+ expected.append("RemoteIndex Name");
+ // expected.append("Category Name"); // only the category can be bypassed!
+ expected.append("file.name");
+
+ REQUIRE(source.targetPath() == expected);
+}
+
TEST_CASE("source target path without package", M) {
try {
const Source source(Source::GenericPlatform, "a", "b");
