Apache/struts
1
0
Fork 0
mirror of https://github.com/apache/struts.git synced 2026-08-06 07:06:58 +00:00
Code Issues Packages Projects Releases Wiki Activity

WW-5643 fix(json): confine StrutsJSONReader parse state to the parsing thread (#1775)

Browse Source 
* 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 <g0w6y@users.noreply.github.com>
This commit is contained in:
ⳕⲛτⲉⲅⲥⲉⳏτⲟⲅ 🕵🏻
2026-07-14 12:18:46 +05:30
committed by GitHub
parent 8fb8bcbcf0
commit 75a285a106
2 changed files with 159 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java
+78 -56
View File
@@ -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.
* </p>
*
* <p>
* 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.
* </p>
*/
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<ParseState> 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<String, Object> 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<String, Object> 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<Object> 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<Object> 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;
};
}
plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java
+81
View File
@@ -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.
*
* <p>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.</p>
*/
@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();
}
}