From 75a285a1065313e082d4ecf4e678211ee65331e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=B3=95=E2=B2=9B=CF=84=E2=B2=89=E2=B2=85=E2=B2=A5?= =?UTF-8?q?=E2=B2=89=E2=B3=8F=CF=84=E2=B2=9F=E2=B2=85=20=F0=9F=95=B5?= =?UTF-8?q?=F0=9F=8F=BB?= <192411347+g0w6y@users.noreply.github.com> Date: Tue, 14 Jul 2026 12:18:46 +0530 Subject: [PATCH] WW-5643 fix(json): confine StrutsJSONReader parse state to the parsing thread (#1775) * 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. * 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. --------- Co-authored-by: g0w6y --- .../apache/struts2/json/StrutsJSONReader.java | 134 ++++++++++-------- .../struts2/json/StrutsJSONReaderTest.java | 81 +++++++++++ 2 files changed, 159 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 530a479cc..96593d2b9 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 @@ import java.util.Map; * 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 class StrutsJSONReader implements JSONReader { } 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 @@ public class StrutsJSONReader implements JSONReader { return ret; } finally { - this.depth--; + state.depth--; } } @@ -199,21 +217,22 @@ public class StrutsJSONReader implements JSONReader { } 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 @@ public class StrutsJSONReader implements JSONReader { 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 @@ public class StrutsJSONReader implements JSONReader { 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 @@ public class StrutsJSONReader implements JSONReader { } 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 @@ public class StrutsJSONReader implements JSONReader { } 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 @@ public class StrutsJSONReader implements JSONReader { 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 48c69c865..827711c61 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 org.junit.Test; 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,80 @@ public class StrutsJSONReaderTest { 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 -- 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 testConcurrentReuseDoesNotBypassMaxDepthOrLeakDataAcrossParses() throws Exception { + int maxDepth = 5; + var reader = new StrutsJSONReader(); + reader.setMaxDepth(maxDepth); + + String overDepthPayload = nestedObject(maxDepth + 1); + int threadCount = 16; + int iterations = 10_000; + AtomicInteger overDepthAccepted = new AtomicInteger(); + 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(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 is not itself the thing measured here + } + try { + reader.read(overDepthPayload); + overDepthAccepted.incrementAndGet(); + } catch (JSONException expected) { + // must always be rejected: depth is one more than maxDepth + } + } + }); + } + + 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 await(CountDownLatch latch) { + try { + latch.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + + private static String nestedObject(int depth) { + StringBuilder sb = new StringBuilder(); + sb.append("{\"a\":".repeat(depth)).append("1").append("}".repeat(depth)); + return sb.toString(); + } }