Fix path traversal via double percent-encoded slash bypassing directory containment - #154
Open
iaohkut-from-NightWolf-Team wants to merge 1 commit into
Conversation
…ry containment (CWE-22) new URL()'s dot-segment removal neutralizes literal ../ and %2e%2e/ sequences, but if the path separator itself is also percent-encoded (%2f), the whole segment survives new URL() untouched as one opaque path component. A later decodeURIComponent() call then materializes a real ../ traversal after the URL layer's own normalization already ran, reaching isFileInDirectory()'s naive prefix check and escaping this.dir via a same-prefix sibling directory (e.g. `_site-leak` next to `_site`). Reject any literal `..` path segment that appears only after percent-decoding, closing the bypass at its actual point of origin without touching isFileInDirectory() itself (which a separate, unrelated feature — the `directory.html` sibling-of-directory URL resolution — intentionally relies on for its own same-prefix matching behavior; changing that function's semantics broke 6 existing tests when tried, hence the more targeted fix here). Verified against the existing test suite (32/32 passing) plus a PoC that previously read a file from a sibling directory outside the served root. Co-Authored-By: iaohkut <thb2601@gmail.com>
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.
Fixes #152.
Summary
new URL()'s dot-segment removal neutralizes literal../and%2e%2e/sequences, but if the path separator itself is also percent-encoded (%2f), the whole segment survivesnew URL()untouched as one opaque path component. A laterdecodeURIComponent()call then materializes a real../traversal after the URL layer's own normalization already ran, reachingisFileInDirectory()'s naive prefix check and escapingthis.dirvia a same-prefix sibling directory (e.g._site-leaknext to_site).Fix
Reject any literal
..path segment that appears only after percent-decoding, closing the bypass at its actual point of origin:Note on scope: I initially tried fixing
isFileInDirectory()'s prefix-boundary check directly, but that function is also intentionally relied on (via the same prefix-matching behavior) by the unrelateddirectory.html-as-sibling-of-directory/URL resolution feature — changing its semantics broke 6 existing tests. This targeted fix closes the actual exploit mechanism (the post-decode traversal segment) without touching that other feature.Verification
See #152 for the full report and original reproduction (note: the original end-to-end curl reproduction there also demonstrates the separate unhandled-exception crash from #150 — this PR and that one are complementary and both needed for full robustness).