fix: make staging path-traversal check separator-aware (fail-open on Windows) - #422
Open
mani-mathur-arch wants to merge 2 commits into
Open
fix: make staging path-traversal check separator-aware (fail-open on Windows)#422mani-mathur-arch wants to merge 2 commits into
mani-mathur-arch wants to merge 2 commits into
Conversation
localPathIsAllowed guarded against staging PUT/GET escaping the configured stagingAllowedLocalPaths by rejecting a relative path containing the literal "../". filepath.Rel returns OS-native separators, so on Windows an escaping path is "..\\x", which the "../" substring never matches — the guard returned true (allowed) for a path outside the allowlist. This is a fail-open path-traversal check on Windows, present since the driver's first commit. Compare the relative path against filepath.Separator instead: a path is inside the allowed dir iff it is neither ".." nor starts with ".."+separator. Using a prefix check (not Contains) also stops a sibling like "..foo" from being mistaken for an escape. Only the default Thrift path reaches this code; the kernel/SEA backend rejects staging statements before execStagingOperation. Surfaced by the new macOS/Windows CI matrix — the existing TestPathAllowed now passes on Windows. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
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
1 participant
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.
What
localPathIsAllowed(connection.go) guards stagingPUT/GETagainst a local file escaping the configuredstagingAllowedLocalPaths. It did so by rejecting a relative path containing the literal"../":filepath.Relreturns OS-native separators, so on Windows an escaping path is..\x, which the"../"substring never matches. The guard therefore returnstrue(allowed) for a path outside the allowlist — a fail-open path-traversal check on Windows.Fix
Compare against
filepath.Separatorinstead: a path is inside the allowed dir iff it is neither".."nor starts with".." + separator. Using a prefix check (notContains) also stops a sibling like..foofrom being mistaken for an escape.Scope
Only the default Thrift path reaches this code — the kernel/SEA backend rejects staging statements before
execStagingOperation, so it never calls this guard.History
Present since the driver's first commit (Sept 2023) — not a recent regression. The check has always used the hard-coded
"../"; it was simply never exercised on Windows because CI was Linux-only.Testing
The existing
TestPathAllowed/Should_not_allow_paths_that_don't_share_directoryasserts the escaping path is disallowed. It passed on Linux/macOS and failed on Windows before this change; with the fix it passes on all three. Verified green on the macOS + Windows CI matrix (see #421, which surfaced this).This pull request and its description were written by Isaac.