CVE-2022-25297: Files or Directories Accessible to External Parties (CWE-552)
value: This affects the package drogonframework/drogon before 1.7.5. The unsafe handling of file names during upload using HttpFile::save() method may enable attackers to write files to arbitrary locations outside the designated target folder.
CWE description: The product makes files or directories accessible to unauthorized actors, even though they should not be.
CVSS v3 base score: 8.8
Published: 2022-02-21T08:15Z
Repository: https://github.com/drogonframework/drogon
Language: C++
Files changed: lib/src/HttpFileImpl.cc, lib/tests/CMakeLists.txt, lib/tests/unittests/HttpFileTest.cc
Commit date: 2022-02-11 23:54:42 +0800
Commit message: Prevent malformed upload path causing arbitrary write (#1174)


Diff: diff --git a/lib/src/HttpFileImpl.cc b/lib/src/HttpFileImpl.cc
index ad9c692..faf9394 100644
--- a/lib/src/HttpFileImpl.cc
+++ b/lib/src/HttpFileImpl.cc
@@ -16,10 +16,11 @@
 #include "HttpAppFrameworkImpl.h"
 #include "filesystem.h"
 #include <drogon/MultiPart.h>
 #include <fstream>
 #include <iostream>
+#include <algorithm>
 
 using namespace drogon;
 
 int HttpFileImpl::save() const
 {
@@ -29,32 +30,49 @@ int HttpFileImpl::save() const
 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
 {
     assert(!fileName.empty());
     filesystem::path fsFileName(utils::toNativePath(fileName));
diff --git a/lib/tests/CMakeLists.txt b/lib/tests/CMakeLists.txt
index 824d5b5..7940952 100644
--- a/lib/tests/CMakeLists.txt
+++ b/lib/tests/CMakeLists.txt
@@ -1,47 +1,51 @@
 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
     integration_test/server/CustomCtrl.cc
     integration_test/server/CustomHeaderFilter.cc
@@ -62,14 +66,15 @@ set(INTEGRATION_TEST_SERVER_SOURCES
     integration_test/server/RangeTestController.cc
     integration_test/server/BeginAdviceTest.cc
     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})
 drogon_create_views(integration_test_server
                     ${CMAKE_CURRENT_SOURCE_DIR}/integration_test/server
@@ -96,11 +101,10 @@ add_custom_command(
           copy_directory
           ${CMAKE_CURRENT_SOURCE_DIR}/integration_test/server/a-directory
           $<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)
diff --git a/lib/tests/unittests/HttpFileTest.cc b/lib/tests/unittests/HttpFileTest.cc
new file mode 100644
index 0000000..d7bd50e
--- /dev/null
+++ b/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");
+    }
+}

Vulnerable code:     filesystem::path fsPath(utils::toNativePath(path));
    if (!fsPath.is_absolute() &&
        (!fsPath.has_parent_path() ||
         (fsPath.begin()->string() != "." && fsPath.begin()->string() != "..")))
        filesystem::path fsUploadPath(utils::toNativePath(
            HttpAppFrameworkImpl::instance().getUploadPath()));
        fsPath = fsUploadPath / fsPath;
    filesystem::path fsFileName(utils::toNativePath(fileName_));
    if (!filesystem::exists(fsPath))
        LOG_TRACE << "create path:" << fsPath;
        filesystem::create_directories(fsPath, err);
    return saveTo(fsPath / fsFileName);
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 ${UNITTEST_SOURCES} unittests/CoroutineTest.cc)
    set(UNITTEST_SOURCES ${UNITTEST_SOURCES} unittests/BrotliTest.cc)
if (CMAKE_CXX_COMPILER_ID MATCHES "MSVC" AND BUILD_DROGON_SHARED)
    set(UNITTEST_SOURCES ${UNITTEST_SOURCES} ../src/HttpUtils.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)
    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_property(TARGET ${tests}
             PROPERTY CXX_STANDARD ${DROGON_CXX_STANDARD})
ParseAndAddDrogonTests(unittest)
Fixed code: #include <algorithm>
    filesystem::path fsUploadDir(utils::toNativePath(path));

    if (!fsUploadDir.is_absolute() && (!fsUploadDir.has_parent_path() ||
                                       (fsUploadDir.begin()->string() != "." &&
                                        fsUploadDir.begin()->string() != "..")))
        fsUploadDir = utils::toNativePath(
                          HttpAppFrameworkImpl::instance().getUploadPath()) /
                      fsUploadDir;

    fsUploadDir = filesystem::weakly_canonical(fsUploadDir);

    if (!filesystem::exists(fsUploadDir))
        LOG_TRACE << "create path:" << fsUploadDir;
        filesystem::create_directories(fsUploadDir, err);

    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);
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)
  set(UNITTEST_SOURCES ${UNITTEST_SOURCES} unittests/CoroutineTest.cc)
  set(UNITTEST_SOURCES ${UNITTEST_SOURCES} unittests/BrotliTest.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)
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_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_property(TARGET ${tests} PROPERTY CXX_STANDARD ${DROGON_CXX_STANDARD})
parseandadddrogontests(unittest)
#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");
    }
}