Skip to content
Permalink
Browse files Browse the repository at this point in the history
Prevent malformed upload path causing arbitrary write (#1174)
  • Loading branch information
Kirill89 committed Feb 11, 2022
1 parent 8ed0434 commit 3c78532
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 49 deletions.
42 changes: 30 additions & 12 deletions lib/src/HttpFileImpl.cc
Expand Up @@ -18,6 +18,7 @@
#include <drogon/MultiPart.h>
#include <fstream>
#include <iostream>
#include <algorithm>

using namespace drogon;

Expand All @@ -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
{
Expand Down
78 changes: 41 additions & 37 deletions 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
Expand All @@ -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})
Expand Down Expand Up @@ -98,9 +103,8 @@ add_custom_command(
$<TARGET_FILE_DIR:integration_test_server>/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)
52 changes: 52 additions & 0 deletions lib/tests/unittests/HttpFileTest.cc
@@ -0,0 +1,52 @@
#include "../../lib/src/HttpFileImpl.h"
#include <drogon/drogon_test.h>
#include <filesystem>

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");
}
}

0 comments on commit 3c78532

Please sign in to comment.