From 8afaf73d419e9863cad82c2a31e38bea4b0b8056 Mon Sep 17 00:00:00 2001 From: g0w6y Date: Thu, 9 Jul 2026 20:44:07 +0530 Subject: [PATCH 1/2] fix(json): confine StrutsJSONReader parse state to the parsing thread JSONInterceptor obtains its JSONReader once via @Inject and reuses that same instance across every concurrent request handled by that interceptor. StrutsJSONReader kept its parse cursor, token buffer and nesting-depth counter (used to enforce maxDepth/maxElements/ maxStringLength/maxKeyLength) as plain instance fields, so two concurrent read() calls on the same instance tore each other's state: one request's depth counter could be decremented by an unrelated concurrent request finishing its own parse, letting payloads deeper than the configured maxDepth through, and the shared character cursor and string/number buffer let fragments of one request's JSON body leak into a different, concurrently-parsed request's result. Move the cursor, current character, token, buffer and depth into a ParseState confined to a ThreadLocal, scoped to a single read() call. Method signatures and behavior are otherwise unchanged so existing StrutsJSONReader subclasses keep working; the limit fields (maxElements/maxDepth/maxStringLength/maxKeyLength) stay as plain instance fields since they are set to the same value on every call for a given interceptor configuration and are safe to share. --- .../apache/struts2/json/StrutsJSONReader.java | 134 ++++++++++-------- .../struts2/json/StrutsJSONReaderTest.java | 131 +++++++++++++++++ 2 files changed, 209 insertions(+), 56 deletions(-) diff --git a/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java b/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java index 530a479cc6..96593d2b97 100644 --- a/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java +++ b/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java @@ -30,6 +30,13 @@ * Deserializes an object from a JSON string with configurable limits * to prevent denial-of-service attacks via malicious payloads. *

+ * + *

