From: William Lallemand Date: Sat, 29 Aug 2026 09:49:09 +0000 (+0000) Subject: BUG/MINOR: payload: fix the cipher_len bound check in smp_client_hello_parse() X-Git-Tag: v3.5-dev6~40 X-Git-Url: https://git.kaiwu.me/?a=commitdiff_plain;h=98db20a83dd7be7bb1ea70e97b5e8e5b544ce9b0;p=haproxy.git BUG/MINOR: payload: fix the cipher_len bound check in smp_client_hello_parse() smp_client_hello_parse() validates the CipherSuite length of a TLS ClientHello with: ext_len > hs_len where is the number of ClientHello bytes remaining, counted from , which at that point still points at the 2-byte cipher_len field itself. The CLIENTHELLO_CIPHERSUITE exit path returns that very together with *len = cipher_len, and smp_fetch_ssl_cipherlist() then publishes over bytes. The cipher suites therefore have to fit *after* the length field, so the check must be: ext_len > hs_len - 2 Without this, a ClientHello announcing a cipher_len one or two bytes larger than what its handshake message can hold is accepted, and req.ssl_cipherlist returns up to 2 bytes read past the end of the ClientHello. When the ClientHello is the last thing in the buffer, those 2 bytes are leftovers of a previous connection, which then leak to whatever logs or routes on req.ssl_cipherlist: # 45-byte ClientHello announcing cipher_len = 10 for 8 cipher bytes cipherlist=130213031301009fcafe <- "cafe" sent after the TLS record cipherlist=130213031301009f4142 <- nothing sent after the ClientHello The CLIENTHELLO_EXTENSIONS path was not affected; it only survived by accident, because it re-checks "hs_len < 2" after subtracting 2 + ext_len. A reg-test exercising both a well-formed cipher_len and an over-announced one is added. This must be backported as far as 3.2, where req.ssl_cipherlist was introduced by commit fa063a9e7 ("MINOR: sample: add 4 new sample fetches for clienthello parsing"). Reported-by: kongr --- diff --git a/reg-tests/sample_fetches/ssl_client_hello.vtc b/reg-tests/sample_fetches/ssl_client_hello.vtc new file mode 100644 index 000000000..bbefcf5b7 --- /dev/null +++ b/reg-tests/sample_fetches/ssl_client_hello.vtc @@ -0,0 +1,85 @@ +varnishtest "TLS ClientHello parser: cipher_len must not exceed the ClientHello" +feature cmd "$HAPROXY_PROGRAM -cc 'version_atleast(3.2-dev11)'" + +# smp_client_hello_parse() validates the CipherSuite length with +# +# ext_len > hs_len +# +# where is the number of ClientHello bytes remaining *including* the +# 2-byte cipher_len field itself. The CLIENTHELLO_CIPHERSUITE exit path +# returns (pointing at cipher_len) together with *len = cipher_len, and +# smp_fetch_ssl_cipherlist() then publishes over bytes. +# The check must therefore be "ext_len > hs_len - 2". +# +# Without that, a ClientHello announcing cipher_len = 10 while only 8 bytes of +# cipher suites fit in the handshake message makes req.ssl_cipherlist return +# 10 bytes, the last 2 of which are read past the end of the ClientHello: +# either whatever the client appended behind it (c2 below), or, when the +# ClientHello is the last thing in the buffer, leftovers of a previous +# connection. + +feature ignore_unknown_macro + +syslog S1 -level info { + # c1: well-formed ClientHello, the 8 announced cipher bytes are returned + # and the 2 bytes trailing the TLS record are not. + recv + expect ~ "[^:\\[ ]\\[${h1_pid}\\]: cipherlist=130213031301009f$" + # c2: cipher_len announces 2 bytes more than the ClientHello holds, the + # ClientHello must be rejected rather than over-read. + recv + expect ~ "[^:\\[ ]\\[${h1_pid}\\]: cipherlist=-$" +} -start + +haproxy h1 -conf { + global + .if feature(THREAD) + thread-groups 1 + .endif + nbthread 1 + + defaults + mode tcp + timeout connect "${HAPROXY_TEST_TIMEOUT-5s}" + timeout client "${HAPROXY_TEST_TIMEOUT-5s}" + timeout server "${HAPROXY_TEST_TIMEOUT-5s}" + + frontend fe1 + bind "fd@${fe1}" + log ${S1_addr}:${S1_port} local0 + log-format "cipherlist=%[var(txn.cl)]" + + tcp-request inspect-delay 100ms + tcp-request content set-var(txn.cl) req.ssl_cipherlist,hex,lower + tcp-request content reject +} -start + +# Both ClientHellos must be sent in a single write: HAProxy rejects the +# connection as soon as the ruleset is evaluated, so a second write would +# hit a closed socket. +# +# Layout of the 56 bytes below: +# 16 03 01 00 31 TLS record: handshake, TLSv1.0, length = 49 +# 01 00 00 2d Handshake: ClientHello, length = 45 +# 03 03 client_version +# 00 .. 1f Random[32] +# 00 session_id_len = 0 +# 00 08 / 00 0a cipher_len +# 13 02 .. 00 9f 8 bytes of cipher suites, end of the ClientHello +# ca fe trailing garbage, outside of the TLS record; it must +# never show up in req.ssl_cipherlist + +# Well-formed ClientHello: cipher_len = 8, 8 bytes of cipher suites. +client c1 -connect ${h1_fe1_sock} { + sendhex "1603010031 0100002d 0303 000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f 00 0008 130213031301009f cafe" + delay 0.3 +} -run + +# Same ClientHello, but cipher_len announces 10 bytes while only 8 fit in the +# 45-byte handshake message. +client c2 -connect ${h1_fe1_sock} { + sendhex "1603010031 0100002d 0303 000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f 00 000a 130213031301009f cafe" + delay 0.3 +} -run + +syslog S1 -wait diff --git a/src/payload.c b/src/payload.c index fe88fea9b..aef51f236 100644 --- a/src/payload.c +++ b/src/payload.c @@ -152,7 +152,7 @@ smp_client_hello_parse( struct sample *smp, enum client_hello_type type, unsigne if (hs_len < 4 || /* minimum one cipher */ (ext_len = (data[0] << 8) + data[1]) < 2 || /* minimum 2 bytes for a cipher */ - ext_len > hs_len) + ext_len > hs_len - 2) /* ciphers must fit after cipher_len */ goto not_ssl_hello; /* Jump to the compression methods. For fetching cipher list this processing is not required. */