Require 3 elements on the PUSH arm of the subscribe callback path - #1366
Require 3 elements on the PUSH arm of the subscribe callback path#1366afonsojanu wants to merge 1 commit into
Conversation
__redisGetSubscribeCallback routed any REDIS_REPLY_PUSH straight into element[1] and (for unsubscribe) element[2] without checking reply->elements first, unlike the sibling ARRAY arm right next to it, which already requires elements >= 3. A conforming Redis server always sends 3 or 4 elements on a pub/sub push, but a proxy, an incomplete RESP3 implementation, or a hostile peer can send fewer. A one-element PUSH whose text reads as "subscribe" still passes redisIsSubscribeReply's shape check (it only looks at element[0]), so it lands here and element[1] reads one pointer past the end of the reply's element array. Under ASan this is a heap-buffer-overflow; without it, the read is into whatever memory happens to follow the allocation, which segfaults in practice. Require the same elements >= 3 shape the ARRAY arm and the function's own comment already document, on the PUSH arm as well. A short PUSH now falls through to the same path already used for a regular command reply arriving in subscribed mode, instead of an out-of-bounds read. Added a regression test that drives a real redisAsyncContext through a fake loopback peer sending exactly this one-element PUSH after a SUBSCRIBE, confirmed to crash the process without the fix and pass cleanly with it, under both a plain build and one with -fsanitize=address.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6de25a0994
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * https://redis.io/docs/latest/develop/interact/pubsub/#format-of-pushed-messages */ | ||
| if ((reply->type == REDIS_REPLY_ARRAY && !(c->flags & REDIS_SUPPORTS_PUSH) && reply->elements >= 3) || | ||
| reply->type == REDIS_REPLY_PUSH) { | ||
| (reply->type == REDIS_REPLY_PUSH && reply->elements >= 3)) { |
There was a problem hiding this comment.
When an undersized pub/sub PUSH arrives after SUBSCRIBE while an ordinary command is pending, this condition falls through to the else branch, which removes the next callback from ac->sub.replies and invokes it with the malformed PUSH. The real command response then consumes a later callback or is discarded, desynchronizing the callback queue. This is possible because redisIsSubscribeReply classifies the PUSH using only element 0, while ordinary commands issued in subscribed mode are queued in ac->sub.replies; malformed PUSH replies should instead be discarded or treated as protocol errors without shifting that queue.
Useful? React with 👍 / 👎.
1 participant
Fixes #1359.
__redisGetSubscribeCallback routes any REDIS_REPLY_PUSH straight into element[1] and (for unsubscribe) element[2] without checking reply->elements first, unlike the ARRAY arm of the same condition, which already requires elements >= 3.
A conforming Redis server always sends 3 or 4 elements on a pub/sub push, but a proxy, an incomplete RESP3 implementation, or a hostile peer can send fewer. A one-element PUSH whose text reads as "subscribe" still passes redisIsSubscribeReply's shape check (it only looks at element[0]), so it lands in this function and element[1] reads one pointer past the end of the reply's element array. Under ASan this is a heap-buffer-overflow; without it, the read is into whatever memory happens to follow the allocation, which segfaults in practice.
The fix requires the same elements >= 3 shape the ARRAY arm and the function's own comment already document, on the PUSH arm as well. A short PUSH now falls through to the same path already used for a regular command reply arriving in subscribed mode, instead of an out-of-bounds read.
Added a regression test that drives a real redisAsyncContext through a fake loopback peer sending exactly this one-element PUSH after a SUBSCRIBE. Confirmed it crashes the process without the fix and passes cleanly with it, both in a plain build and under -fsanitize=address. Also confirmed the actual reported PoC from the issue no longer reproduces.
Note
High Risk
Closes an OOB read on untrusted wire data in async subscribe handling; the fix is a small guard with a targeted regression test.
Overview
Fixes a heap out-of-bounds read in async pub/sub when a peer sends a malformed RESP3
PUSHthat looks like a subscribe notification but has too few elements.__redisGetSubscribeCallbacknow requiresreply->elements >= 3forREDIS_REPLY_PUSH, matching the existing ARRAY branch and Redis pub/sub push shape. Undersized pushes are handled like a normal in-subscribe command reply instead of indexingelement[1]/element[2]past the array.Adds
test_pubsub_short_push_no_oob, which uses a loopback TCP fake server to send a one-elementsubscribePUSH afterSUBSCRIBEand asserts the async context stays healthy (regression for #1359).Reviewed by Cursor Bugbot for commit 6de25a0. Bugbot is set up for automated code reviews on this repo. Configure here.