From: William Lallemand Date: Sat, 29 Aug 2026 10:05:33 +0000 (+0000) Subject: BUG/MINOR: payload: bound ClientHello extension lists by the extension length X-Git-Tag: v3.5-dev6~39 X-Git-Url: https://git.kaiwu.me/?a=commitdiff_plain;h=cb1eebf9d985a2100c6eff4368cde539d89fd845;p=haproxy.git BUG/MINOR: payload: bound ClientHello extension lists by the extension length smp_fetch_ssl_supported_groups(), smp_fetch_ssl_sigalgs(), smp_fetch_ssl_keyshare_groups() and smp_fetch_ssl_hello_sni() all check the length of the list carried by their extension against > hs_len - 6 where is what remains of the *whole* extension block, not of the extension being parsed. The list lives in the extension body, so the bound has to be - 2. All four sites already reject ext_len < 2 beforehand, so the new bound cannot go negative, and since ext_len is itself checked against hs_len - 4 a few lines above, it is strictly tighter than the current one. With the loose check, an extension whose body holds nothing but the 2-byte list length is accepted while announcing a list that runs into the extensions that follow, and the fetch returns bytes belonging to another extension: groups=ff010020aaaaaaaaaaaa... <- next extension's header and body sigalgs=ff010020aaaaaaaaaaaa... keyshare=ff01 sni=HIJACKED!! For req.ssl_sni the effect is the most visible, since name_type, name_len and the hostname are then all read from the neighbouring extension, and a hostname is reported that appears in no server_name entry. Those reads all stay within the ClientHello, whose bytes are supplied by the client anyway, so this is neither an out-of-bounds access nor an information disclosure: the returned sample ends at worst on the last byte of the extension block. It does however let a client decouple what use-server/use_backend rules and logs observe from what it really announced, on a ClientHello that a conformant TLS stack would reject. A reg-test covering, for each of the four fetches, a well-formed extension and an over-announced list is added. The req.ssl_supported_groups, req.ssl_sigalgs and req.ssl_keyshare_groups parts should be backported as far as 3.2, where those fetches appeared with commit fa063a9e7 ("MINOR: sample: add 4 new sample fetches for clienthello parsing"). The req.ssl_sni part is much older, it dates back to commit b6672b547 ("MINOR: acl: add support for TLS server name matching using SNI") in 1.5-dev8, so backports to branches older than 3.2 need to keep that hunk only. --- diff --git a/reg-tests/sample_fetches/ssl_client_hello_ext.vtc b/reg-tests/sample_fetches/ssl_client_hello_ext.vtc new file mode 100644 index 000000000..14d4ff32b --- /dev/null +++ b/reg-tests/sample_fetches/ssl_client_hello_ext.vtc @@ -0,0 +1,117 @@ +varnishtest "TLS ClientHello: extension lists must fit in their own extension" +feature cmd "$HAPROXY_PROGRAM -cc 'version_atleast(3.2-dev11)'" + +# smp_fetch_ssl_supported_groups(), smp_fetch_ssl_sigalgs(), +# smp_fetch_ssl_keyshare_groups() and smp_fetch_ssl_hello_sni() all validate +# the length of the list held by their extension with +# +# > hs_len - 6 +# +# where is what remains of the whole extension block, not of the +# extension currently being parsed. The list lives in the extension body, so +# the bound is - 2. +# +# With the loose check, an extension whose body is only the 2-byte list length +# is accepted while announcing a list that runs into the extensions that +# follow, and the fetch returns bytes that belong to another extension. For +# req.ssl_sni, name_type, name_len and the hostname itself are then all read +# from the neighbour, so a hostname can be reported that appears in no +# server_name entry at all. +# +# The reads stay within the ClientHello, so this is not an out-of-bounds +# access, but it lets a client steer the value seen by use-server/use_backend +# rules and logs. + +feature ignore_unknown_macro + +syslog S1 -level info { + # c1: well-formed extensions, every fetch returns its own contents + recv + expect ~ "[^:\\[ ]\\[${h1_pid}\\]: groups=001d0017 sigalgs=04030804 keyshare=001d sni=www.example.com$" + # c2..c5: one over-announced list each, every fetch must give up + recv + expect ~ "[^:\\[ ]\\[${h1_pid}\\]: groups=- sigalgs=- keyshare=- sni=-$" + recv + expect ~ "[^:\\[ ]\\[${h1_pid}\\]: groups=- sigalgs=- keyshare=- sni=-$" + recv + expect ~ "[^:\\[ ]\\[${h1_pid}\\]: groups=- sigalgs=- keyshare=- sni=-$" + recv + expect ~ "[^:\\[ ]\\[${h1_pid}\\]: groups=- sigalgs=- keyshare=- sni=-$" +} -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 "groups=%[var(txn.g)] sigalgs=%[var(txn.a)] keyshare=%[var(txn.k)] sni=%[var(txn.s)]" + + tcp-request inspect-delay 100ms + tcp-request content set-var(txn.g) req.ssl_supported_groups,hex,lower + tcp-request content set-var(txn.a) req.ssl_sigalgs,hex,lower + tcp-request content set-var(txn.k) req.ssl_keyshare_groups,hex,lower + tcp-request content set-var(txn.s) req.ssl_sni + tcp-request content reject +} -start + +# Each ClientHello has to be sent in a single write: HAProxy rejects the +# connection as soon as the ruleset is evaluated. They all share the same +# 43-byte prologue: +# +# 16 03 01 LL LL TLS record: handshake, TLSv1.0 +# 01 00 00 LL Handshake: ClientHello +# 03 03 client_version +# 00 .. 1f Random[32] +# 00 session_id_len = 0 +# 00 02 13 02 cipher_len = 2, one cipher suite +# 01 00 compression_len = 1, method 0 +# LL LL extensions_len +# +# and differ only by the extension block that follows. + +# server_name "www.example.com", supported_groups 001d:0017, +# sigalgs 0403:0804, key_share for group 001d. All well-formed. +client c1 -connect ${h1_fe1_sock} { + sendhex "1603010069 010000650303000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f000002130201 00 003a 000000140012 00000f 7777772e6578616d706c652e636f6d 000a00060004001d0017 000d00060004040308040 0330 00a0008001d0004aabbccdd" + delay 0.3 +} -run + +# supported_groups: 2-byte body announcing a 32-byte list. Without the fix, +# req.ssl_supported_groups returns the next extension: ff010020aaaa... +client c2 -connect ${h1_fe1_sock} { + sendhex "1603010059 010000550303000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f000002130201 00 002a 000a00020020 ff010020 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + delay 0.3 +} -run + +# same, for sigalgs +client c3 -connect ${h1_fe1_sock} { + sendhex "1603010059 010000550303000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f000002130201 00 002a 000d00020020 ff010020 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + delay 0.3 +} -run + +# same, for key_share +client c4 -connect ${h1_fe1_sock} { + sendhex "1603010059 010000550303000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f000002130201 00 002a 003300020020 ff010020 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + delay 0.3 +} -run + +# server_name: 2-byte body announcing a 32-byte list. Without the fix, +# name_type (00), name_len (000a) and the hostname are read out of the next +# extension header and body, and req.ssl_sni reports "HIJACKED!!". +client c5 -connect ${h1_fe1_sock} { + sendhex "1603010055 010000510303000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f000002130201 00 0026 000000020020 00000a48 494a41434b45442121 cccccccccccccccccccccccccccccccccccccc" + delay 0.3 +} -run + +syslog S1 -wait diff --git a/src/payload.c b/src/payload.c index aef51f236..50da7c8a1 100644 --- a/src/payload.c +++ b/src/payload.c @@ -587,8 +587,8 @@ smp_fetch_ssl_supported_groups(const struct arg *args, struct sample *smp, const goto not_ssl_hello; grp_len = (data[4] << 8) + data[5]; /* Supported group list length */ - if (grp_len < 2 || grp_len > hs_len - 6) - goto not_ssl_hello; /* at least 2 bytes per supported group */ + if (grp_len < 2 || grp_len > ext_len - 2) + goto not_ssl_hello; /* at least 2 bytes per supported group, and the list must fit in the extension */ smp->data.type = SMP_T_BIN; smp->data.u.str.area = (char *)data + 6; @@ -642,8 +642,8 @@ smp_fetch_ssl_sigalgs(const struct arg *args, struct sample *smp, const char *kw goto not_ssl_hello; sigalg_len = (data[4] << 8) + data[5]; /* Sigalgs list length */ - if (sigalg_len < 2 || sigalg_len > hs_len - 6) - goto not_ssl_hello; /* at least 2 bytes per sigalg */ + if (sigalg_len < 2 || sigalg_len > ext_len - 2) + goto not_ssl_hello; /* at least 2 bytes per sigalg, and the list must fit in the extension */ smp->data.type = SMP_T_BIN; smp->data.u.str.area = (char *)data + 6; @@ -701,8 +701,8 @@ smp_fetch_ssl_keyshare_groups(const struct arg *args, struct sample *smp, const goto not_ssl_hello; keyshare_len = (data[4] << 8) + data[5]; /* Client keyshare length */ - if (keyshare_len < 4 || keyshare_len > hs_len - 6) - goto not_ssl_hello; /* at least 4 bytes for one keyshare entry */ + if (keyshare_len < 4 || keyshare_len > ext_len - 2) + goto not_ssl_hello; /* at least 4 bytes for one keyshare entry, and the list must fit in the extension */ dataPointer = data + 6; /* start of keyshare entries */ readPosition = 0; numberOfKeyshares = 0; @@ -792,8 +792,8 @@ smp_fetch_ssl_hello_sni(const struct arg *args, struct sample *smp, const char * goto not_ssl_hello; srv_len = (data[4] << 8) + data[5]; - if (srv_len < 4 || srv_len > hs_len - 6) - goto not_ssl_hello; /* at least 4 bytes per server name */ + if (srv_len < 4 || srv_len > ext_len - 2) + goto not_ssl_hello; /* at least 4 bytes per server name, and the list must fit in the extension */ name_type = data[6]; name_len = (data[7] << 8) + data[8];