Validate PUBLISH and MODULE payload lengths against packet size - #3972
Merged
madolson merged 2 commits intoAug 11, 2026
Merged
Conversation
clusterIsValidPacket() added the packet-supplied 32-bit channel, message, and module payload lengths into the 32-bit explen without bounds checks. A packet declaring e.g. channel_len 0xffffffff wraps explen back down to the header size and passes the totlen == explen check, after which the publish handler reads past the receive buffer and crashes the node. Check each declared length against the remaining packet space before adding it, the same way the gossip and extension data are already validated for PING/PONG/MEET packets in this function. Add a regression test that sends a forged PUBLISH with a wrapped length and confirms the node survives. Refs trailofbits/ptp-valkey#7 Signed-off-by: Tjaden Hess <tjade273@gmail.com>
📝 WalkthroughWalkthroughThis PR updates ChangesCluster packet validation hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://to.zgwd.com/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/cluster/packet.tcl (1)
189-192: ⚡ Quick winAvoid hard-coding wrapped
totlen; derive it from the forged packet size.Using literal
2264makes this regression fragile if cluster header/layout changes. Compute it fromstring length $packetso the test keeps validating the same overflow class without coupling to one struct size.Suggested tweak
- # Set totlen to the wrapped value so it matches the computed explen. - set packet [string replace $packet 4 7 [binary format I 2264]] - assert_equal 2264 [string length $packet] + # Set totlen to the wrapped value so it matches the computed explen. + set wrapped_totlen [string length $packet] + set packet [string replace $packet 4 7 [binary format I $wrapped_totlen]] + assert_equal $wrapped_totlen [string length $packet]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/cluster/packet.tcl` around lines 189 - 192, The test currently hardcodes totlen as 2264 in the string replace call and assertion; instead build the forged packet with a placeholder totlen (e.g., binary format I 0), compute the actual length with explen=[string length $packet], then replace bytes 4..7 using [binary format I $explen] and assert_equal $explen [string length $packet]; update the string replace that writes the totlen and the subsequent assert_equal to use the computed explen variable so the test derives totlen from packet size (referencing the packet variable and the string replace call).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cluster_legacy.c`:
- Around line 3794-3814: The code reads module_len from msg->data.module.msg.len
(via toClusterMsgLight/toClusterMsg) before confirming the fixed module header
bytes are within totlen, which can read past the received buffer for truncated
packets; move the ntohl(...) reads for module_len to after you validate explen
(the fixed header size computed from sizeof(clusterMsgLight)/sizeof(clusterMsg)
and sizeof(clusterMsgModule) - 3) is <= totlen so the header bytes are present,
i.e. first compute explen and verify if (totlen < explen) return error, then
safely access module_len using ntohl from the appropriate message view
(clusterMsgLight or clusterMsg), and finally check (totlen - explen) <
module_len before adding module_len to explen.
---
Nitpick comments:
In `@tests/unit/cluster/packet.tcl`:
- Around line 189-192: The test currently hardcodes totlen as 2264 in the string
replace call and assertion; instead build the forged packet with a placeholder
totlen (e.g., binary format I 0), compute the actual length with explen=[string
length $packet], then replace bytes 4..7 using [binary format I $explen] and
assert_equal $explen [string length $packet]; update the string replace that
writes the totlen and the subsequent assert_equal to use the computed explen
variable so the test derives totlen from packet size (referencing the packet
variable and the string replace call).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a971a48-42a2-4f58-a078-0c50e27bb985
📒 Files selected for processing (2)
src/cluster_legacy.ctests/unit/cluster/packet.tcl
Comment on lines
+3794
to
3814
| uint32_t module_len; | ||
| if (is_light) { | ||
| clusterMsgLight *msg_light = toClusterMsgLight(link->rcvbuf); | ||
| explen = sizeof(clusterMsgLight) - sizeof(union clusterMsgData); | ||
| explen += sizeof(clusterMsgModule) - 3 + ntohl(msg_light->data.module.msg.len); | ||
| module_len = ntohl(msg_light->data.module.msg.len); | ||
| } else { | ||
| clusterMsg *msg = toClusterMsg(link->rcvbuf); | ||
| explen = sizeof(clusterMsg) - sizeof(union clusterMsgData); | ||
| explen += sizeof(clusterMsgModule) - 3 + ntohl(msg->data.module.msg.len); | ||
| module_len = ntohl(msg->data.module.msg.len); | ||
| } | ||
| explen += sizeof(clusterMsgModule) - 3; | ||
| /* The module payload length comes from the packet. Make sure it fits in | ||
| * the remaining space before adding it, so explen can't overflow. */ | ||
| if (totlen < explen || (totlen - explen) < module_len) { | ||
| serverLog(LL_WARNING, | ||
| "Received invalid %s packet with module payload length that exceeds total packet length (%lld)", | ||
| clusterGetMessageTypeString(type), (unsigned long long)totlen); | ||
| return 0; | ||
| } | ||
| explen += module_len; | ||
| } else { |
There was a problem hiding this comment.
Defer module_len read until fixed header length is validated.
On Line 3798 and Line 3802, module_len is read before verifying the fixed module header bytes are inside totlen. For truncated packets, this reads beyond the declared packet payload boundary (from stale rcvbuf bytes). Validate fixed bytes first, then parse module_len.
💡 Suggested patch
- } else if (type == CLUSTERMSG_TYPE_MODULE) {
- uint32_t module_len;
+ } else if (type == CLUSTERMSG_TYPE_MODULE) {
+ uint32_t module_len;
+ clusterMsgModule *module_data;
if (is_light) {
clusterMsgLight *msg_light = toClusterMsgLight(link->rcvbuf);
explen = sizeof(clusterMsgLight) - sizeof(union clusterMsgData);
- module_len = ntohl(msg_light->data.module.msg.len);
+ module_data = &msg_light->data.module.msg;
} else {
clusterMsg *msg = toClusterMsg(link->rcvbuf);
explen = sizeof(clusterMsg) - sizeof(union clusterMsgData);
- module_len = ntohl(msg->data.module.msg.len);
+ module_data = &msg->data.module.msg;
}
explen += sizeof(clusterMsgModule) - 3;
+ if (totlen < explen) {
+ serverLog(LL_WARNING,
+ "Received invalid %s packet with module header that exceeds total packet length (%lld)",
+ clusterGetMessageTypeString(type), (unsigned long long)totlen);
+ return 0;
+ }
+ module_len = ntohl(module_data->len);
/* The module payload length comes from the packet. Make sure it fits in
* the remaining space before adding it, so explen can't overflow. */
- if (totlen < explen || (totlen - explen) < module_len) {
+ if ((totlen - explen) < module_len) {
serverLog(LL_WARNING,
"Received invalid %s packet with module payload length that exceeds total packet length (%lld)",
clusterGetMessageTypeString(type), (unsigned long long)totlen);
return 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| uint32_t module_len; | |
| if (is_light) { | |
| clusterMsgLight *msg_light = toClusterMsgLight(link->rcvbuf); | |
| explen = sizeof(clusterMsgLight) - sizeof(union clusterMsgData); | |
| explen += sizeof(clusterMsgModule) - 3 + ntohl(msg_light->data.module.msg.len); | |
| module_len = ntohl(msg_light->data.module.msg.len); | |
| } else { | |
| clusterMsg *msg = toClusterMsg(link->rcvbuf); | |
| explen = sizeof(clusterMsg) - sizeof(union clusterMsgData); | |
| explen += sizeof(clusterMsgModule) - 3 + ntohl(msg->data.module.msg.len); | |
| module_len = ntohl(msg->data.module.msg.len); | |
| } | |
| explen += sizeof(clusterMsgModule) - 3; | |
| /* The module payload length comes from the packet. Make sure it fits in | |
| * the remaining space before adding it, so explen can't overflow. */ | |
| if (totlen < explen || (totlen - explen) < module_len) { | |
| serverLog(LL_WARNING, | |
| "Received invalid %s packet with module payload length that exceeds total packet length (%lld)", | |
| clusterGetMessageTypeString(type), (unsigned long long)totlen); | |
| return 0; | |
| } | |
| explen += module_len; | |
| } else { | |
| uint32_t module_len; | |
| clusterMsgModule *module_data; | |
| if (is_light) { | |
| clusterMsgLight *msg_light = toClusterMsgLight(link->rcvbuf); | |
| explen = sizeof(clusterMsgLight) - sizeof(union clusterMsgData); | |
| module_data = &msg_light->data.module.msg; | |
| } else { | |
| clusterMsg *msg = toClusterMsg(link->rcvbuf); | |
| explen = sizeof(clusterMsg) - sizeof(union clusterMsgData); | |
| module_data = &msg->data.module.msg; | |
| } | |
| explen += sizeof(clusterMsgModule) - 3; | |
| if (totlen < explen) { | |
| serverLog(LL_WARNING, | |
| "Received invalid %s packet with module header that exceeds total packet length (%lld)", | |
| clusterGetMessageTypeString(type), (unsigned long long)totlen); | |
| return 0; | |
| } | |
| module_len = ntohl(module_data->len); | |
| /* The module payload length comes from the packet. Make sure it fits in | |
| * the remaining space before adding it, so explen can't overflow. */ | |
| if ((totlen - explen) < module_len) { | |
| serverLog(LL_WARNING, | |
| "Received invalid %s packet with module payload length that exceeds total packet length (%lld)", | |
| clusterGetMessageTypeString(type), (unsigned long long)totlen); | |
| return 0; | |
| } | |
| explen += module_len; | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cluster_legacy.c` around lines 3794 - 3814, The code reads module_len
from msg->data.module.msg.len (via toClusterMsgLight/toClusterMsg) before
confirming the fixed module header bytes are within totlen, which can read past
the received buffer for truncated packets; move the ntohl(...) reads for
module_len to after you validate explen (the fixed header size computed from
sizeof(clusterMsgLight)/sizeof(clusterMsg) and sizeof(clusterMsgModule) - 3) is
<= totlen so the header bytes are present, i.e. first compute explen and verify
if (totlen < explen) return error, then safely access module_len using ntohl
from the appropriate message view (clusterMsgLight or clusterMsg), and finally
check (totlen - explen) < module_len before adding module_len to explen.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #3972 +/- ##
============================================
- Coverage 78.55% 78.39% -0.16%
============================================
Files 166 166
Lines 88272 88285 +13
============================================
- Hits 69341 69215 -126
- Misses 18931 19070 +139
🚀 New features to boost your workflow:
|
Problem
clusterIsValidPacket()adds the packet-supplied 32-bitchannel_len,message_len, and module payload lengths into the 32-bitexplenwith no bounds checks. A PUBLISH packet declaringchannel_len = 0xffffffffandmessage_len = 1wrapsexplenback down to the header size, so it passes thetotlen == explencompleteness check.clusterProcessPublishPacket()then callscreateStringObject((char *)publish_data->bulk_data, channel_len)with a ~4 GB length, reading far past the receive buffer and crashing the node.The PING/PONG/MEET branch of the same function already validates its variable-length gossip and extension data against the remaining packet space; the PUBLISH/PUBLISHSHARD and MODULE branches did not. CVE-2026-21863 hardened the gossip/extension path but left these branches unchanged.
Fix
Compute the fixed part of
explenfirst, then check each packet-supplied length against the remaining space (totlen - explen) before adding it, the same way the gossip and extension data are validated.Testing
Added a regression test in
tests/unit/cluster/packet.tclthat sends a forged PUBLISH withchannel_len = 0xffffffffand a wrappedtotlen, with a subscriber connected so the decode path runs.I verified that it crashes the node on pre-fix code (SIGBUS in the publish handler) and passes after the fix.