Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent malformed upload path causing arbitrary write #1174

Merged
merged 4 commits into from
Feb 11, 2022
Merged

Prevent malformed upload path causing arbitrary write #1174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f8ad7aa
prevent file upload write outside of the upload folder
Kirill89 Feb 8, 2022
35da633
Fix lint
an-tao Feb 8, 2022
a370bb3
fix C++14 issue
Kirill89 Feb 8, 2022
49f6144
Test HttpFileImpl on static building
an-tao Feb 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 30 additions & 12 deletions lib/src/HttpFileImpl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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");
}
}