WW-5643 fix(json): confine StrutsJSONReader parse state to the parsing thread#1775
Conversation
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.
…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.
|
Thank you for the PR but please create a JIRA ticket to cover this work 🙏 Also bear in mind that I work on changes in the JSON plugin as well #1773 |
g0w6y
changed the title
|
Created WW-5643 to track this: https://issues.apache.org/jira/browse/WW-5643. Ready for review/merge whenever you get a chance thanks! |
|
Given this fixes a cross request data leak between concurrent users along with a maxDepth limit bypass, should we handle this as a security advisory instead of a normal fix? Happy to follow whichever process you prefer, just let me know. |
|
Thanks for the patch, and especially the concurrency regression tests. The thread-safety problem is real — Longer term I'd rather address it at the lifecycle level in |
|
hey @lukaszlenart lets go with security advisory too whats your opinion |
|
I opened a followup PR #1782 to address the mentioned improvement |
2 participants
Fixes WW-5643
Summary
JSONInterceptorobtains itsJSONReaderonce via@Inject(throughJSONUtil) and reuses that same instance across every concurrent request handled by that interceptor -- this is how every Struts interceptor's injected dependencies work, not something specific to this plugin.StrutsJSONReader(introduced to enforce themaxElements/maxDepth/maxStringLength/maxKeyLengthDoS limits) kept its parse cursor, current character, token, string/number buffer and nesting-depth counter as plain instance fields. Two concurrentread()calls on the same reader instance therefore tear each other's parsing state.Concretely, under ordinary concurrent traffic to the same JSON action (no special configuration required -- these limits are on by default):
depthcounter can be decremented by an unrelated concurrent request finishing its own parse while another request's deeply-nested payload is still mid-parse, letting a payload nested deeper than the configuredmaxDepththrough undetected. This defeats the exact DoS protection this limit exists to provide.Both were verified with live, quantified reproductions before writing the fix (not just reasoned about): a payload nested one level deeper than
maxDepth(which must always be rejected) was incorrectly accepted under two-thread contention on a shared reader instance, and a "victim" payload containing a unique marker string bled into a concurrently-parsing "attacker" payload's own parsed result on the same shared reader.Fix
Move the cursor, current character, token, buffer and depth into a
ParseStateobject confined to aThreadLocal, created fresh inread(String)and cleared in afinallyblock. All method signatures and control flow are otherwise unchanged, so existingStrutsJSONReadersubclasses (thenext()/skipWhiteSpace()/object()/array()/number()/string()/add()/addDigits()/unicode()extension points are allprotected) continue to work unmodified -- the parse-state fields were alreadyprivate, so no external code could have been touching them directly. The limit fields (maxElements,maxDepth,maxStringLength,maxKeyLength) stay as ordinary instance fields, since they're set to the same value on every call for a given interceptor configuration and are safe to share across threads.Test plan
StrutsJSONReaderTest:testConcurrentReuseDoesNotBypassMaxDepth: two threads share one reader instance; one repeatedly submits a payload one level deeper thanmaxDepth, the other hammers the same instance with unrelated shallow payloads. Asserts the over-depth payload is never accepted.testConcurrentReuseDoesNotLeakDataAcrossParses: two threads share one reader instance; a "victim" thread submits JSON containing a unique marker string while an "attacker" thread concurrently parses unrelated JSON and inspects its own result. Asserts the attacker's parsed result never contains a fragment of the victim's data.StrutsJSONReaderTest/JSONReaderTest/JSONInterceptorTestsuites pass unchanged -- confirms no behavioral regression from the refactor.struts2-json-pluginmodule test suite passes (127/127).