mirror of
https://github.com/redis/redis.git
synced 2026-06-09 00:33:08 -04:00
Reject cluster bus PING extensions with missing null terminator (#15263)
## Summary The cluster bus PING/PONG/MEET packet parser validated extension padding and total length but never checked that string-carrying extensions are properly null-terminated, allowing a crafted packet to trigger out-of-bounds reads when the payload is later consumed as a C string. 1. **Null-termination check for hostname and human-nodename extensions (`cluster_legacy.c`)** Added a check inside the existing extension-validation loop in `clusterProcessPacket`: for `CLUSTERMSG_EXT_TYPE_HOSTNAME` and `CLUSTERMSG_EXT_TYPE_HUMAN_NODENAME` extension types, it verifies that the data portion is non-empty (`datalen > 0`) and that the last byte is `'\0'`. Packets failing this check are rejected with a warning log and an early return, the same way other malformed-extension cases are handled. 2. **Test (`hostnames.tcl`)** A new test exercises the rejection path by constructing a raw cluster-bus PING packet with a 32-byte hostname extension that contains no `'\0'`, sending it directly to a node's bus port, and verifying the packet is dropped (warning logged, hostname not updated in `CLUSTER NODES`). Two helper procs (`build_cluster_bus_ping` and `build_hostname_extension`) build the binary packet from scratch in Tcl, allowing fine-grained control over extension contents without needing a modified Redis sender.
This commit is contained in:
Sergei Georgiev
GitHub
parent
bff152c7ac
commit
37894faeea
3 changed files with 142 additions and 23 deletions
|
|
@ -222,7 +222,7 @@ sds auxShardIdGetter(clusterNode *n, sds s) {
|
|||
}
|
||||
|
||||
int auxShardIdPresent(clusterNode *n) {
|
||||
return strlen(n->shard_id);
|
||||
return strnlen(n->shard_id, CLUSTER_NAMELEN);
|
||||
}
|
||||
|
||||
int auxHumanNodenameSetter(clusterNode *n, void *value, int length) {
|
||||
|
|
@ -2820,7 +2820,7 @@ int clusterProcessPacket(clusterLink *link) {
|
|||
clusterMsgPingExt *ext = getInitialPingExt(hdr, count);
|
||||
while (extensions--) {
|
||||
uint16_t extlen = getPingExtLength(ext);
|
||||
if (extlen % 8 != 0) {
|
||||
if (extlen < sizeof(clusterMsgPingExt) || extlen % 8 != 0) {
|
||||
serverLog(LL_WARNING, "Received a %s packet without proper padding (%d bytes)",
|
||||
clusterGetMessageTypeString(type), (int) extlen);
|
||||
return 1;
|
||||
|
|
@ -2831,6 +2831,42 @@ int clusterProcessPacket(clusterLink *link) {
|
|||
(unsigned long long) totlen);
|
||||
return 1;
|
||||
}
|
||||
uint16_t exttype = ntohs(ext->type);
|
||||
uint32_t datalen = extlen - sizeof(clusterMsgPingExt);
|
||||
if (exttype == CLUSTERMSG_EXT_TYPE_HOSTNAME ||
|
||||
exttype == CLUSTERMSG_EXT_TYPE_HUMAN_NODENAME) {
|
||||
char *str = (char *) ext->ext;
|
||||
if (datalen == 0 || str[datalen - 1] != '\0') {
|
||||
serverLog(LL_WARNING,
|
||||
"Received %s packet with missing null terminator in extension type %d",
|
||||
clusterGetMessageTypeString(type), exttype);
|
||||
return 1;
|
||||
}
|
||||
} else if (exttype == CLUSTERMSG_EXT_TYPE_FORGOTTEN_NODE) {
|
||||
if (datalen < sizeof(clusterMsgPingExtForgottenNode)) {
|
||||
serverLog(LL_WARNING,
|
||||
"Received %s packet with truncated extension type %d",
|
||||
clusterGetMessageTypeString(type), exttype);
|
||||
return 1;
|
||||
}
|
||||
} else if (exttype == CLUSTERMSG_EXT_TYPE_SHARDID) {
|
||||
char *str = (char *) ext->ext;
|
||||
if (datalen < sizeof(clusterMsgPingExtShardId) ||
|
||||
verifyClusterNodeId(str, CLUSTER_NAMELEN) != C_OK)
|
||||
{
|
||||
serverLog(LL_WARNING,
|
||||
"Received %s packet with invalid shard id in extension type %d",
|
||||
clusterGetMessageTypeString(type), exttype);
|
||||
return 1;
|
||||
}
|
||||
} else if (exttype == CLUSTERMSG_EXT_TYPE_INTERNALSECRET) {
|
||||
if (datalen < sizeof(clusterMsgPingExtInternalSecret)) {
|
||||
serverLog(LL_WARNING,
|
||||
"Received %s packet with truncated extension type %d",
|
||||
clusterGetMessageTypeString(type), exttype);
|
||||
return 1;
|
||||
}
|
||||
}
|
||||
explen += extlen;
|
||||
ext = getNextPingExt(ext);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -263,9 +263,14 @@ proc check_cluster_node_mark {flag ref_node_index instance_id_to_check} {
|
|||
fail "Unable to find instance id in cluster nodes. ID: $instance_id_to_check"
|
||||
}
|
||||
|
||||
# Build the 2256-byte cluster bus header (CLUSTERMSG_MIN_LEN) common to all
|
||||
# message types. Only the type field and the data that follows differ.
|
||||
proc build_cluster_bus_header {sender_name sender_port sender_cport msg_type totlen} {
|
||||
# Build the 2256-byte cluster bus header (CLUSTERMSG_MIN_LEN) shared by all
|
||||
# message types. The sender identity, type, length, and the
|
||||
# extensions/flags/mflags0 fields are supplied by the caller; everything else
|
||||
# (epochs, slots, etc.) is fixed boilerplate. The extensions/flags/mflags0
|
||||
# fields default to 0 for callers that don't need them (e.g. a PING carrying
|
||||
# extension data overrides them). Message-type-specific payload is appended
|
||||
# after this header.
|
||||
proc build_cluster_bus_header {sender_name sender_port sender_cport msg_type totlen {num_extensions 0} {flags 0} {mflags0 0}} {
|
||||
set CLUSTER_NAMELEN 40
|
||||
set NET_IP_STR_LEN 46
|
||||
|
||||
|
|
@ -278,24 +283,24 @@ proc build_cluster_bus_header {sender_name sender_port sender_cport msg_type tot
|
|||
set hdr ""
|
||||
append hdr "RCmb"
|
||||
append hdr [binary format I $totlen]
|
||||
append hdr [binary format S 1] ;# ver
|
||||
append hdr [binary format S $sender_port] ;# port
|
||||
append hdr [binary format S $msg_type] ;# type
|
||||
append hdr [binary format S 0] ;# count
|
||||
append hdr [binary format W 1] ;# currentEpoch
|
||||
append hdr [binary format W 2] ;# configEpoch
|
||||
append hdr [binary format W 0] ;# offset
|
||||
append hdr $sender_padded ;# sender
|
||||
append hdr $myslots ;# myslots
|
||||
append hdr $slaveof ;# slaveof
|
||||
append hdr $myip ;# myip
|
||||
append hdr [binary format S 0] ;# extensions
|
||||
append hdr $notused1 ;# notused1
|
||||
append hdr [binary format S 0] ;# pport
|
||||
append hdr [binary format S $sender_cport] ;# cport
|
||||
append hdr [binary format S 0] ;# flags
|
||||
append hdr [binary format c 0] ;# state
|
||||
append hdr [binary format ccc 0 0 0] ;# mflags
|
||||
append hdr [binary format S 1] ;# ver
|
||||
append hdr [binary format S $sender_port] ;# port
|
||||
append hdr [binary format S $msg_type] ;# type
|
||||
append hdr [binary format S 0] ;# count
|
||||
append hdr [binary format W 1] ;# currentEpoch
|
||||
append hdr [binary format W 2] ;# configEpoch
|
||||
append hdr [binary format W 0] ;# offset
|
||||
append hdr $sender_padded ;# sender
|
||||
append hdr $myslots ;# myslots
|
||||
append hdr $slaveof ;# slaveof
|
||||
append hdr $myip ;# myip
|
||||
append hdr [binary format S $num_extensions] ;# extensions
|
||||
append hdr $notused1 ;# notused1
|
||||
append hdr [binary format S 0] ;# pport
|
||||
append hdr [binary format S $sender_cport] ;# cport
|
||||
append hdr [binary format S $flags] ;# flags
|
||||
append hdr [binary format c 0] ;# state
|
||||
append hdr [binary format ccc $mflags0 0 0] ;# mflags
|
||||
|
||||
return $hdr
|
||||
}
|
||||
|
|
|
|||
|
|
@ -16,6 +16,36 @@ proc get_slot_field {slot_output shard_id node_id attrib_id} {
|
|||
return [lindex [lindex [lindex $slot_output $shard_id] $node_id] $attrib_id]
|
||||
}
|
||||
|
||||
proc build_cluster_bus_ping {sender_name sender_port sender_cport extensions_data} {
|
||||
set CLUSTERMSG_TYPE_PING 0
|
||||
set CLUSTERMSG_FLAG0_EXT_DATA 0x04
|
||||
set CLUSTER_NODE_MASTER 1
|
||||
|
||||
set num_extensions [expr {[string length $extensions_data] > 0 ? 1 : 0}]
|
||||
set mflags0 [expr {$num_extensions > 0 ? $CLUSTERMSG_FLAG0_EXT_DATA : 0}]
|
||||
set totlen [expr {2256 + [string length $extensions_data]}]
|
||||
|
||||
set hdr [build_cluster_bus_header $sender_name $sender_port $sender_cport \
|
||||
$CLUSTERMSG_TYPE_PING $totlen $num_extensions $CLUSTER_NODE_MASTER $mflags0]
|
||||
append hdr $extensions_data
|
||||
return $hdr
|
||||
}
|
||||
|
||||
proc build_hostname_extension {hostname_bytes} {
|
||||
set ext_header_size 8
|
||||
set total_ext_len [expr {$ext_header_size + [string length $hostname_bytes]}]
|
||||
set padded_len [expr {(($total_ext_len + 7) / 8) * 8}]
|
||||
set padding_len [expr {$padded_len - $total_ext_len}]
|
||||
|
||||
set ext ""
|
||||
append ext [binary format I $padded_len]
|
||||
append ext [binary format S 0]
|
||||
append ext [binary format S 0]
|
||||
append ext $hostname_bytes
|
||||
append ext [string repeat \x00 $padding_len]
|
||||
return $ext
|
||||
}
|
||||
|
||||
# Start a cluster with 3 masters and 4 replicas.
|
||||
# These tests rely on specific node ordering, so make sure no node fails over.
|
||||
start_cluster 3 4 {tags {external:skip cluster} overrides {cluster-replica-no-failover yes}} {
|
||||
|
|
@ -227,4 +257,52 @@ test "Test hostname validation" {
|
|||
# Note this isn't a valid hostname, but it passes our internal validation
|
||||
R 0 config set cluster-announce-hostname "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-."
|
||||
}
|
||||
|
||||
test "PING with hostname extension missing null terminator is rejected" {
|
||||
for {set j 0} {$j < [llength $::servers]} {incr j} {
|
||||
R $j config set cluster-announce-hostname ""
|
||||
}
|
||||
wait_for_condition 50 100 {
|
||||
[are_hostnames_propagated ""] eq 1
|
||||
} else {
|
||||
fail "hostnames were not cleared"
|
||||
}
|
||||
|
||||
set node1_id [R 1 CLUSTER MYID]
|
||||
set target_host [srv 0 host]
|
||||
set target_bus_port [expr {[srv 0 port] + 10000}]
|
||||
set sender_port [srv -1 port]
|
||||
set sender_cport [expr {$sender_port + 10000}]
|
||||
|
||||
# Freeze node 1 so its real PINGs cannot overwrite the injected hostname.
|
||||
set node1_pid [srv -1 pid]
|
||||
pause_process $node1_pid
|
||||
|
||||
set payload_len 32
|
||||
set bad_hostname [string repeat A $payload_len]
|
||||
set bad_ext [build_hostname_extension $bad_hostname]
|
||||
set bad_packet [build_cluster_bus_ping $node1_id $sender_port $sender_cport $bad_ext]
|
||||
|
||||
# Record the log position before injecting the bad packet.
|
||||
set loglines [count_log_lines 0]
|
||||
|
||||
if {$::tls} {
|
||||
set fd [::tls::socket \
|
||||
-cafile "$::tlsdir/ca.crt" \
|
||||
-certfile "$::tlsdir/client.crt" \
|
||||
-keyfile "$::tlsdir/client.key" \
|
||||
$target_host $target_bus_port]
|
||||
} else {
|
||||
set fd [socket $target_host $target_bus_port]
|
||||
}
|
||||
fconfigure $fd -translation binary -buffering full
|
||||
puts -nonewline $fd $bad_packet
|
||||
flush $fd
|
||||
|
||||
# Verify the server logged the proper rejection message.
|
||||
wait_for_log_messages 0 {"*missing null terminator*"} $loglines 50 100
|
||||
|
||||
close $fd
|
||||
resume_process $node1_pid
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue