Fix predictable marker collision in EncodingPatternPreservation#914
Open
Socialpranker wants to merge 1 commit into
Open
Fix predictable marker collision in EncodingPatternPreservation#914Socialpranker wants to merge 1 commit into
Socialpranker wants to merge 1 commit into
Conversation
EncodingPatternPreservation.captureAndReplaceMatches() previously used a fixed, publicly-known default marker (this class's simple name, "EncodingPatternPreservation") to temporarily stand in for matched content while the rest of the string is encoded. restoreOriginalContent() restores captured content in FIFO order using replaceFirst(marker, ...), matching the marker as plain literal text. If the input already contains that literal string ahead of the real matched content (e.g. CSSCodec.encode() on "EncodingPatternPreservation background:rgb(1,2,3)"), replaceFirst matches the attacker-supplied text instead of the real placeholder, silently desynchronizing every subsequent restoration and corrupting the encoded output. Fix: derive the default marker per-instance from a random UUID (with hyphens stripped, since a hyphen would itself be altered by an encoding pass such as CSSCodec's before restoration), so it cannot be predicted or embedded by input content. The public setReplacementMarker() API is unchanged. Added regression tests covering marker unpredictability and the literal-marker-in-input desync scenario.
c80d785 to
cd45dd2
Compare
Author
|
Filed the underlying bug as #915 with repro details, per the issue → PR workflow described in CONTRIBUTING-TO-ESAPI.txt. |
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.
Summary
EncodingPatternPreservation(used byCSSCodecto protectrgb(...)triplets from CSS-escaping duringencodeForCSS) previously used a fixed, publicly-known default marker: this class's own simple name,"EncodingPatternPreservation".captureAndReplaceMatches()temporarily swaps matched content for this marker;restoreOriginalContent()restores it afterwards usingreplaceFirst(marker, ...), matching the marker as plain literal text, in FIFO order.Because the marker is a known constant, input that already contains that literal string ahead of real matched content causes
replaceFirstto match the attacker-supplied text instead of the real placeholder. Every subsequent restoration in the same call is then shifted by one, silently corrupting the output — restored content ends up in the wrong position (or is left as a raw, unrestored marker string).Minimal repro:
I want to be precise about severity: I checked whether this is exploitable as an injection/XSS primitive and don't believe it is — the only regex currently fed through this class (
CSSCodec's RGB-triplet pattern) can only ever capture digits,%, commas, whitespace andrgb(/), so a desynchronized restore can't smuggle unexpected characters throughencodeForCSS(). This is an output-integrity bug (silently wrong/corrupted encoder output), not a bypass of the encoding itself.Fix
Derive the default marker per-instance from a random UUID instead of a fixed literal, so it can't be predicted or pre-supplied in input. Hyphens are stripped from the UUID because a hyphen would itself be altered by an encoding pass (e.g.
CSSCodec.encode()backslash-escapes non-immune characters) beforerestoreOriginalContent()runs, which would break the exact-match lookup — I hit this while writing the fix and confirmed it against the existingCSSCodecTestsuite. The publicsetReplacementMarker()API is unchanged for callers who want to supply their own marker.Test plan
testDefaultMarkerIsUnpredictablePerInstance— two instances never produce the same default marker.testInputContainingLiteralClassNameDoesNotDesyncRestoration— regression test reproducing the exact scenario above.mvn -Dtest=EncodingPatternPreservationTest,CSSCodecTest test— 11/11 pass.mvn test— pass, no other suite affected.