diff --git a/lib/src/HttpFileImpl.cc b/lib/src/HttpFileImpl.cc index ad9c692a76..faf9394246 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,45 @@ 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..79409527c0 100644 --- a/lib/tests/CMakeLists.txt +++ b/lib/tests/CMakeLists.txt @@ -1,45 +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 - 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 @@ -64,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}) @@ -98,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) diff --git a/lib/tests/unittests/HttpFileTest.cc b/lib/tests/unittests/HttpFileTest.cc new file mode 100644 index 0000000000..d7bd50e5c9 --- /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.string()); + + 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"); + } +}