From cd45dd2dfd70d18111887f0729b1d5a7ca1e9b47 Mon Sep 17 00:00:00 2001 From: Socialpranker <273312799+Socialpranker@users.noreply.github.com> Date: Tue, 14 Jul 2026 11:11:54 +0200 Subject: [PATCH] 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. --- .../ref/EncodingPatternPreservation.java | 32 ++++++++++++++--- .../ref/EncodingPatternPreservationTest.java | 35 +++++++++++++++++-- 2 files changed, 61 insertions(+), 6 deletions(-) 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)");