diff --git a/ChangeLog.d/tls12-handshake-length-read-bounds.txt b/ChangeLog.d/tls12-handshake-length-read-bounds.txt new file mode 100644 index 0000000000..d061786255 --- /dev/null +++ b/ChangeLog.d/tls12-handshake-length-read-bounds.txt @@ -0,0 +1,10 @@ +Bugfix + * Fix a buffer over-read when parsing a TLS 1.2 ClientHello or + CertificateRequest that is truncated inside the two-byte length field + of the ciphersuite list (server side) or of the certificate + authorities list (client side): the length checks preceding those + fields reserved too few bytes, so the length field could be read past + the end of the message. Such messages are now rejected before the + read. + * The TLS 1.2 client now rejects a CertificateRequest message whose + supported_signature_algorithms list has an odd length. diff --git a/library/ssl_tls12_client.c b/library/ssl_tls12_client.c index c8149087d0..a80a26ed49 100644 --- a/library/ssl_tls12_client.c +++ b/library/ssl_tls12_client.c @@ -2217,7 +2217,7 @@ static int ssl_parse_certificate_request(mbedtls_ssl_context *ssl) buf = ssl->in_msg; /* certificate_types */ - if (ssl->in_hslen <= mbedtls_ssl_hs_hdr_len(ssl)) { + if (ssl->in_hslen < mbedtls_ssl_hs_hdr_len(ssl) + 1) { MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate request message")); mbedtls_ssl_send_alert_message(ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR); @@ -2227,16 +2227,10 @@ static int ssl_parse_certificate_request(mbedtls_ssl_context *ssl) n = cert_type_len; /* - * In the subsequent code there are two paths that read from buf: - * * the length of the signature algorithms field (if minor version of - * SSL is 3), - * * distinguished name length otherwise. - * Both reach at most the index: - * ...hdr_len + 2 + n, - * therefore the buffer length at this point must be greater than that - * regardless of the actual code path. + * The two-byte supported_signature_algorithms length is read next, + * at offset ...hdr_len + 1 + n. */ - if (ssl->in_hslen <= mbedtls_ssl_hs_hdr_len(ssl) + 2 + n) { + if (ssl->in_hslen < mbedtls_ssl_hs_hdr_len(ssl) + 1 + n + 2) { MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate request message")); mbedtls_ssl_send_alert_message(ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR); @@ -2247,18 +2241,13 @@ static int ssl_parse_certificate_request(mbedtls_ssl_context *ssl) sig_alg_len = MBEDTLS_GET_UINT16_BE(buf, mbedtls_ssl_hs_hdr_len(ssl) + 1 + n); /* - * The furthest access in buf is in the loop few lines below: - * sig_alg[i + 1], - * where: - * sig_alg = buf + ...hdr_len + 3 + n, - * max(i) = sig_alg_len - 1. - * Therefore the furthest access is: - * buf[...hdr_len + 3 + n + sig_alg_len - 1 + 1], - * which reduces to: - * buf[...hdr_len + 3 + n + sig_alg_len], - * which is one less than we need the buf to be. + * The list is a sequence of two-byte values, so its length must be + * even. The message must have room for the list itself, plus the + * two-byte certificate_authorities length that follows it and is + * read below. */ - if (ssl->in_hslen <= mbedtls_ssl_hs_hdr_len(ssl) + 3 + n + sig_alg_len) { + if (sig_alg_len % 2 != 0 || + ssl->in_hslen < mbedtls_ssl_hs_hdr_len(ssl) + 1 + n + 2 + sig_alg_len + 2) { MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate request message")); mbedtls_ssl_send_alert_message( ssl, diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index b02ae505ef..598f8b946b 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -1016,7 +1016,7 @@ static int ssl_parse_client_hello(mbedtls_ssl_context *ssl) sess_len = buf[34]; if (sess_len > sizeof(ssl->session_negotiate->id) || - sess_len + 34 + 2 > msg_len) { /* 2 for cipherlist length field */ + sess_len + 35 > msg_len) { MBEDTLS_SSL_DEBUG_MSG(1, ("bad client hello message")); mbedtls_ssl_send_alert_message(ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR); @@ -1037,9 +1037,16 @@ static int ssl_parse_client_hello(mbedtls_ssl_context *ssl) #if defined(MBEDTLS_SSL_PROTO_DTLS) if (ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) { cookie_offset = 35 + sess_len; + + if (cookie_offset + 1 > msg_len) { + MBEDTLS_SSL_DEBUG_MSG(1, ("bad client hello message")); + mbedtls_ssl_send_alert_message(ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR); + return MBEDTLS_ERR_SSL_DECODE_ERROR; + } cookie_len = buf[cookie_offset]; - if (cookie_offset + 1 + cookie_len + 2 > msg_len) { + if (cookie_offset + 1 + cookie_len > msg_len) { MBEDTLS_SSL_DEBUG_MSG(1, ("bad client hello message")); mbedtls_ssl_send_alert_message(ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR); @@ -1085,6 +1092,16 @@ static int ssl_parse_client_hello(mbedtls_ssl_context *ssl) #endif /* MBEDTLS_SSL_PROTO_DTLS */ ciph_offset = 35 + sess_len; + /* The two-byte ciphersuite list length field must be within the + * message: the checks above only guarantee that the fields before it + * (session id, and for DTLS the cookie) are. */ + if (ciph_offset + 2 > msg_len) { + MBEDTLS_SSL_DEBUG_MSG(1, ("bad client hello message")); + mbedtls_ssl_send_alert_message(ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR); + return MBEDTLS_ERR_SSL_DECODE_ERROR; + } + ciph_len = MBEDTLS_GET_UINT16_BE(buf, ciph_offset); if (ciph_len < 2 || diff --git a/tests/suites/test_suite_ssl.tls-defrag.data b/tests/suites/test_suite_ssl.tls-defrag.data index 7817c4f501..ec8c1f8035 100644 --- a/tests/suites/test_suite_ssl.tls-defrag.data +++ b/tests/suites/test_suite_ssl.tls-defrag.data @@ -63,6 +63,26 @@ Inject ClientHello - TLS 1.2 unknown ciphersuite (for reference) depends_on:MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED:PSA_WANT_KEY_TYPE_AES:PSA_WANT_ALG_SHA_256:PSA_WANT_ALG_GCM:PSA_WANT_ECC_SECP_R1_256:PSA_WANT_ECC_SECP_R1_384:PSA_WANT_ALG_SHA_1 inject_client_content_on_the_wire:MBEDTLS_PK_ECDSA:MBEDTLS_SSL_CLIENT_HELLO:"160303002f0100002b03030123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef000002cccc01000000":"got no ciphersuites in common":MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE +# See "ClientHello breakdown" above +# ClientHello body ending right after the session id: version (2) + +# random (32) + session id length (1) + session id (3) = 38 bytes, the +# minimum accepted, with the ciphersuite list length field missing +# entirely. Only run in TLS 1.2-only builds: when TLS 1.3 is enabled, +# the TLS 1.3 code parses the ClientHello first and rejects it itself. +Inject ClientHello - TLS 1.2 truncated after session id +depends_on:MBEDTLS_SSL_PROTO_TLS1_2:!MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED:PSA_WANT_KEY_TYPE_AES:PSA_WANT_ALG_SHA_256:PSA_WANT_ALG_GCM:PSA_WANT_ECC_SECP_R1_256:PSA_WANT_ECC_SECP_R1_384:PSA_WANT_ALG_SHA_1 +inject_client_content_on_the_wire:MBEDTLS_PK_ECDSA:MBEDTLS_SSL_CLIENT_HELLO:"160303002a0100002603030123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef035f5f5f":"bad client hello message":MBEDTLS_ERR_SSL_DECODE_ERROR + +# See "ClientHello breakdown" above +# ClientHello body ending inside the ciphersuite list length: version (2) + +# random (32) + session id length (1) + session id (2) + first length +# byte (1) = 38 bytes. Reading the full two-byte length field used to +# over-read one byte past the end of the message. +# Only run in TLS 1.2-only builds, see above. +Inject ClientHello - TLS 1.2 truncated inside ciphersuite list length +depends_on:MBEDTLS_SSL_PROTO_TLS1_2:!MBEDTLS_SSL_PROTO_TLS1_3:MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED:PSA_WANT_KEY_TYPE_AES:PSA_WANT_ALG_SHA_256:PSA_WANT_ALG_GCM:PSA_WANT_ECC_SECP_R1_256:PSA_WANT_ECC_SECP_R1_384:PSA_WANT_ALG_SHA_1 +inject_client_content_on_the_wire:MBEDTLS_PK_ECDSA:MBEDTLS_SSL_CLIENT_HELLO:"160303002a0100002603030123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef025f5f00":"bad client hello message":MBEDTLS_ERR_SSL_DECODE_ERROR + # See "ClientHello breakdown" above # ephemeral with secp256r1 + MBEDTLS_TLS1_3_AES_128_GCM_SHA256 Inject ClientHello - TLS 1.3 good (for reference)