[shadye-6]

Summary

Fixes #2655

In HttpFileImpl::save(const std::string &path) (lib/src/HttpFileImpl.cc), destination path validation previously used the three-iterator overload of std::equal:

if (!std::equal(fsUploadDir.begin(),
                fsUploadDir.end(),
                fsSaveToPath.begin()))

Under ISO C++17 [alg.equal], three-iterator std::equal requires that the second range (fsSaveToPath) contains at least as many elements as the first range (fsUploadDir). When an untrusted multipart upload filename resolves via std::filesystem::weakly_canonical to an ancestor directory (such as ../../c when uploading to .../a/b/c/upload, or ..), fsSaveToPath contains fewer path components than fsUploadDir.

When all components of the shorter destination path match the corresponding upload-directory components, std::equal attempts to dereference the second iterator after it has reached fsSaveToPath.end(). This violates the algorithm's range precondition and results in undefined behavior. Under standard library assertion modes (-D_GLIBCXX_ASSERTIONS), this consistently triggers a process abort inside the noexcept method:

/usr/include/c++/13/bits/fs_path.h:1398: const std::filesystem::__cxx11::path& std::filesystem::__cxx11::path::iterator::operator*() const: Assertion '_M_cur != _M_path->_M_cmpts.end()' failed.
Aborted (core dumped)

---

Solution

Replaced the three-iterator std::equal with the four-iterator overload of std::mismatch:

auto mismatch = std::mismatch(fsUploadDir.begin(),
                              fsUploadDir.end(),
                              fsSaveToPath.begin(),
                              fsSaveToPath.end());
if (mismatch.first != fsUploadDir.end())
{
    LOG_ERROR
        << "Attempt writing outside of upload directory detected. Path: "
        << fileName_;
    return -1;
}

Unlike four-iterator std::equal (which enforces strict equality of range lengths and would reject legitimate uploads to subdirectories), std::mismatch ensures fsUploadDir is a valid prefix of fsSaveToPath while safely handling shorter, equal, or longer destination paths without out-of-bounds access.

---

Additions & Tests Added

Added 3 regression sub-tests to lib/tests/unittests/HttpFileTest.cc:

1. FileNameWithStrictPrefixTraversal: Sets upload path to .../test_uploads_dir/a/b/c/upload and filename to ../../c. The canonical destination .../test_uploads_dir/a/b/c has fewer components while matching every prefix component. This reproduces the abort on the unpatched codebase under -D_GLIBCXX_ASSERTIONS and safely returns -1 with the patch applied.
2. FileNameWithDotDotTraversal: Verifies that single .. traversal is safely rejected (return -1).
3. FileNameWithNestedSubdirectory: Verifies that legitimate uploads into nested subdirectories (nested/test_nested_file) continue to succeed (return 0).

---

Verification & Checks

• Regression test reproduces the abort on the original implementation with -D_GLIBCXX_ASSERTIONS.
• Patched implementation safely rejects traversal paths (-1) without aborting.
• Valid nested upload succeeds (0).
• Full unit test suite: 73 test cases and 1,420 assertions passed.
• Modified files formatted using the repository .clang-format configuration (note: root format.sh enforces clang-format version 17; modified files were verified against .clang-format with zero diffs).

[an-tao]

@shadye-6 Thanks for the fix and the regression tests. The std::mismatch change looks correct to me: it avoids dereferencing past fsSaveToPath.end() while still requiring fsUploadDir to be a prefix of the destination path.
Two things before this can move forward:

1. This PR currently has merge conflicts with master. Could you rebase it?
2. Bolk audit and framework hardning #2659 also mentions fixing an out-of-bounds path comparison in upload saving. Could you check whether it touches the same code in HttpFileImpl::save()? If it does, please confirm whether the two fixes overlap or conflict, so we can keep only one version and avoid a duplicate change.
   If Bolk audit and framework hardning #2659 does not cover this case, please keep this PR and rebase it on top of the latest master.

[shadye-6]

I checked the files changed of PR #2659. It modifies the same path-validation logic in HttpFileImpl::save(), replacing the three-iterator std::equal with the four-iterator std::mismatch approach. The two fixes are functionally equivalent and overlap, so I agree we should go with one PR.
My PR #2656 additionally includes three regression tests covering:

• Strict-prefix traversal (../../c)
• Single .. traversal
• Valid nested-directory uploads
  Would you rather I close Fix out-of-bounds iterator dereference in HttpFileImpl::save() (#2655) #2656 and have these regression tests incorporated into Bolk audit and framework hardning #2659?

[an-tao]

I checked the files changed of PR #2659. It modifies the same path-validation logic in HttpFileImpl::save(), replacing the three-iterator std::equal with the four-iterator std::mismatch approach. The two fixes are functionally equivalent and overlap, so I agree we should go with one PR. My PR #2656 additionally includes three regression tests covering:

• Strict-prefix traversal (../../c)
• Single .. traversal
• Valid nested-directory uploads
  Would you rather I close Fix out-of-bounds iterator dereference in HttpFileImpl::save() (#2655) #2656 and have these regression tests incorporated into Bolk audit and framework hardning #2659?

Please rebase from master branch and keep these tests, thanks so much.

[shadye-6]

@an-tao after rebasing i encountered a Windows MSVC static CI timeout in HttpFileExceptionSafety, for which i tried debugging the root cause.
i have closed the PR now, do let me know if you want me to open a new PR clean with the regression cases only. Thank you.