Skip to content

Fix path fence bypass for relative paths with leading ".."#745

Merged
garydgregory merged 3 commits into
apache:masterfrom
dxbjavid:fix-pathfence-traversal
Jun 1, 2026
Merged

Fix path fence bypass for relative paths with leading ".."#745
garydgregory merged 3 commits into
apache:masterfrom
dxbjavid:fix-pathfence-traversal

Conversation

@dxbjavid
Copy link
Copy Markdown
Contributor

@dxbjavid dxbjavid commented Jun 1, 2026

PathFence.apply() calls path.normalize().toAbsolutePath(), so leading .. segments survive normalize() and toAbsolutePath() then prepends the working directory. The component-wise startsWith(root) check matches the prefix even though the real path escapes the fence, e.g. ${file:UTF-8:../secret} reads outside a fence rooted at the working directory. Resolve to absolute first, then normalize, for both the roots and the candidate path.

@garydgregory
Copy link
Copy Markdown
Member

@dxbjavid
Thank you for the PR.
You must provide a unit test that fails when the changes to main are not applied. Otherwise, this is a regression waiting to happen.

@dxbjavid
Copy link
Copy Markdown
Contributor Author

dxbjavid commented Jun 1, 2026

Added one. The existing ../ tests only passed because the escaped file didn't exist, so the read failed and got rethrown as IllegalArgumentException anyway. The new test in FileStringLookupTest points a relative .. path at a real file outside the fence: on current main the fence lets it through and the file is read (nothing thrown), with the fix it's rejected.

@garydgregory
Copy link
Copy Markdown
Member

Hello @dxbjavid
Thank you for your prompt update.
I can see the new test failing now without the main changes; BUT the changes to the constructor are not needed for the test to pass.
Would you please add another test to validate the change to the constructor?
TY!

@dxbjavid
Copy link
Copy Markdown
Contributor Author

dxbjavid commented Jun 1, 2026

Added testFenceRootWithParentSegment. It builds a fence rooted at target/.. (which resolves to the working dir) and reads an in-fence file. Without the constructor normalize the root keeps the trailing .., so the component-wise startsWith never matches and the legit file is wrongly rejected. The test fails on main and passes with the fix.

@garydgregory garydgregory changed the title fix path fence bypass for relative paths with leading .. Fix path fence bypass for relative paths with leading .. Jun 1, 2026
@garydgregory garydgregory changed the title Fix path fence bypass for relative paths with leading .. Fix path fence bypass for relative paths with leading ".." Jun 1, 2026
@garydgregory garydgregory merged commit c68fe3c into apache:master Jun 1, 2026
9 checks passed
@garydgregory
Copy link
Copy Markdown
Member

@dxbjavid
Thank you, merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants