fix(json): confine StrutsJSONReader parse state to the parsing thread#1775
Open
g0w6y wants to merge 2 commits into
Open
fix(json): confine StrutsJSONReader parse state to the parsing thread#1775g0w6y wants to merge 2 commits into
g0w6y wants to merge 2 commits into
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.
3 tasks
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).