From f8ad7aadcd2ce9aa87a41f8893b25ac42983a9d6 Mon Sep 17 00:00:00 2001 From: Kirill Efimov Date: Tue, 8 Feb 2022 15:23:45 +0200 Subject: [PATCH 1/4] prevent file upload write outside of the upload folder --- lib/src/HttpFileImpl.cc | 40 +++++++++++++++------- lib/tests/CMakeLists.txt | 2 ++ lib/tests/unittests/HttpFileTest.cc | 52 +++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 12 deletions(-) create mode 100644 lib/tests/unittests/HttpFileTest.cc diff --git a/lib/src/HttpFileImpl.cc b/lib/src/HttpFileImpl.cc index ad9c692a76..2f6196ddc6 100644 --- a/lib/src/HttpFileImpl.cc +++ b/lib/src/HttpFileImpl.cc @@ -18,6 +18,7 @@ #include #include #include +#include using namespace drogon; @@ -31,28 +32,43 @@ int HttpFileImpl::save(const std::string &path) const assert(!path.empty()); if (fileName_.empty()) return -1; - filesystem::path fsPath(utils::toNativePath(path)); - if (!fsPath.is_absolute() && - (!fsPath.has_parent_path() || - (fsPath.begin()->string() != "." && fsPath.begin()->string() != ".."))) + filesystem::path fsUploadDir(utils::toNativePath(path)); + + if (!fsUploadDir.is_absolute() && + (!fsUploadDir.has_parent_path() || + (fsUploadDir.begin()->string() != "." && + fsUploadDir.begin()->string() != ".."))) { - filesystem::path fsUploadPath(utils::toNativePath( - HttpAppFrameworkImpl::instance().getUploadPath())); - fsPath = fsUploadPath / fsPath; + fsUploadDir = utils::toNativePath( + HttpAppFrameworkImpl::instance().getUploadPath()) / fsUploadDir; } - filesystem::path fsFileName(utils::toNativePath(fileName_)); - if (!filesystem::exists(fsPath)) + + fsUploadDir = filesystem::weakly_canonical(fsUploadDir); + + if (!filesystem::exists(fsUploadDir)) { - LOG_TRACE << "create path:" << fsPath; + LOG_TRACE << "create path:" << fsUploadDir; drogon::error_code err; - filesystem::create_directories(fsPath, err); + filesystem::create_directories(fsUploadDir, err); if (err) { LOG_SYSERR; return -1; } } - return saveTo(fsPath / fsFileName); + + filesystem::path fsSaveToPath(filesystem::weakly_canonical( + fsUploadDir / utils::toNativePath(fileName_))); + + if (!std::equal(fsUploadDir.begin(), fsUploadDir.end(), fsSaveToPath.begin())) + { + LOG_ERROR + << "Attempt writing outside of upload directory detected. Path: " + << fileName_; + return -1; + } + + return saveTo(fsSaveToPath); } int HttpFileImpl::saveAs(const std::string &fileName) const { diff --git a/lib/tests/CMakeLists.txt b/lib/tests/CMakeLists.txt index 824d5b58b1..d2b9450f5c 100644 --- a/lib/tests/CMakeLists.txt +++ b/lib/tests/CMakeLists.txt @@ -7,6 +7,8 @@ set(UNITTEST_SOURCES unittests/main.cc unittests/CookieTest.cc unittests/ClassNameTest.cc unittests/HttpDateTest.cc + ../src/HttpFileImpl.cc + unittests/HttpFileTest.cc unittests/HttpHeaderTest.cc unittests/MD5Test.cc unittests/MsgBufferTest.cc diff --git a/lib/tests/unittests/HttpFileTest.cc b/lib/tests/unittests/HttpFileTest.cc new file mode 100644 index 0000000000..4295f4f84d --- /dev/null +++ b/lib/tests/unittests/HttpFileTest.cc @@ -0,0 +1,52 @@ +#include "../../lib/src/HttpFileImpl.h" +#include +#include + +using namespace drogon; +using namespace std; + +DROGON_TEST(HttpFile) +{ + SUBSECTION(Save) + { + HttpFileImpl file; + file.setFileName("test_file_name"); + file.setFile("test", 4); + auto out = file.save("./test_uploads_dir"); + + CHECK(out == 0); + CHECK(filesystem::exists("./test_uploads_dir/test_file_name")); + + filesystem::remove_all("./test_uploads_dir"); + } + + SUBSECTION(SavePathRelativeTraversal) + { + auto uploadPath = filesystem::current_path() / "test_uploads_dir"; + + HttpFileImpl file; + file.setFileName("../test_malicious_file_name"); + file.setFile("test", 4); + auto out = file.save(uploadPath); + + CHECK(out == -1); + CHECK(!filesystem::exists(uploadPath / "../test_malicious_file_name")); + + filesystem::remove_all(uploadPath); + filesystem::remove(uploadPath / "../test_malicious_file_name"); + } + + SUBSECTION(SavePathAbsoluteTraversal) + { + HttpFileImpl file; + file.setFileName("/tmp/test_malicious_file_name"); + file.setFile("test", 4); + auto out = file.save("./test_uploads_dir"); + + CHECK(out == -1); + CHECK(!filesystem::exists("/tmp/test_malicious_file_name")); + + filesystem::remove_all("test_uploads_dir"); + filesystem::remove_all("/tmp/test_malicious_file_name"); + } +} From 35da633a61fb47c4cf0c445049a52b15d09144bb Mon Sep 17 00:00:00 2001 From: an-tao Date: Tue, 8 Feb 2022 21:37:00 +0800 Subject: [PATCH 2/4] Fix lint --- lib/src/HttpFileImpl.cc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/src/HttpFileImpl.cc b/lib/src/HttpFileImpl.cc index 2f6196ddc6..faf9394246 100644 --- a/lib/src/HttpFileImpl.cc +++ b/lib/src/HttpFileImpl.cc @@ -34,13 +34,13 @@ int HttpFileImpl::save(const std::string &path) const return -1; filesystem::path fsUploadDir(utils::toNativePath(path)); - if (!fsUploadDir.is_absolute() && - (!fsUploadDir.has_parent_path() || - (fsUploadDir.begin()->string() != "." && - fsUploadDir.begin()->string() != ".."))) + if (!fsUploadDir.is_absolute() && (!fsUploadDir.has_parent_path() || + (fsUploadDir.begin()->string() != "." && + fsUploadDir.begin()->string() != ".."))) { fsUploadDir = utils::toNativePath( - HttpAppFrameworkImpl::instance().getUploadPath()) / fsUploadDir; + HttpAppFrameworkImpl::instance().getUploadPath()) / + fsUploadDir; } fsUploadDir = filesystem::weakly_canonical(fsUploadDir); @@ -60,7 +60,9 @@ int HttpFileImpl::save(const std::string &path) const filesystem::path fsSaveToPath(filesystem::weakly_canonical( fsUploadDir / utils::toNativePath(fileName_))); - if (!std::equal(fsUploadDir.begin(), fsUploadDir.end(), fsSaveToPath.begin())) + if (!std::equal(fsUploadDir.begin(), + fsUploadDir.end(), + fsSaveToPath.begin())) { LOG_ERROR << "Attempt writing outside of upload directory detected. Path: " From a370bb39dc2ac51abb960c579f56f04b63dd8b5d Mon Sep 17 00:00:00 2001 From: Kirill Date: Tue, 8 Feb 2022 16:47:43 +0200 Subject: [PATCH 3/4] fix C++14 issue --- lib/tests/unittests/HttpFileTest.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tests/unittests/HttpFileTest.cc b/lib/tests/unittests/HttpFileTest.cc index 4295f4f84d..d7bd50e5c9 100644 --- a/lib/tests/unittests/HttpFileTest.cc +++ b/lib/tests/unittests/HttpFileTest.cc @@ -27,7 +27,7 @@ DROGON_TEST(HttpFile) HttpFileImpl file; file.setFileName("../test_malicious_file_name"); file.setFile("test", 4); - auto out = file.save(uploadPath); + auto out = file.save(uploadPath.string()); CHECK(out == -1); CHECK(!filesystem::exists(uploadPath / "../test_malicious_file_name")); From 49f61448f0463fce68032c53e03bf6f85b7f3655 Mon Sep 17 00:00:00 2001 From: an-tao Date: Wed, 9 Feb 2022 00:13:35 +0800 Subject: [PATCH 4/4] Test HttpFileImpl on static building --- lib/tests/CMakeLists.txt | 80 ++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 39 deletions(-) diff --git a/lib/tests/CMakeLists.txt b/lib/tests/CMakeLists.txt index d2b9450f5c..79409527c0 100644 --- a/lib/tests/CMakeLists.txt +++ b/lib/tests/CMakeLists.txt @@ -1,47 +1,49 @@ link_libraries(${PROJECT_NAME}) -set(UNITTEST_SOURCES unittests/main.cc - unittests/Base64Test.cc - unittests/UrlCodecTest.cc - unittests/GzipTest.cc - unittests/HttpViewDataTest.cc - unittests/CookieTest.cc - unittests/ClassNameTest.cc - unittests/HttpDateTest.cc - ../src/HttpFileImpl.cc - unittests/HttpFileTest.cc - unittests/HttpHeaderTest.cc - unittests/MD5Test.cc - unittests/MsgBufferTest.cc - unittests/OStringStreamTest.cc - unittests/PubSubServiceUnittest.cc - unittests/Sha1Test.cc - ../src/ssl_funcs/Sha1.cc - ../src/HttpUtils.cc - unittests/FileTypeTest.cc - unittests/DrObjectTest.cc - unittests/HttpFullDateTest.cc - unittests/MainLoopTest.cc - unittests/CacheMapTest.cc - unittests/StringOpsTest.cc) +set(UNITTEST_SOURCES + unittests/main.cc + unittests/Base64Test.cc + unittests/UrlCodecTest.cc + unittests/GzipTest.cc + unittests/HttpViewDataTest.cc + unittests/CookieTest.cc + unittests/ClassNameTest.cc + unittests/HttpDateTest.cc + unittests/HttpHeaderTest.cc + unittests/MD5Test.cc + unittests/MsgBufferTest.cc + unittests/OStringStreamTest.cc + unittests/PubSubServiceUnittest.cc + unittests/Sha1Test.cc + ../src/ssl_funcs/Sha1.cc + unittests/FileTypeTest.cc + unittests/DrObjectTest.cc + unittests/HttpFullDateTest.cc + unittests/MainLoopTest.cc + unittests/CacheMapTest.cc + unittests/StringOpsTest.cc) if(DROGON_CXX_STANDARD GREATER_EQUAL 20 AND HAS_COROUTINE) - set(UNITTEST_SOURCES ${UNITTEST_SOURCES} unittests/CoroutineTest.cc) + set(UNITTEST_SOURCES ${UNITTEST_SOURCES} unittests/CoroutineTest.cc) endif() if(Brotli_FOUND) - set(UNITTEST_SOURCES ${UNITTEST_SOURCES} unittests/BrotliTest.cc) + set(UNITTEST_SOURCES ${UNITTEST_SOURCES} unittests/BrotliTest.cc) endif() -if (CMAKE_CXX_COMPILER_ID MATCHES "MSVC" AND BUILD_DROGON_SHARED) - set(UNITTEST_SOURCES ${UNITTEST_SOURCES} ../src/HttpUtils.cc) +if(CMAKE_CXX_COMPILER_ID MATCHES "MSVC" AND BUILD_DROGON_SHARED) + set(UNITTEST_SOURCES ${UNITTEST_SOURCES} ../src/HttpUtils.cc) +else() + set(UNITTEST_SOURCES ${UNITTEST_SOURCES} ../src/HttpFileImpl.cc + unittests/HttpFileTest.cc) endif() add_executable(unittest ${UNITTEST_SOURCES}) -set(INTEGRATION_TEST_CLIENT_SOURCES integration_test/client/main.cc - integration_test/client/WebSocketTest.cc - integration_test/client/MultipleWsTest.cc - integration_test/client/HttpPipeliningTest.cc) +set(INTEGRATION_TEST_CLIENT_SOURCES + integration_test/client/main.cc + integration_test/client/WebSocketTest.cc + integration_test/client/MultipleWsTest.cc + integration_test/client/HttpPipeliningTest.cc) add_executable(integration_test_client ${INTEGRATION_TEST_CLIENT_SOURCES}) set(INTEGRATION_TEST_SERVER_SOURCES @@ -66,10 +68,11 @@ set(INTEGRATION_TEST_SERVER_SOURCES integration_test/server/main.cc) if(DROGON_CXX_STANDARD GREATER_EQUAL 20 AND HAS_COROUTINE) - set(INTEGRATION_TEST_SERVER_SOURCES ${INTEGRATION_TEST_SERVER_SOURCES} - integration_test/server/api_v1_CoroTest.cc) - set(CMAKE_CXX_STANDARD 20) - set(CMAKE_CXX_STANDARD_REQUIRED TRUE) + set(INTEGRATION_TEST_SERVER_SOURCES + ${INTEGRATION_TEST_SERVER_SOURCES} + integration_test/server/api_v1_CoroTest.cc) + set(CMAKE_CXX_STANDARD 20) + set(CMAKE_CXX_STANDARD_REQUIRED TRUE) endif(DROGON_CXX_STANDARD GREATER_EQUAL 20 AND HAS_COROUTINE) add_executable(integration_test_server ${INTEGRATION_TEST_SERVER_SOURCES}) @@ -100,9 +103,8 @@ add_custom_command( $/a-directory) set(tests unittest integration_test_server integration_test_client) -set_property(TARGET ${tests} - PROPERTY CXX_STANDARD ${DROGON_CXX_STANDARD}) +set_property(TARGET ${tests} PROPERTY CXX_STANDARD ${DROGON_CXX_STANDARD}) set_property(TARGET ${tests} PROPERTY CXX_STANDARD_REQUIRED ON) set_property(TARGET ${tests} PROPERTY CXX_EXTENSIONS OFF) -ParseAndAddDrogonTests(unittest) +parseandadddrogontests(unittest)