g0w6y opened a new pull request, #1775:
URL: https://github.com/apache/struts/pull/1775
## Summary
`JSONInterceptor` obtains its `JSONReader` once via `@Inject` (through
`JSONUtil`) 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 the
`maxElements`/`maxDepth`/`maxStringLength`/`maxKeyLength` DoS limits) kept its
parse cursor, current character, token, string/number buffer and nesting-depth
counter as **plain instance fields**. Two concurrent `read()` 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):
- The shared `depth` counter 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 configured
`maxDepth` through undetected. This defeats the exact DoS protection this limit
exists to provide.
- The shared character cursor and token buffer let fragments of **one
request's JSON body leak into a different, concurrently-parsed request's
result** -- i.e. one user's request data can end up inside an unrelated
concurrent user's deserialized object.
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
`ParseState` object confined to a `ThreadLocal`, created fresh in
`read(String)` and cleared in a `finally` block. All method signatures and
control flow are otherwise unchanged, so existing `StrutsJSONReader` subclasses
(the
`next()`/`skipWhiteSpace()`/`object()`/`array()`/`number()`/`string()`/`add()`/`addDigits()`/`unicode()`
extension points are all `protected`) continue to work unmodified -- the
parse-state fields were already `private`, 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
- [x] Two new regression tests in `StrutsJSONReaderTest`:
- `testConcurrentReuseDoesNotBypassMaxDepth`: two threads share one reader
instance; one repeatedly submits a payload one level deeper than `maxDepth`,
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.
- [x] Full existing
`StrutsJSONReaderTest`/`JSONReaderTest`/`JSONInterceptorTest` suites pass
unchanged -- confirms no behavioral regression from the refactor.
- [x] Full `struts2-json-plugin` module test suite passes (127/127).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
