diff --git a/src/main/java/org/owasp/esapi/codecs/ref/EncodingPatternPreservation.java b/src/main/java/org/owasp/esapi/codecs/ref/EncodingPatternPreservation.java index 9ca5c51a8..bb588f11f 100644 --- a/src/main/java/org/owasp/esapi/codecs/ref/EncodingPatternPreservation.java +++ b/src/main/java/org/owasp/esapi/codecs/ref/EncodingPatternPreservation.java @@ -2,6 +2,7 @@ import java.util.ArrayList; import java.util.List; +import java.util.UUID; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -12,12 +13,35 @@ * */ public class EncodingPatternPreservation { - /** Default replacement marker. */ - private static final String REPLACEMENT_MARKER = EncodingPatternPreservation.class.getSimpleName(); /** Pattern that is used to identify which content should be replaced. */ private final Pattern noEncodeContent; - /** The Marker used to replace found Pattern references. */ - private String replacementMarker = REPLACEMENT_MARKER; + /** + * The Marker used to replace found Pattern references. Defaults to a + * per-instance random value so that input content cannot collide with it; + * see {@link #captureAndReplaceMatches(String)}. + */ + private String replacementMarker = defaultReplacementMarker(); + + /** + * Builds an unpredictable default marker. Using a fixed, publicly-known + * marker (e.g. this class's simple name) would let an attacker embed that + * literal string in the input ahead of real matched content; since + * {@link #restoreOriginalContent(String)} replaces markers in encounter + * order via {@code replaceFirst}, that attacker-supplied marker would be + * replaced first, desynchronizing every subsequent restoration. + *
+ * The marker is kept purely alphanumeric (no hyphens) because callers + * such as {@link org.owasp.esapi.codecs.CSSCodec} run an encoding pass + * over the marker before it is restored; a hyphen would itself be + * escaped by that pass, breaking the exact-match lookup in + * {@link #restoreOriginalContent(String)}. + * + * @return A marker string that is not derivable from public information. + */ + private static String defaultReplacementMarker() { + return EncodingPatternPreservation.class.getSimpleName() + + UUID.randomUUID().toString().replace("-", ""); + } /** * The ordered-list of elements that were replaced in the last call to diff --git a/src/test/java/org/owasp/esapi/codecs/ref/EncodingPatternPreservationTest.java b/src/test/java/org/owasp/esapi/codecs/ref/EncodingPatternPreservationTest.java index 59701c58f..acd739720 100644 --- a/src/test/java/org/owasp/esapi/codecs/ref/EncodingPatternPreservationTest.java +++ b/src/test/java/org/owasp/esapi/codecs/ref/EncodingPatternPreservationTest.java @@ -15,7 +15,9 @@ public void testReplaceAndRestore() { String origStr = "12 ABC 34 DEF 56 G 7"; String replacedStr = epp.captureAndReplaceMatches(origStr); - assertEquals("12 EncodingPatternPreservation 34 DEF 56 G 7", replacedStr); + assertFalse("Replaced string should no longer contain the matched pattern", replacedStr.contains("ABC")); + assertTrue(replacedStr.startsWith("12 EncodingPatternPreservation")); + assertTrue(replacedStr.endsWith(" 34 DEF 56 G 7")); String restored = epp.restoreOriginalContent(replacedStr); assertEquals(origStr, restored); @@ -28,12 +30,41 @@ public void testReplaceMultipleAndRestore() { String origStr = "12 ABC 34 ABC 56 G 7 ABC8"; String replacedStr = epp.captureAndReplaceMatches(origStr); - assertEquals("12 EncodingPatternPreservation 34 EncodingPatternPreservation 56 G 7 EncodingPatternPreservation8", replacedStr); + assertFalse("Replaced string should no longer contain the matched pattern", replacedStr.contains("ABC")); String restored = epp.restoreOriginalContent(replacedStr); assertEquals(origStr, restored); } + @Test + public void testDefaultMarkerIsUnpredictablePerInstance() { + Pattern numberRegex = Pattern.compile("(ABC)"); + EncodingPatternPreservation epp1 = new EncodingPatternPreservation(numberRegex); + EncodingPatternPreservation epp2 = new EncodingPatternPreservation(numberRegex); + + String replaced1 = epp1.captureAndReplaceMatches("ABC"); + String replaced2 = epp2.captureAndReplaceMatches("ABC"); + + assertNotEquals("Default marker must not be a fixed, guessable value shared across instances", + replaced1, replaced2); + } + + @Test + public void testInputContainingLiteralClassNameDoesNotDesyncRestoration() { + // Regression test: previously the default marker was the fixed literal + // "EncodingPatternPreservation", so input already containing that literal + // string ahead of real matched content would be restored in place of the + // real match, desynchronizing every subsequent restoreFirst() call. + Pattern numberRegex = Pattern.compile("(ABC)"); + EncodingPatternPreservation epp = new EncodingPatternPreservation(numberRegex); + String origStr = "EncodingPatternPreservation prefix ABC suffix"; + + String replacedStr = epp.captureAndReplaceMatches(origStr); + String restored = epp.restoreOriginalContent(replacedStr); + + assertEquals(origStr, restored); + } + @Test public void testSetMarker() { Pattern numberRegex = Pattern.compile("(ABC)");