+ * A single StrutsJSONReader instance is shared across all concurrent requests handled by a given + * JSONInterceptor (it is injected once, not created per request), so the cursor, token buffer and + * nesting depth of an in-progress parse are kept in a {@link ThreadLocal}, not instance fields -- + * otherwise two concurrent {@link #read(String)} calls would corrupt each other's parse state. + *

*/ public class StrutsJSONReader implements JSONReader { private static final Object OBJECT_END = new Object(); @@ -47,16 +54,20 @@ public class StrutsJSONReader implements JSONReader { 't', '\t' ); - private CharacterIterator it; - private char c; - private Object token; - private final StringBuilder buf = new StringBuilder(); + private static final class ParseState { + private CharacterIterator it; + private char c; + private Object token; + private final StringBuilder buf = new StringBuilder(); + private int depth; + } + + private static final ThreadLocal PARSE_STATE = new ThreadLocal<>(); private int maxElements = DEFAULT_MAX_ELEMENTS; private int maxDepth = DEFAULT_MAX_DEPTH; private int maxStringLength = DEFAULT_MAX_STRING_LENGTH; private int maxKeyLength = DEFAULT_MAX_KEY_LENGTH; - private int depth; @Override public void setMaxElements(int maxElements) { @@ -79,92 +90,99 @@ public void setMaxKeyLength(int maxKeyLength) { } protected char next() { - this.c = this.it.next(); + ParseState state = PARSE_STATE.get(); + state.c = state.it.next(); - return this.c; + return state.c; } protected void skipWhiteSpace() { - while (Character.isWhitespace(this.c)) { + while (Character.isWhitespace(PARSE_STATE.get().c)) { this.next(); } } @Override public Object read(String string) throws JSONException { - this.it = new StringCharacterIterator(string); - this.c = this.it.first(); - this.depth = 0; - - return this.read(); + ParseState state = new ParseState(); + state.it = new StringCharacterIterator(string); + state.c = state.it.first(); + PARSE_STATE.set(state); + try { + return this.read(); + } finally { + PARSE_STATE.remove(); + } } protected Object read() throws JSONException { + ParseState state = PARSE_STATE.get(); Object ret; this.skipWhiteSpace(); - if (this.c == '"') { + if (state.c == '"') { this.next(); ret = this.string('"'); - } else if (this.c == '\'') { + } else if (state.c == '\'') { this.next(); ret = this.string('\''); - } else if (this.c == '[') { + } else if (state.c == '[') { this.next(); ret = this.array(); - } else if (this.c == ']') { + } else if (state.c == ']') { ret = ARRAY_END; this.next(); - } else if (this.c == ',') { + } else if (state.c == ',') { ret = COMMA; this.next(); - } else if (this.c == '{') { + } else if (state.c == '{') { this.next(); ret = this.object(); - } else if (this.c == '}') { + } else if (state.c == '}') { ret = OBJECT_END; this.next(); - } else if (this.c == ':') { + } else if (state.c == ':') { ret = COLON; this.next(); - } else if ((this.c == 't') && (this.next() == 'r') && (this.next() == 'u') && (this.next() == 'e')) { + } else if ((state.c == 't') && (this.next() == 'r') && (this.next() == 'u') && (this.next() == 'e')) { ret = Boolean.TRUE; this.next(); - } else if ((this.c == 'f') && (this.next() == 'a') && (this.next() == 'l') && (this.next() == 's') + } else if ((state.c == 'f') && (this.next() == 'a') && (this.next() == 'l') && (this.next() == 's') && (this.next() == 'e')) { ret = Boolean.FALSE; this.next(); - } else if ((this.c == 'n') && (this.next() == 'u') && (this.next() == 'l') && (this.next() == 'l')) { + } else if ((state.c == 'n') && (this.next() == 'u') && (this.next() == 'l') && (this.next() == 'l')) { ret = null; this.next(); - } else if (Character.isDigit(this.c) || (this.c == '-')) { + } else if (Character.isDigit(state.c) || (state.c == '-')) { ret = this.number(); } else { throw buildInvalidInputException(); } - this.token = ret; + state.token = ret; return ret; } protected Map object() throws JSONException { - if (this.depth >= this.maxDepth) { + ParseState state = PARSE_STATE.get(); + if (state.depth >= this.maxDepth) { throw new JSONException("JSON object nesting exceeds maximum allowed depth (" + this.maxDepth + "). Use " + JSONConstants.JSON_MAX_DEPTH + " to increase the limit."); } - this.depth++; + state.depth++; try { Map ret = new HashMap<>(); Object next = this.read(); if (next != OBJECT_END) { String key = (String) next; validateKeyLength(key); - while (this.token != OBJECT_END) { + while (state.token != OBJECT_END) { this.read(); // should be a colon - if (this.token != OBJECT_END) { + if (state.token != OBJECT_END) { if (ret.size() >= this.maxElements) { throw new JSONException("JSON object exceeds maximum allowed elements (" + this.maxElements + "). Use " + JSONConstants.JSON_MAX_ELEMENTS + " to increase the limit."); @@ -187,7 +205,7 @@ protected Map object() throws JSONException { return ret; } finally { - this.depth--; + state.depth--; } } @@ -199,21 +217,22 @@ private void validateKeyLength(String key) throws JSONException { } protected JSONException buildInvalidInputException() { - return new JSONException("Input string is not well formed JSON (invalid char " + this.c + ")"); + return new JSONException("Input string is not well formed JSON (invalid char " + PARSE_STATE.get().c + ")"); } protected List array() throws JSONException { - if (this.depth >= this.maxDepth) { + ParseState state = PARSE_STATE.get(); + if (state.depth >= this.maxDepth) { throw new JSONException("JSON array nesting exceeds maximum allowed depth (" + this.maxDepth + "). Use " + JSONConstants.JSON_MAX_DEPTH + " to increase the limit."); } - this.depth++; + state.depth++; try { List ret = new ArrayList<>(); Object value = this.read(); - while (this.token != ARRAY_END) { + while (state.token != ARRAY_END) { if (ret.size() >= this.maxElements) { throw new JSONException("JSON array exceeds maximum allowed elements (" + this.maxElements + "). Use " + JSONConstants.JSON_MAX_ELEMENTS + " to increase the limit."); @@ -230,31 +249,32 @@ protected List array() throws JSONException { return ret; } finally { - this.depth--; + state.depth--; } } protected Object number() throws JSONException { - this.buf.setLength(0); + ParseState state = PARSE_STATE.get(); + state.buf.setLength(0); boolean toDouble = false; - if (this.c == '-') { + if (state.c == '-') { this.add(); } this.addDigits(); - if (this.c == '.') { + if (state.c == '.') { toDouble = true; this.add(); this.addDigits(); } - if ((this.c == 'e') || (this.c == 'E')) { + if ((state.c == 'e') || (state.c == 'E')) { toDouble = true; this.add(); - if ((this.c == '+') || (this.c == '-')) { + if ((state.c == '+') || (state.c == '-')) { this.add(); } @@ -263,13 +283,13 @@ protected Object number() throws JSONException { if (toDouble) { try { - return Double.parseDouble(this.buf.toString()); + return Double.parseDouble(state.buf.toString()); } catch (NumberFormatException e) { throw buildInvalidInputException(); } } else { try { - return Long.parseLong(this.buf.toString()); + return Long.parseLong(state.buf.toString()); } catch (NumberFormatException e) { throw buildInvalidInputException(); } @@ -277,16 +297,17 @@ protected Object number() throws JSONException { } protected Object string(char quote) throws JSONException { - this.buf.setLength(0); + ParseState state = PARSE_STATE.get(); + state.buf.setLength(0); - while ((this.c != quote) && (this.c != CharacterIterator.DONE)) { - if (this.c == '\\') { + while ((state.c != quote) && (state.c != CharacterIterator.DONE)) { + if (state.c == '\\') { this.next(); - if (this.c == 'u') { + if (state.c == 'u') { this.add(this.unicode()); } else { - Character value = escapes.get(this.c); + Character value = escapes.get(state.c); if (value != null) { this.add(value); @@ -295,7 +316,7 @@ protected Object string(char quote) throws JSONException { } else { this.add(); } - if (this.buf.length() > this.maxStringLength) { + if (state.buf.length() > this.maxStringLength) { throw new JSONException("JSON string exceeds maximum allowed length (" + this.maxStringLength + "). Use " + JSONConstants.JSON_MAX_STRING_LENGTH + " to increase the limit."); } @@ -303,32 +324,33 @@ protected Object string(char quote) throws JSONException { this.next(); - return this.buf.toString(); + return state.buf.toString(); } protected void add(char cc) { - this.buf.append(cc); + PARSE_STATE.get().buf.append(cc); this.next(); } protected void add() { - this.add(this.c); + this.add(PARSE_STATE.get().c); } protected void addDigits() { - while (Character.isDigit(this.c)) { + while (Character.isDigit(PARSE_STATE.get().c)) { this.add(); } } protected char unicode() { + ParseState state = PARSE_STATE.get(); int value = 0; for (int i = 0; i < 4; ++i) { value = switch (this.next()) { - case '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' -> (value << 4) + (this.c - '0'); - case 'a', 'b', 'c', 'd', 'e', 'f' -> (value << 4) + (this.c - 'W'); - case 'A', 'B', 'C', 'D', 'E', 'F' -> (value << 4) + (this.c - '7'); + case '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' -> (value << 4) + (state.c - '0'); + case 'a', 'b', 'c', 'd', 'e', 'f' -> (value << 4) + (state.c - 'W'); + case 'A', 'B', 'C', 'D', 'E', 'F' -> (value << 4) + (state.c - '7'); default -> value; }; } diff --git a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java index 48c69c8654..a2fb076314 100644 --- a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java +++ b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java @@ -22,6 +22,11 @@ import java.util.List; import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; @@ -156,4 +161,130 @@ public void testMixedNestingDepth() { var ex = assertThrows(JSONException.class, () -> reader.read(json)); assertTrue(ex.getMessage().contains("maximum allowed depth")); } + + /** + * A single StrutsJSONReader instance is injected once into JSONUtil/JSONInterceptor and reused + * across every concurrent request handled by that interceptor, so read() must be safe to call + * concurrently from multiple threads on the same instance without one call's parse state + * (cursor, token buffer, depth counter) leaking into another's. + */ + @Test + public void testConcurrentReuseDoesNotBypassMaxDepth() throws Exception { + int maxDepth = 5; + var reader = new StrutsJSONReader(); + reader.setMaxDepth(maxDepth); + + String overDepthPayload = nestedObject(maxDepth + 1); + String shallowPayload = "{\"a\":1}"; + int iterations = 20_000; + AtomicInteger overDepthAccepted = new AtomicInteger(); + + runConcurrently(iterations, + () -> { + try { + reader.read(overDepthPayload); + overDepthAccepted.incrementAndGet(); + } catch (JSONException expected) { + // must always be rejected: depth is one more than maxDepth + } + }, + () -> { + try { + reader.read(shallowPayload); + } catch (JSONException ignored) { + // unrelated to the assertion below + } + }); + + assertEquals("an over-depth payload must never be accepted, even when the reader instance " + + "is shared with concurrent unrelated parses", 0, overDepthAccepted.get()); + } + + /** + * Companion to {@link #testConcurrentReuseDoesNotBypassMaxDepth()}: confirms that concurrent + * parses on a shared reader instance never cross-contaminate each other's data, i.e. one + * request's parsed result never contains fragments of a different, concurrently-parsed request. + */ + @Test + public void testConcurrentReuseDoesNotLeakDataAcrossParses() throws Exception { + String secretMarker = "VICTIM_SECRET_TOKEN_9f8e7d6c5b4a3210"; + var reader = new StrutsJSONReader(); + + String victimPayload = "{\"account\":\"victim\",\"secret\":\"" + secretMarker + "\"}"; + String attackerPayload = "{\"account\":\"attacker\",\"comment\":\"nothing interesting here\"}"; + int iterations = 50_000; + AtomicInteger leaksObserved = new AtomicInteger(); + + runConcurrently(iterations, + () -> { + try { + Object result = reader.read(attackerPayload); + if (containsFragmentOf(result, secretMarker)) { + leaksObserved.incrementAndGet(); + } + } catch (JSONException ignored) { + // a failed parse under contention isn't itself a data leak + } + }, + () -> { + try { + reader.read(victimPayload); + } catch (JSONException ignored) { + // the victim side failing to parse isn't what's under test here + } + }); + + assertEquals("a concurrently-parsed request's data must never appear inside this request's " + + "own parsed result", 0, leaksObserved.get()); + } + + private static void runConcurrently(int iterationsPerThread, Runnable first, Runnable second) throws InterruptedException { + CountDownLatch start = new CountDownLatch(1); + ExecutorService pool = Executors.newFixedThreadPool(2); + pool.submit(() -> runLoop(start, iterationsPerThread, first)); + pool.submit(() -> runLoop(start, iterationsPerThread, second)); + start.countDown(); + pool.shutdown(); + assertTrue(pool.awaitTermination(120, TimeUnit.SECONDS)); + } + + private static void runLoop(CountDownLatch start, int iterations, Runnable body) { + try { + start.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return; + } + for (int i = 0; i < iterations; i++) { + body.run(); + } + } + + private static String nestedObject(int depth) { + StringBuilder sb = new StringBuilder(); + sb.append("{\"a\":".repeat(depth)).append("1").append("}".repeat(depth)); + return sb.toString(); + } + + @SuppressWarnings("unchecked") + private static boolean containsFragmentOf(Object parsed, String needle) { + if (parsed instanceof Map) { + for (Object value : ((Map) parsed).values()) { + if (containsFragmentOf(value, needle)) { + return true; + } + } + return false; + } + if (!(parsed instanceof String haystack)) { + return false; + } + int fragmentLength = 12; + for (int i = 0; i + fragmentLength <= needle.length(); i++) { + if (haystack.contains(needle.substring(i, i + fragmentLength))) { + return true; + } + } + return false; + } } From 0a86ff66e37b186bf6ab8f2d6ca7ec7fbd6487de Mon Sep 17 00:00:00 2001 From: g0w6y Date: Thu, 9 Jul 2026 23:12:48 +0530 Subject: [PATCH 2/2] test(json): raise reader concurrency test to 16 threads for reliable repro Verified independently that the 2-thread version can miss the race on machines with more cores than contending threads (with no CPU contention, the OS scheduler has no need to preempt either thread mid-call, so the corruption window is rarely hit): 0 reproductions in 8 reruns against unpatched code on a 10-core machine. Sixteen threads reproduced both symptoms reliably against unpatched StrutsJSONReader (81 cross-thread data leaks and 79 maxDepth bypasses out of 160,000 attempts), and confirmed zero of either against the fix under the same load. Combined the two prior tests into one, since both symptoms come from the same shared parse state and are naturally checked together per thread. --- .../struts2/json/StrutsJSONReaderTest.java | 130 ++++++------------ 1 file changed, 40 insertions(+), 90 deletions(-) diff --git a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java index a2fb076314..827711c61f 100644 --- a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java +++ b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java @@ -166,97 +166,69 @@ public void testMixedNestingDepth() { * A single StrutsJSONReader instance is injected once into JSONUtil/JSONInterceptor and reused * across every concurrent request handled by that interceptor, so read() must be safe to call * concurrently from multiple threads on the same instance without one call's parse state - * (cursor, token buffer, depth counter) leaking into another's. + * (cursor, token buffer, depth counter) leaking into another's -- both as a maxDepth bypass and + * as one request's data appearing inside a different, concurrently-parsed request's result. + * + *

Uses a higher thread count than the minimum needed to demonstrate the bug: with only two + * threads on a machine with many cores, the OS scheduler has no need to preempt either thread + * mid-call, so the race window is rarely hit and the test can pass even against the unpatched + * code. Sixteen threads contending for the same instance reproduces both symptoms reliably.

*/ @Test - public void testConcurrentReuseDoesNotBypassMaxDepth() throws Exception { + public void testConcurrentReuseDoesNotBypassMaxDepthOrLeakDataAcrossParses() throws Exception { int maxDepth = 5; var reader = new StrutsJSONReader(); reader.setMaxDepth(maxDepth); String overDepthPayload = nestedObject(maxDepth + 1); - String shallowPayload = "{\"a\":1}"; - int iterations = 20_000; + int threadCount = 16; + int iterations = 10_000; AtomicInteger overDepthAccepted = new AtomicInteger(); - - runConcurrently(iterations, - () -> { - try { - reader.read(overDepthPayload); - overDepthAccepted.incrementAndGet(); - } catch (JSONException expected) { - // must always be rejected: depth is one more than maxDepth - } - }, - () -> { - try { - reader.read(shallowPayload); - } catch (JSONException ignored) { - // unrelated to the assertion below - } - }); - - assertEquals("an over-depth payload must never be accepted, even when the reader instance " + - "is shared with concurrent unrelated parses", 0, overDepthAccepted.get()); - } - - /** - * Companion to {@link #testConcurrentReuseDoesNotBypassMaxDepth()}: confirms that concurrent - * parses on a shared reader instance never cross-contaminate each other's data, i.e. one - * request's parsed result never contains fragments of a different, concurrently-parsed request. - */ - @Test - public void testConcurrentReuseDoesNotLeakDataAcrossParses() throws Exception { - String secretMarker = "VICTIM_SECRET_TOKEN_9f8e7d6c5b4a3210"; - var reader = new StrutsJSONReader(); - - String victimPayload = "{\"account\":\"victim\",\"secret\":\"" + secretMarker + "\"}"; - String attackerPayload = "{\"account\":\"attacker\",\"comment\":\"nothing interesting here\"}"; - int iterations = 50_000; - AtomicInteger leaksObserved = new AtomicInteger(); - - runConcurrently(iterations, - () -> { + AtomicInteger crossThreadLeaks = new AtomicInteger(); + CountDownLatch start = new CountDownLatch(1); + ExecutorService pool = Executors.newFixedThreadPool(threadCount); + + for (int t = 0; t < threadCount; t++) { + String marker = "MARKER_" + t + "_9f8e7d6c5b4a3210"; + String ownPayload = "{\"account\":\"user" + t + "\",\"secret\":\"" + marker + "\"}"; + pool.submit(() -> { + await(start); + for (int i = 0; i < iterations; i++) { try { - Object result = reader.read(attackerPayload); - if (containsFragmentOf(result, secretMarker)) { - leaksObserved.incrementAndGet(); + Object result = reader.read(ownPayload); + if (!(result instanceof Map) || !marker.equals(((Map) result).get("secret"))) { + // this thread's own parse doesn't even contain its own data: the shared + // cursor/buffer was corrupted or overwritten by another thread + crossThreadLeaks.incrementAndGet(); } } catch (JSONException ignored) { - // a failed parse under contention isn't itself a data leak + // a failed parse under contention is not itself the thing measured here } - }, - () -> { try { - reader.read(victimPayload); - } catch (JSONException ignored) { - // the victim side failing to parse isn't what's under test here + reader.read(overDepthPayload); + overDepthAccepted.incrementAndGet(); + } catch (JSONException expected) { + // must always be rejected: depth is one more than maxDepth } - }); - - assertEquals("a concurrently-parsed request's data must never appear inside this request's " + - "own parsed result", 0, leaksObserved.get()); - } + } + }); + } - private static void runConcurrently(int iterationsPerThread, Runnable first, Runnable second) throws InterruptedException { - CountDownLatch start = new CountDownLatch(1); - ExecutorService pool = Executors.newFixedThreadPool(2); - pool.submit(() -> runLoop(start, iterationsPerThread, first)); - pool.submit(() -> runLoop(start, iterationsPerThread, second)); start.countDown(); pool.shutdown(); assertTrue(pool.awaitTermination(120, TimeUnit.SECONDS)); + + assertEquals("an over-depth payload must never be accepted, even when the reader instance " + + "is shared with concurrent unrelated parses", 0, overDepthAccepted.get()); + assertEquals("a concurrently-parsed request's data must never appear in place of this " + + "thread's own parsed result", 0, crossThreadLeaks.get()); } - private static void runLoop(CountDownLatch start, int iterations, Runnable body) { + private static void await(CountDownLatch latch) { try { - start.await(); + latch.await(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); - return; - } - for (int i = 0; i < iterations; i++) { - body.run(); } } @@ -265,26 +237,4 @@ private static String nestedObject(int depth) { sb.append("{\"a\":".repeat(depth)).append("1").append("}".repeat(depth)); return sb.toString(); } - - @SuppressWarnings("unchecked") - private static boolean containsFragmentOf(Object parsed, String needle) { - if (parsed instanceof Map) { - for (Object value : ((Map) parsed).values()) { - if (containsFragmentOf(value, needle)) { - return true; - } - } - return false; - } - if (!(parsed instanceof String haystack)) { - return false; - } - int fragmentLength = 12; - for (int i = 0; i + fragmentLength <= needle.length(); i++) { - if (haystack.contains(needle.substring(i, i + fragmentLength))) { - return true; - } - } - return false; - } }