diff --git a/ChangeLog.d/tls12-handshake-length-read-bounds.txt b/ChangeLog.d/tls12-handshake-length-read-bounds.txt index f9e14e209b..d061786255 100644 --- a/ChangeLog.d/tls12-handshake-length-read-bounds.txt +++ b/ChangeLog.d/tls12-handshake-length-read-bounds.txt @@ -1,6 +1,10 @@ Bugfix - * Fix a one-byte buffer over-read when parsing a TLS 1.2 ClientHello or - CertificateRequest. The length checks preceding the ciphersuite-list - length (server side) and the certificate-authorities length (client - side) reserved one byte too few, so a truncated message could cause the - two-byte length field to be read one byte past the end of the message. + * 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 b3341770ea..16a69a8527 100644 --- a/library/ssl_tls12_client.c +++ b/library/ssl_tls12_client.c @@ -2218,7 +2218,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); @@ -2228,16 +2228,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); @@ -2248,18 +2242,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, @@ -2279,16 +2268,6 @@ static int ssl_parse_certificate_request(mbedtls_ssl_context *ssl) n += 2 + sig_alg_len; - /* The signature-algorithms check above leaves room for only one of the two - * distinguished-name length bytes, so make sure both are in the message - * before reading them. */ - if (ssl->in_hslen < mbedtls_ssl_hs_hdr_len(ssl) + 3 + n) { - 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); - return MBEDTLS_ERR_SSL_DECODE_ERROR; - } - /* certificate_authorities */ dn_len = MBEDTLS_GET_UINT16_BE(buf, mbedtls_ssl_hs_hdr_len(ssl) + 1 + n); diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index 85d7a8c625..4737a6ce5a 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -1005,7 +1005,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); @@ -1026,9 +1026,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); @@ -1074,9 +1081,9 @@ 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 DTLS branch above reserves it via the cookie length check; the - * session id length check does not, so guard the read here. */ + /* 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, 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)