BUG/MINOR: payload: fix handshake length off-by-4 in ssl_hello_sni/alpn

smp_fetch_ssl_hello_sni() and smp_fetch_ssl_hello_alpn() each have
their own copy of the ClientHello preamble parser instead of using the
shared smp_client_hello_parse(). The handshake length ("hs_len") is
compared against "bleft" before "bleft" is decremented for the 4-byte
handshake header (msg_type + 3-byte length), so a ClientHello claiming
a body up to 4 bytes larger than what was actually sent is still
accepted as complete. Every subsequent length-derived bound inherits
this same 4-byte over-count, letting the final read (SNI hostname or
ALPN protocol name) run up to 4 bytes past the received buffer and
disclose uninitialized memory through req.ssl_sni / req.ssl_alpn.

This is the same issue already fixed in smp_client_hello_parse() by
commit 2653936510 ("BUG/MINOR: payload: fix the handshake length
bounds check smp_client_hello_parse()"), just never ported to these
two functions' own duplicated preamble. This applies the same fix:
"data += 4" now comes with "bleft -= 4" before the length check
instead of after it, and the record-layer length is checked as soon
as the handshake is entered.

This must be backported to all supported versions.

diff --git a/src/payload.c b/src/payload.c
index 5e39133ba993685c08d92e235309d46845477377..167fb4303485ed4a968d1c9fe5f3f14684f8cedf 100644 (file)
--- a/src/payload.c
+++ b/src/payload.c
@@ -806,6 +806,9 @@ smp_fetch_ssl_hello_sni(const struct arg *args, struct sample *smp, const char *
        data += 5; /* enter TLS handshake */
        bleft -= 5;
 
+       if (bleft < hs_len)
+               goto too_short;
+
        /* Check for a complete client hello starting at <data> */
        if (bleft < 1)
                goto too_short;
@@ -819,15 +822,18 @@ smp_fetch_ssl_hello_sni(const struct arg *args, struct sample *smp, const char *
        if (hs_len < 2 + 32 + 1 + 2 + 2 + 1 + 1 + 2 + 2)
                goto not_ssl_hello; /* too short to have an extension */
 
+       data += 4;
+       bleft -= 4;
+
        /* We want the full handshake here */
        if (bleft < hs_len)
                goto too_short;
 
-       data += 4;
        /* Start of the ClientHello message */
        if (data[0] < 0x03 || data[1] < 0x01) /* TLSv1 minimum */
                goto not_ssl_hello;
 
+       /* Note: covered by the hs_len test 30 lines above */
        ext_len = data[34]; /* session_id_len */
        if (ext_len > 32 || ext_len > (hs_len - 35)) /* check for correct session_id len */
                goto not_ssl_hello;
@@ -979,6 +985,9 @@ smp_fetch_ssl_hello_alpn(const struct arg *args, struct sample *smp, const char
        data += 5; /* enter TLS handshake */
        bleft -= 5;
 
+       if (bleft < hs_len)
+               goto too_short;
+
        /* Check for a complete client hello starting at <data> */
        if (bleft < 1)
                goto too_short;
@@ -992,15 +1001,18 @@ smp_fetch_ssl_hello_alpn(const struct arg *args, struct sample *smp, const char
        if (hs_len < 2 + 32 + 1 + 2 + 2 + 1 + 1 + 2 + 2)
                goto not_ssl_hello; /* too short to have an extension */
 
+       data += 4;
+       bleft -= 4;
+
        /* We want the full handshake here */
        if (bleft < hs_len)
                goto too_short;
 
-       data += 4;
        /* Start of the ClientHello message */
        if (data[0] < 0x03 || data[1] < 0x01) /* TLSv1 minimum */
                goto not_ssl_hello;
 
+       /* Note: covered by the hs_len test 30 lines above */
        ext_len = data[34]; /* session_id_len */
        if (ext_len > 32 || ext_len > (hs_len - 35)) /* check for correct session_id len */
                goto not_ssl_hello;