diff --git a/src/cluster_legacy.c b/src/cluster_legacy.c index a3d8dccff2..f494443c50 100644 --- a/src/cluster_legacy.c +++ b/src/cluster_legacy.c @@ -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); } diff --git a/tests/support/cluster_util.tcl b/tests/support/cluster_util.tcl index d2a8633e07..1457565bea 100644 --- a/tests/support/cluster_util.tcl +++ b/tests/support/cluster_util.tcl @@ -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 } diff --git a/tests/unit/cluster/hostnames.tcl b/tests/unit/cluster/hostnames.tcl index 223622864c..0a1c483b16 100644 --- a/tests/unit/cluster/hostnames.tcl +++ b/tests/unit/cluster/hostnames.tcl @@ -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 +} }