fix: guard RESP parsers against RecursionError on deeply nested replies (#4116) - #4144
fix: guard RESP parsers against RecursionError on deeply nested replies (#4116)#4144C1-BA-B1-F3 wants to merge 2 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| return self.execute_command("BLPOP", *keys) | ||
| return self.execute_command( | ||
| "BLPOP", *keys, _blocking_timeout=timeout | ||
| ) |
There was a problem hiding this comment.
Zero blocking timeout breaks wait
Medium Severity
Blocking list commands pass _blocking_timeout=timeout where Redis uses 0 to mean block indefinitely. That value is forwarded as the socket read timeout, which on async clients becomes an immediate async_timeout(0) and on sync clients sets a zero-second/non-blocking socket read, so indefinite blocking commands fail instead of waiting for data.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 498b42d. Configure here.
|
I think this PR as well is poluted with additional changes meant to be added to other fixes. Please clean up. |
The pure-Python RESP parser path uses recursive parsing for nested aggregate replies (arrays, sets, maps, push). A malicious server or compromised proxy can inject deeply nested RESP replies to trigger RecursionError, causing client-side denial of service. Add MAX_NESTING_DEPTH (100) to all four parser classes: - _RESP2Parser / _AsyncRESP2Parser - _RESP3Parser / _AsyncRESP3Parser When nesting depth is exceeded, raise InvalidResponse instead of allowing unbounded recursion. Fixes redis#4116
498b42d to
32859c2
Compare
|
Thanks @petyaslavova! Branch cleaned up — now only contains the RESP RecursionError guard changes. The unrelated commits have been removed. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Reviewed by Cursor Bugbot for commit 32859c2. Configure here.
| return response | ||
|
|
||
| async def _read_response( | ||
| self, disable_decoding: bool = False, push_request: bool = False |
There was a problem hiding this comment.
Push continuation resets nesting depth
Medium Severity
After handling a RESP3 push reply, the follow-up _read_response call omits _depth, so nesting accounting restarts at zero. Nested aggregate parsing elsewhere increments _depth, so a hostile reply can combine deep nesting with pushes and grow recursion beyond the intended MAX_NESTING_DEPTH cap, weakening the DoS fix.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 32859c2. Configure here.
|
Thanks for cleaning up the branch. The approach is right and the tests exercise the real parser path via SocketBuffer, which is exactly what we want for a security regression. Before merging, please add a non-regression test for a burst of top-level RESP3 push frames so we prove the guard doesn't falsely trip on legitimate push traffic (you can borrow the scenario from #4200). One thing for a follow-up (not required here): the push continuation in resp3.py recurses without incrementing depth, so a flood of consecutive push frames is a separate recursion vector outside this issue's scope. Once the push-burst test is in and CI (ruff + tests) is green, this should be ready for another review. |
| response = resp_dict | ||
| # push response | ||
| elif byte == b">": | ||
| if _depth >= self.MAX_NESTING_DEPTH: |
There was a problem hiding this comment.
the push branch guards the recursion into the elements, but the tail call right below it (the "not a push_request, go read the actual reply" one) doesn't forward _depth, so it restarts at 0. a server/proxy dribbling out-of-band push frames back-to-back gets one stack frame per frame with the counter reset every time, which is the same RecursionError this PR is closing. same in _AsyncRESP3Parser.
3 participants
Problem
The pure-Python RESP parser path uses recursive parsing for nested aggregate replies (arrays, sets, maps, push). A malicious Redis server, compromised proxy, or on-path actor can inject deeply nested RESP replies to trigger
RecursionError, causing client-side denial of service.Reproduction (from #4116):
Fix
Add
MAX_NESTING_DEPTH = 100to all four pure-Python parser classes:_RESP2Parser/_AsyncRESP2Parser(resp2.py)_RESP3Parser/_AsyncRESP3Parser(resp3.py)When nesting depth exceeds the limit, raise
InvalidResponseinstead of allowing unbounded recursion. The depth limit of 100 is generous for legitimate Redis workloads while preventing stack exhaustion.All aggregate types are guarded:
*(array),~(set),%(map),>(push).Tests
12 new tests covering:
InvalidResponseInvalidResponse, notRecursionError~), map (%)Fixes #4116
Note
Medium Risk
Touches core wire-protocol parsing on every connection; legitimate deeply nested replies above 100 levels would now fail, but the limit is unlikely for normal Redis workloads.
Overview
Adds a 100-level nesting cap on pure-Python RESP2/RESP3 parsers (sync and async) so malicious or pathological aggregate replies no longer blow the stack with
RecursionError._read_responsenow tracks_depthand raisesInvalidResponsewhen depth exceedsMAX_NESTING_DEPTH. RESP2 guards nested arrays (*); RESP3 also guards sets (~), maps (%), and push (>), with depth passed through recursive child reads.New tests in
test_resp_recursion.pycover normal and at-limit nesting, over-limit and very deep (5000-level) payloads, and all four parser classes plus RESP3 set/map types.Reviewed by Cursor Bugbot for commit b4915b5. Bugbot is set up for automated code reviews on this repo. Configure here.