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..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 @@ -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,80 @@ 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 -- 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(); + } }