fix(#5895): bound RESP array nesting depth and element count in the Redis wrapper - #5902
Conversation
…edis wrapper RedisNetworkExecutor.parseNext() recursed once per RESP array nesting level with no depth limit, so a small (~47 KB) deeply-nested payload overflowed the connection thread's JVM stack before the NOAUTH check could reject it. The client-declared array length was equally unbounded, letting a single *2000000000\r\n header start a parse loop the client could keep alive indefinitely. Both are now bounded by two configurable settings mirroring Redis' own protocol limits (arcadedb.redis.maxMultiBulkDepth, default 32; arcadedb.redis.maxMultiBulkLength, default 1048576, matching Redis' own hard cap). Either violation fails fast with a RESP error reply and closes the connection.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 5 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Review: fix(#5895): bound RESP array nesting depth and element count in the Redis wrapperSolid fix for the two vectors described in the issue - the depth counter is threaded cleanly through Security - the same vulnerability class is still open for bulk strings ($)
```java
Minor: malformed (non-numeric) length escapes the new error handling
Minor: no lower-bound validation on the new settingsIf Design note (non-blocking)Real Redis's request-side protocol doesn't actually support nested arrays as command elements at all - Test coverageThe two new tests are well-targeted (raw socket, reproduce-then-verify-fixed style, plus a following Jedis round-trip to confirm the listener/thread survives). Two gaps:
Nits
Overall: good, well-tested fix for the two reported vectors with a sensible fail-closed design. I'd consider the bulk-string ($) gap above worth addressing before/alongside this, since it's the same bug class and likely higher-traffic code path than the array-nesting one. |
…ength handling Code review on the array-nesting fix flagged that RESP bulk strings ($) had the same unbounded-length problem as arrays, but on a hotter path: the command name and every argument (including GET/SET's own payloads) are bulk strings, so a $2000000000\r\n header could tie up a connection thread the same way *2000000000\r\n did, or grow the parse buffer without bound if the bytes were actually sent. Bounded by a new arcadedb.redis.maxBulkLength setting (default 512MB, matching Redis' own proto-max-bulk-len). Also: a non-numeric length (e.g. $abc\r\n) used to throw an uncaught NumberFormatException that killed the connection thread outright instead of getting the clean -ERR Protocol error + close treatment added for the size-related cases; and the three new limit settings now fall back to their default (with a warning) if misconfigured below the minimum value a connection can actually parse with.
|
Addressed the review points:
All 51 |
ReviewSolid, well-scoped security fix. The core mechanism is correct: bounding recursion depth via a counter checked at the top of Nice touches
Found one issue worth fixing
This is the only such trailing Minor / non-blocking observations
Overall: the fix addresses the reported DoS class correctly, is well tested, and the code matches the file's existing style. Just the stray doc link to clean up. |
…gth limits Second review round caught that appending the new Redis section directly after the vector-index section left that section's closing [#5588] reference link stranded past the new content instead of closing its own section - moved it back. Also added symmetric end-to-end tests confirming arcadedb.redis.maxMultiBulkLength and arcadedb.redis.maxBulkLength are actually wired to a non-default configured value (configuredDepthLimitIsHonored already covered maxMultiBulkDepth this way).
|
Addressed round 2:
All 53 |
ReviewSolid fix for a real pre-auth DoS in the Redis wire protocol ( Correctness
Minor, non-blocking observations
Test coverage Docs & config Nice, thorough writeup in the PR description and commit messages too — the three-commit progression (nesting depth → bulk-string length → wiring tests + doc fix) reads like genuine iterative hardening rather than a single rushed patch. |
…nection Round 3 review noted sanitizedLimit() logging a WARNING on every new connection when a setting is misconfigured could get log-spammy on a busy server churning through connections against a static bad value. The value is still re-read (and re-validated) per connection since it can change at runtime, but the warning itself now only fires once per setting per JVM.
|
Round 3 was clean (no blocking issues). Of the three minor/non-blocking notes:
All 53 |
ReviewSolid, well-scoped fix for a real pre-auth DoS (issue #5895). The depth/length bounds are checked at the earliest possible point (right after the header line is parsed, before entering any loop), which correctly closes both the stack-overflow vector and the "declare a huge size, trickle bytes" CPU/parse-loop vector described in the PR. Good iterative hardening across the four commits (array depth -> bulk length -> malformed-length handling -> log-spam fix). Test coverage is thorough: it reproduces the crash/hang against the described scenarios and separately proves each of the three settings is actually wired end-to-end rather than only exercising defaults. Possible issue: new error path bypasses the existing RESP error-sanitization convention
static String respErrorMessage(final Throwable error) {
...
return message.replace('\r', ' ').replace('\n', ' ');
}The new } catch (final RedisProtocolLimitException e) {
...
value.append("-ERR ").append(e.getMessage());
appendCrLf();
replyToClient(value);
Minor / informational, pre-existing (not introduced by this PR)
Nit
Nice-to-haves already done well
No blocking issues found; the one worth addressing before merge is the sanitization inconsistency above, since this PR's whole purpose is hardening this exact parser against adversarial input. |
…ErrorMessage() Round 4 review caught that RedisProtocolLimitException's catch block in run() embedded e.getMessage() into the RESP -ERR line verbatim, unlike executeCommand's generic catch block which strips \r/\n via respErrorMessage() precisely because RESP error replies are single-line. parseLength()'s malformed-token message embeds the raw client-supplied token, and parseValueUntilLF() only treats \r as the start of the CRLF terminator, so a bare \n survives into the token (e.g. $1\nA\r\n) and would have been echoed unsanitized into the reply. Now reuses respErrorMessage() for consistency, with a regression test that reproduces the raw-\n reply against the unpatched code and confirms the fix. Also documents that sanitizedLimit()'s floor of 1 for the length-based settings means "parses at all", not "usable for real traffic" (per review nit).
|
Round 4 addressed:
All 54 |
ReviewSolid fix for a real pre-auth DoS in the Redis wire protocol (unbounded RESP array recursion leading to Code quality / correctness
Performance
Security
Test coverage
Docs / config
Overall: well-scoped, well-tested fix for a genuine pre-auth vulnerability. Only nit-level feedback above; nothing blocking. |
Round 5 review noted the depth check (depth > maxMultiBulkDepth) admits one more nesting level than documented, since depth starts at 0 for the top-level call: a "default 32" setting actually let 33 levels through. Changed to >=, which required raising maxMultiBulkDepth's sanitizedLimit floor from 1 to 2 (a flat command's single argument is parsed at depth 1, so >= needs at least 2 to admit it). Also added a regression test for sanitizedLimit's fallback-with-warning path itself (the only untested branch in an otherwise thorough suite per the review), which pipelines a malformed-shape command with a normal PING on the same connection to distinguish "fell back to the default" from "used the broken configured value".
|
Round 5 addressed:
All 55 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 5 |
🟢 Coverage 93.75% diff coverage · -6.84% coverage variation
Metric Results Coverage variation ✅ -6.84% coverage variation Diff coverage ✅ 93.75% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (6bbb0e7) 152707 117904 77.21% Head commit (d8c18a6) 185097 (+32390) 130253 (+12349) 70.37% (-6.84%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#5902) 48 45 93.75% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Review: RESP array/bulk-string length bounds (fixes #5895)Solid fix for a real pre-auth DoS: unbounded RESP array nesting could overflow the connection thread's stack, and unbounded declared lengths ( Strengths
Worth considering (non-blocking)
Nits
Nice work tightening this down through the review rounds (especially catching the bulk-string case and the raw-newline-in-error-message edge case) - this closes a legitimate pre-auth DoS cleanly. |
…ded-growth gap Round 6 review noted that the new size/depth checks only fire once parseValueUntilLF() has actually produced a token (it looks for a terminating CRLF), so a client that never sends one - e.g. "$" followed by megabytes of digits with no \r\n - grows that buffer unbounded and holds the thread before maxBulkLength/maxMultiBulkLength/maxMultiBulkDepth ever get a value to check against. It's the same DoS class this PR fixes, just one level up: the length token itself instead of the declared length. Real RESP length/integer tokens and simple-string values are always short, so parseValueUntilLF() now caps accumulation at a fixed 64 characters (a signed 64-bit decimal has at most 20), throwing the same RedisProtocolLimitException as the other checks. Regression test confirms this hangs on unpatched code (verified via SocketTimeoutException) and is rejected cleanly with the fix.
|
Round 6 addressed:
All 56 |
ReviewSolid, well-scoped security fix for #5895. The commit history shows this already went through several rounds of self-review (depth off-by-one, A few non-blocking observations: 1. 2. 3. Minor: Nit: the depth/length constants and Overall: the core stack-overflow and unbounded-loop DoS vectors are closed correctly, the fix mirrors the wire protocol's own documented limits, error handling is consistent (single-line RESP replies, no raw exception messages leaking), and test coverage is thorough (including the trickier wiring/fallback/sanitization cases). The two substantive points above (#1 and #2) are worth a look but neither blocks this as currently a solid, defensive fix for a real pre-auth DoS. |
|
Full CI run is complete. Summary for review: All 7 automated review rounds addressed (see commit history and inline replies above) — the fix now bounds RESP array nesting depth, declared array/bulk-string length, and the raw length-token accumulator itself, with consistent fail-closed error handling (CRLF-sanitized single-line RESP replies) and 56 passing regression tests in Two out-of-scope findings filed as follow-ups, not fixed here to keep this PR focused on the #5895 DoS class:
CI status: all checks green except three pre-existing failures, none touching
Ready for your review, Luca. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5902 +/- ##
==========================================
- Coverage 68.39% 68.00% -0.40%
==========================================
Files 1794 1795 +1
Lines 152707 152754 +47
Branches 32366 32376 +10
==========================================
- Hits 104450 103878 -572
- Misses 34843 35498 +655
+ Partials 13414 13378 -36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
* fix: RESP parser correctness and idle-connection hardening (#5907, #5911, #5912) Three follow-ups from the #5895/#5902 Redis wrapper security review: - #5907: parseChars() widened each byte to a UTF-16 char via (char) b instead of decoding it, sign-extending any byte >= 0x80 and mangling non-ASCII/binary bulk strings. Now reads into a right-sized byte[] and decodes once with the same charset replyToClient() encodes with. respondValue()'s bulk-length header had the mirror-image bug (String char count instead of UTF-8 byte count), which was previously masked because the old parseChars happened to produce 1 char per input byte; fixed alongside since a wrong header on the way out desyncs a client exactly like a truncated bulk string would. - #5911: the $ (bulk string) branch always called skipLF() after parseChars(), but a RESP2 null bulk string ($-1\r\n) has no trailing CRLF - it's a complete, self-terminated token. skipLF() then consumed the next token's leading byte, desyncing the parser. Mirrors the existing null/empty-array short-circuit. This makes a null command[0] reachable, which exposed an unguarded cmd.getClass() NPE outside any catch block in executeCommand(); guarded it. - #5912: the Redis listener never configured a socket read timeout, so an unauthenticated client that opens a connection and never sends anything (or trickles bytes arbitrarily slowly) held the connection thread open indefinitely - a slow-connection-exhaustion vector distinct from the unbounded-declared-size DoS #5902 already fixes. Now sets NETWORK_SOCKET_TIMEOUT as a pre-auth read timeout, mirroring BoltNetworkExecutor's handshake-timeout pattern, and lifts it back to infinite once authentication succeeds (an authenticated RESP connection is expected to sit idle between commands). Regression tests in RedisRespCorrectnessTest cover all three: a multi-byte UTF-8 SET/GET round trip through both Jedis and raw RESP, a null-bulk-string argument followed by AUTH/PING on the same connection to prove the parser stays in sync, an idle unauthenticated connection getting closed, and an idle authenticated connection NOT getting closed. * docs(#5911): clarify why every negative bulk length is treated as null Addresses a review comment on #5965: the RESP2 spec only defines -1 as the null bulk string, but rejecting other negative sizes would need its own protocol-error branch for no behavioral benefit over treating them the same as the one negative value that is defined. * fix(#5911): reject a null bulk-string command argument cleanly Code review on #5965 pointed out that the #5911 fix makes $-1 reachable as any array element, not just the command name - e.g. "SET key $-1" or "GET $-1". defaultBucket is a ConcurrentHashMap, whose put/get reject a null key or value outright, so those reached a raw NullPointerException deep inside setVariable/getVariable instead of one clear reply (still caught by executeCommand's generic catch, so not a crash/hang, just an unhelpful message). Reject any null argument uniformly, before dispatch, with a clean "-ERR Protocol error: unexpected null bulk string argument" instead of letting whichever handler touches it first fail differently depending on which map/method it happens to call. Also tags the two new idle-timeout tests @tag("slow") per CLAUDE.md (they wait on a lowered NETWORK_SOCKET_TIMEOUT plus a 1.5s sleep), matching the existing Issue5470BatchStreamStallIT precedent. * fix(#5912): re-arm the pre-auth idle timeout after a failed re-AUTH Code review on #5965 caught a state-machine gap: markAuthenticated() lifts the socket's read timeout to infinite on success, but a connection that authenticates once and then fails a *subsequent* AUTH/HELLO re-authentication attempt had authenticatedUser reset to null without the timeout being re-armed - leaving the connection logically unauthenticated but with an infinite read timeout, breaking the "unauthenticated implies bounded timeout" invariant #5912 relies on. Introduces markUnauthenticated(), used everywhere authenticatedUser was being set back to null, which resets the socket to the same bounded NETWORK_SOCKET_TIMEOUT the constructor arms pre-auth. * docs/test: address remaining review notes on #5965 Third code-review round on #5965 came back with no blocking issues; addresses the three cheap, low-priority points it raised: - Documents that the null-argument guard in executeCommand() is deliberately depth-1-only (a $-1 nested inside a multibulk argument still surfaces as a raw ClassCastException, an obscure and already-non-crashing edge case no handler exercises today). - Adds a TODO(perf) on respondValue()'s double UTF-8 encode (measure bytes for the header, then replyToClient() re-encodes the same text again), explaining why fixing it cleanly needs a larger change than this correctness fix warrants. - Adds a regression test for a non-(-1) negative bulk length ($-5), locking in that "every negative size is null" is a tested decision. * test: close the Jedis client in bulkStringWithMultibyteUtf8RoundTripsExactly Fourth code-review round on #5965 caught that this test was the only one in the file not using try-with-resources, leaking the client connection (and the server-side RedisNetworkExecutor thread, which now sits with an infinite read timeout post-auth per this PR's own fix). No functional bug, just inconsistent with the rest of the file. * docs: clarify swallowed SocketException and correct the Bolt comparison Fifth code-review round on #5965 came back with no blocking issues; addresses its two documentation notes: - Explains why swallowing SocketException in markAuthenticated()/ markUnauthenticated() is safe: setSoTimeout() only throws on an already-broken/closed socket, so there's no live connection left for the timeout state to matter on. - Corrects the constructor comment's "mirrors BoltNetworkExecutor" framing: Bolt only bounds its TLS-detection window, not its own subsequent HELLO/LOGON auth phase, so this actually goes further - it bounds the entire Redis pre-auth phase through AUTH/HELLO itself.
2 participants
Summary
RedisNetworkExecutor.parseNext()decoded RESP arrays recursively with no bound on nesting depth. A ~47 KB payload of deeply nested arrays (*1\r\nrepeated ~11,860 times) overflowed the connection thread's JVM stack with an uncaughtStackOverflowError, reachable pre-authentication by anyone who can open the Redis port, since the whole message must be parsed before theNOAUTHcheck runs.arraySize) was also unbounded: a single*2000000000\r\nheader started a parse loop the client could keep alive indefinitely by trickling bytes, tying up a connection thread.arcadedb.redis.maxMultiBulkDepth(default 32) andarcadedb.redis.maxMultiBulkLength(default 1,048,576, matching Redis' own hard cap on multibulk requests). A violation of either fails fast with a RESP-ERR Protocol error: ...reply, and the connection is closed rather than attempting to resynchronize on an untrusted stream position.Test plan
RedisProtocolLimitsTest(raw-socket, modeled onRedisAuthenticationTest) reproduces both issues against unpatched code (StackOverflowErrorfor the nesting case,SocketTimeoutException/hang for the oversized-length case) and passes against the fix.rediswmodule test suite passes (48 tests, 0 failures).mvn compile).Closes #5895
🤖 Generated with Claude Code