Repository navigation
[release-7.4] BulkLoad: reject path-traversal in manifest file paths (#13665) - #14334
Open
ShreeChaturvedi wants to merge 1 commit into
Open
ShreeChaturvedi wants to merge 1 commit into
ShreeChaturvedi wants to merge 1 commit into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
neethuhaneesha
approved these changes
Oct 21, 2026
…pple#13665) * BulkLoad: reject path-traversal in manifest file paths A crafted bulk-load manifest could set [RelativePath], [DataFileName], or [ByteSampleFileName] to an absolute path or one containing ".." components. getFolder() joins these onto the local staging root and the result flows into abspath() -> eraseDirectoryRecursive() (and subsequent file writes), so a manifest such as "[RelativePath]: ../../../../var/lib/foundationdb" could make every storage server running the job wipe and overwrite an arbitrary directory (e.g. the FDB data directory or TLS material). Add two layers of validation: - Parse/construction time: BulkLoadFileSet::isValid() now rejects relativePath, manifestFileName, dataFileName, and byteSampleFileName that are absolute or contain a ".." component (bulkLoadPathIsContained). Bad manifests are refused before any local path is constructed. - Sink guard: assertLocalFolderContained() verifies the resolved staging folder is at or under the resolved local root immediately before resetFileFolder(), in both bulkLoadTransportCP_impl and bulkLoadTransportBlobstore_impl. Add unit test /fdbserver/BulkLoad/bulkLoadPathIsContained. * Formatting * BulkLoad: use std::string::starts_with in containment check Address review feedback: C++20 starts_with() is clearer than the size()+compare() prefix check. (cherry picked from commit 8f09b3c)
spraza
force-pushed
the
bulkload-path-traversal-7.4
branch
from
October 26, 2026 17:41
31b1d6b to
f5f2ac5
Compare
Collaborator
|
Change looks isolated to bulk load, so ok for 7.4 backport. Accepting. @ShreeChaturvedi triggered rebase. Once CI is clear, feel free to merge. |
spraza
approved these changes
Oct 26, 2026
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Result of foundationdb-pr-clang on Linux RHEL 9
|
Contributor
Result of foundationdb-pr on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
This comment has been minimized.
This comment has been minimized.
Contributor
Result of foundationdb-pr-macos on macOS 14.x
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #13665 to release-7.4. release-8.0 already contains it.
A crafted bulk-load manifest could set
[RelativePath],[DataFileName], or[ByteSampleFileName]to an absolute path or one containing.., drivingresetFileFolder()→eraseDirectoryRecursive()and file writes outside the storage server's local staging root. The fix rejects such fields inBulkLoadFileSet::isValid()and adds a containment check right before the staging folder is reset in both the CP and blobstore transport paths.The
BulkLoading.hchange applied cleanly. Thefdbserver/core/BulkLoadUtil.cpphunk conflicted only because release-7.4 has this code infdbserver/BulkLoadUtil.actor.cpp; the same code was applied there, with theflow/UnitTest.hinclude placed beforeactorcompiler.h. No functional differences from main.Validation:
fdbserverbuilds (clang,USE_WERROR=ON).fdbserver -r unittests -f /fdbserver/BulkLoad/bulkLoadPathIsContainedpasses.tests/fast/BulkLoading.tomlandtests/fast/BulkDumping.tomlpass; no SevError, and the new guard never fires on normal jobs.20261020-222819-shreechaturvedi-cb016c5149db79d4: 100,000/100,000 passed, 0 failed.