Skip to content

Commit cd45dd2

Browse files
committed
Fix predictable marker collision in EncodingPatternPreservation
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.
1 parent 58be169 commit cd45dd2

2 files changed

Lines changed: 61 additions & 6 deletions

File tree

src/main/java/org/owasp/esapi/codecs/ref/EncodingPatternPreservation.java

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import java.util.ArrayList;
44
import java.util.List;
5+
import java.util.UUID;
56
import java.util.regex.Matcher;
67
import java.util.regex.Pattern;
78

@@ -12,12 +13,35 @@
1213
*
1314
*/
1415
public class EncodingPatternPreservation {
15-
/** Default replacement marker. */
16-
private static final String REPLACEMENT_MARKER = EncodingPatternPreservation.class.getSimpleName();
1716
/** Pattern that is used to identify which content should be replaced. */
1817
private final Pattern noEncodeContent;
19-
/** The Marker used to replace found Pattern references. */
20-
private String replacementMarker = REPLACEMENT_MARKER;
18+
/**
19+
* The Marker used to replace found Pattern references. Defaults to a
20+
* per-instance random value so that input content cannot collide with it;
21+
* see {@link #captureAndReplaceMatches(String)}.
22+
*/
23+
private String replacementMarker = defaultReplacementMarker();
24+
25+
/**
26+
* Builds an unpredictable default marker. Using a fixed, publicly-known
27+
* marker (e.g. this class's simple name) would let an attacker embed that
28+
* literal string in the input ahead of real matched content; since
29+
* {@link #restoreOriginalContent(String)} replaces markers in encounter
30+
* order via {@code replaceFirst}, that attacker-supplied marker would be
31+
* replaced first, desynchronizing every subsequent restoration.
32+
* <p>
33+
* The marker is kept purely alphanumeric (no hyphens) because callers
34+
* such as {@link org.owasp.esapi.codecs.CSSCodec} run an encoding pass
35+
* over the marker before it is restored; a hyphen would itself be
36+
* escaped by that pass, breaking the exact-match lookup in
37+
* {@link #restoreOriginalContent(String)}.
38+
*
39+
* @return A marker string that is not derivable from public information.
40+
*/
41+
private static String defaultReplacementMarker() {
42+
return EncodingPatternPreservation.class.getSimpleName()
43+
+ UUID.randomUUID().toString().replace("-", "");
44+
}
2145

2246
/**
2347
* The ordered-list of elements that were replaced in the last call to

src/test/java/org/owasp/esapi/codecs/ref/EncodingPatternPreservationTest.java

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@ public void testReplaceAndRestore() {
1515
String origStr = "12 ABC 34 DEF 56 G 7";
1616
String replacedStr = epp.captureAndReplaceMatches(origStr);
1717

18-
assertEquals("12 EncodingPatternPreservation 34 DEF 56 G 7", replacedStr);
18+
assertFalse("Replaced string should no longer contain the matched pattern", replacedStr.contains("ABC"));
19+
assertTrue(replacedStr.startsWith("12 EncodingPatternPreservation"));
20+
assertTrue(replacedStr.endsWith(" 34 DEF 56 G 7"));
1921

2022
String restored = epp.restoreOriginalContent(replacedStr);
2123
assertEquals(origStr, restored);
@@ -28,12 +30,41 @@ public void testReplaceMultipleAndRestore() {
2830
String origStr = "12 ABC 34 ABC 56 G 7 ABC8";
2931
String replacedStr = epp.captureAndReplaceMatches(origStr);
3032

31-
assertEquals("12 EncodingPatternPreservation 34 EncodingPatternPreservation 56 G 7 EncodingPatternPreservation8", replacedStr);
33+
assertFalse("Replaced string should no longer contain the matched pattern", replacedStr.contains("ABC"));
3234

3335
String restored = epp.restoreOriginalContent(replacedStr);
3436
assertEquals(origStr, restored);
3537
}
3638

39+
@Test
40+
public void testDefaultMarkerIsUnpredictablePerInstance() {
41+
Pattern numberRegex = Pattern.compile("(ABC)");
42+
EncodingPatternPreservation epp1 = new EncodingPatternPreservation(numberRegex);
43+
EncodingPatternPreservation epp2 = new EncodingPatternPreservation(numberRegex);
44+
45+
String replaced1 = epp1.captureAndReplaceMatches("ABC");
46+
String replaced2 = epp2.captureAndReplaceMatches("ABC");
47+
48+
assertNotEquals("Default marker must not be a fixed, guessable value shared across instances",
49+
replaced1, replaced2);
50+
}
51+
52+
@Test
53+
public void testInputContainingLiteralClassNameDoesNotDesyncRestoration() {
54+
// Regression test: previously the default marker was the fixed literal
55+
// "EncodingPatternPreservation", so input already containing that literal
56+
// string ahead of real matched content would be restored in place of the
57+
// real match, desynchronizing every subsequent restoreFirst() call.
58+
Pattern numberRegex = Pattern.compile("(ABC)");
59+
EncodingPatternPreservation epp = new EncodingPatternPreservation(numberRegex);
60+
String origStr = "EncodingPatternPreservation prefix ABC suffix";
61+
62+
String replacedStr = epp.captureAndReplaceMatches(origStr);
63+
String restored = epp.restoreOriginalContent(replacedStr);
64+
65+
assertEquals(origStr, restored);
66+
}
67+
3768
@Test
3869
public void testSetMarker() {
3970
Pattern numberRegex = Pattern.compile("(ABC)");

0 commit comments

Comments
 (0)