From b937b78aceaf6f6239068070ab622e7f8742d8ec Mon Sep 17 00:00:00 2001 From: ahamirwasia Date: Tue, 2 Oct 2018 23:18:49 -0700 Subject: [PATCH 1/2] Fix security issue (#61) --- luna/router.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/luna/router.cpp b/luna/router.cpp index 3e4d947..cf67ef6 100644 --- a/luna/router.cpp +++ b/luna/router.cpp @@ -63,7 +63,10 @@ void router::handle_request(request_method method, } void router::serve_files(std::string mount_point, std::string path_to_files) -{ +{ + std::regex parent_dir_pattern("(../)+"); + path_to_files = std::regex_replace(path_to_files, parent_dir_pattern, ""); + std::regex route{mount_point + "(.*)"}; std::string local_path{path_to_files + "/"}; handle_request(request_method::GET, route, [=](const request &req) -> response From f4f7853c2da35b3e82250e9cd629e80d1fb3d575 Mon Sep 17 00:00:00 2001 From: ahamirwasia Date: Thu, 4 Oct 2018 22:40:04 -0700 Subject: [PATCH 2/2] Fix Security issue --- luna/router.cpp | 80 ++++++++++++++++++++++++++++++++++++++++-- luna/router.h | 2 ++ tests/file_service.cpp | 57 ++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 3 deletions(-) diff --git a/luna/router.cpp b/luna/router.cpp index cf67ef6..d10d413 100644 --- a/luna/router.cpp +++ b/luna/router.cpp @@ -17,6 +17,9 @@ #include "luna/config.h" #include "server.h" #include +#include +#include +#include namespace luna { @@ -62,11 +65,82 @@ void router::handle_request(request_method method, validations)); } +std::string router::sanitize_path(std::string path_to_files) +{ + std::vector url_comps; + std::string delimeter = "/"; + std::string final_path = ""; + std::string working_path = path_to_files; + size_t pos = 0; + int num_front_slash = 0, num_back_slash = 0; + + // path can contain many / as prefix or suffix + // we want to preserve that in the final path + while (pos < path_to_files.size() and path_to_files[pos++] == '/') { + ++num_front_slash; + } + + pos = path_to_files.size() - 1; + while (int(pos) - 1 >= 0 and path_to_files[pos--] == '/') { + ++num_back_slash; + } + + // Now, split by '/' to get URL components + while ((pos = working_path.find(delimeter)) != std::string::npos) { + std::string comp = working_path.substr(0, pos); + if (not comp.empty()) { + url_comps.push_back(comp); + } + working_path.erase(0, pos + delimeter.length()); + } + + std::string last_comp = working_path.substr(0, pos); + if (not last_comp.empty()) { + url_comps.push_back(last_comp); + } + + if (url_comps.empty()) { + // nothing to sanitize + return path_to_files; + } + + std::stack stk; + for (auto& comp : url_comps) { + if (comp == ".." and stk.empty()) { + // trying to access a file outside cwd + // Return empty string which would result in 404 + return ""; + } else if (comp == "..") { + stk.pop(); + } else { + stk.push(comp); + } + } + + // Build final path from stack + while (not stk.empty()) { + final_path = stk.top() + delimeter + final_path; + stk.pop(); + } + + // remove trailing '/' added by previous stmt + final_path.pop_back(); + + // add back the /s in + while (num_front_slash--) { + final_path = "/" + final_path; + } + + while (num_back_slash--) { + final_path += "/"; + } + + return final_path; +} + void router::serve_files(std::string mount_point, std::string path_to_files) { - std::regex parent_dir_pattern("(../)+"); - path_to_files = std::regex_replace(path_to_files, parent_dir_pattern, ""); - + path_to_files = router::sanitize_path(path_to_files); std::regex route{mount_point + "(.*)"}; std::string local_path{path_to_files + "/"}; handle_request(request_method::GET, route, [=](const request &req) -> response diff --git a/luna/router.h b/luna/router.h index ea9e988..891512b 100644 --- a/luna/router.h +++ b/luna/router.h @@ -43,6 +43,8 @@ class router endpoint_handler_cb callback, parameter::validators validations = {}); + std::string sanitize_path(std::string path_to_files); + void serve_files(std::string mount_point, std::string path_to_files); void add_header(std::string &&key, std::string &&value); diff --git a/tests/file_service.cpp b/tests/file_service.cpp index 61d5e70..27e3a80 100644 --- a/tests/file_service.cpp +++ b/tests/file_service.cpp @@ -34,6 +34,63 @@ TEST(file_service, serve_file_404) ASSERT_EQ(404, res.status_code); } + +TEST(file_service, serve_file_malicious) +{ + luna::server server; + auto router = server.create_router("/"); + + // ** Invalid cases ** + std::string path {"../../foo/confidential.txt"}; + path = router->sanitize_path(path); + ASSERT_TRUE(path == ""); + + path = "foo/bar/../../../confidential.txt"; + path = router->sanitize_path(path); + ASSERT_TRUE(path == ""); + + + path = "/foo/bar/../../../confidential.txt"; + path = router->sanitize_path(path); + ASSERT_TRUE(path == ""); + + + path = "foo/bar/../../../confidential.txt/"; + path = router->sanitize_path(path); + ASSERT_TRUE(path == ""); + + path = "/foo/bar/../../../confidential.txt/"; + path = router->sanitize_path(path); + ASSERT_TRUE(path == ""); + + // ** Valid cases ** + path = "/foo/bar/../../test.txt"; + path = router->sanitize_path(path); + ASSERT_TRUE(path == "/test.txt"); + + // check if path was cleaned + path = "/////////foo/bar/../../test.txt"; + path = router->sanitize_path(path); + ASSERT_TRUE(path == "/////////test.txt"); + + path = "foo/bar/../../test.txt/"; + path = router->sanitize_path(path); + ASSERT_TRUE(path == "test.txt/"); + + path = "/foo/bar/../../test.txt/"; + path = router->sanitize_path(path); + ASSERT_TRUE(path == "/test.txt/"); + + path = "/foo/bar/../test.txt"; + path = router->sanitize_path(path); + ASSERT_TRUE(path == "/foo/test.txt"); + + // check if path was unchanged + path = "foo/bar/test.txt"; + path = router->sanitize_path(path); + ASSERT_TRUE(path == "foo/bar/test.txt"); +} + TEST(file_service, serve_text_file) { std::string path{STATIC_ASSET_PATH};