From 12c5aaae574114cf4883d0e21aa0045b0a606110 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 2 Oct 2023 14:55:45 +0200 Subject: [PATCH 1/8] Fix buffer overflow in TLS 1.3 ECDH public key parsing Fix a buffer overflow in TLS 1.3 ServerHello and ClientHello parsing. The length of the public key in an ECDH- or FFDH-based key exchange was not validated. This could result in an overflow of handshake->xxdh_psa_peerkey, overwriting further data in the handshake structure or further on the heap. Signed-off-by: Gilles Peskine --- library/ssl_tls13_generic.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 81fa514f6..dc88c4fdd 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -1516,7 +1516,10 @@ int mbedtls_ssl_tls13_read_public_xxdhe_share(mbedtls_ssl_context *ssl, /* Check if key size is consistent with given buffer length. */ MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, peerkey_len); - /* Store peer's ECDH public key. */ + /* Store peer's ECDH/FFDH public key. */ + if (peerkey_len > sizeof(handshake->xxdh_psa_peerkey)) { + return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; + } memcpy(handshake->xxdh_psa_peerkey, p, peerkey_len); handshake->xxdh_psa_peerkey_len = peerkey_len; From c8df898204584e92b9e5001847788d899b34353d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 2 Oct 2023 14:58:16 +0200 Subject: [PATCH 2/8] Fix buffer overflow in TLS 1.2 ClientKeyExchange parsing Fix a buffer overflow in TLS 1.2 ClientKeyExchange parsing. When MBEDTLS_USE_PSA_CRYPTO is enabled, the length of the public key in an ECDH or ECDHE key exchange was not validated. This could result in an overflow of handshake->xxdh_psa_peerkey, overwriting further data in the handshake structure or further on the heap. Signed-off-by: Gilles Peskine --- library/ssl_tls12_server.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index d2143ac15..ed2fbd1d6 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -3734,6 +3734,11 @@ static int ssl_parse_client_key_exchange(mbedtls_ssl_context *ssl) } /* Store peer's ECDH public key. */ + MBEDTLS_SSL_DEBUG_MSG(3, ("data_len=%zu sizeof(handshake->xxdh_psa_peerkey)=%zu", data_len, sizeof(handshake->xxdh_psa_peerkey))); + if (data_len > sizeof(handshake->xxdh_psa_peerkey)) { + MBEDTLS_SSL_DEBUG_MSG(1, ("Invalid data length")); + return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; + } memcpy(handshake->xxdh_psa_peerkey, p, data_len); handshake->xxdh_psa_peerkey_len = data_len; From c29df535eea63765e6a0fa91da4a736595fe9898 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 2 Oct 2023 14:59:26 +0200 Subject: [PATCH 3/8] Improve robustness of ECDH public key length validation In client-side code with MBEDTLS_USE_PSA_CRYPTO, use the buffer size to validate what is written in handshake->xxdh_psa_peerkey. The previous code was correct, but a little fragile to misconfiguration or maintenance. Signed-off-by: Gilles Peskine --- library/ssl_tls12_client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls12_client.c b/library/ssl_tls12_client.c index cc22a3fe1..02f6cd369 100644 --- a/library/ssl_tls12_client.c +++ b/library/ssl_tls12_client.c @@ -1779,7 +1779,7 @@ static int ssl_parse_server_ecdh_params(mbedtls_ssl_context *ssl, return MBEDTLS_ERR_SSL_DECODE_ERROR; } - if (ecpoint_len > PSA_KEY_EXPORT_ECC_PUBLIC_KEY_MAX_SIZE(PSA_VENDOR_ECC_MAX_CURVE_BITS)) { + if (ecpoint_len > sizeof(handshake->xxdh_psa_peerkey)) { return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; } @@ -2059,7 +2059,7 @@ static int ssl_get_ecdh_params_from_cert(mbedtls_ssl_context *ssl) ret = mbedtls_ecp_point_write_binary(&peer_key->grp, &peer_key->Q, MBEDTLS_ECP_PF_UNCOMPRESSED, &olen, ssl->handshake->xxdh_psa_peerkey, - MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH); + sizeof(ssl->handshake->xxdh_psa_peerkey)); if (ret != 0) { MBEDTLS_SSL_DEBUG_RET(1, ("mbedtls_ecp_point_write_binary"), ret); From b782415e1bd6df03543223e1c653641d9dcd946a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 2 Oct 2023 15:08:37 +0200 Subject: [PATCH 4/8] Changelog entry for xxdh_psa_peerkey size validation Signed-off-by: Gilles Peskine --- ChangeLog.d/xxx_psa_peerkey.txt | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ChangeLog.d/xxx_psa_peerkey.txt diff --git a/ChangeLog.d/xxx_psa_peerkey.txt b/ChangeLog.d/xxx_psa_peerkey.txt new file mode 100644 index 000000000..1ba151000 --- /dev/null +++ b/ChangeLog.d/xxx_psa_peerkey.txt @@ -0,0 +1,6 @@ +Security + * Fix a remotely exploitable heap buffer overflow in TLS handshake parsing. + In TLS 1.3, all configurations are affected except PSK-only ones. + In TLS 1.2, the affected configurations are those with + MBEDTLS_USE_PSA_CRYPTO and ECDH enabled but DHM and RSA disabled. + Credit to OSS-Fuzz. From 6dd5b9a60c2d6f483c30f9b010f663dd63394451 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 2 Oct 2023 15:36:58 +0200 Subject: [PATCH 5/8] In TLS 1.2, only servers are affected Signed-off-by: Gilles Peskine --- ChangeLog.d/xxx_psa_peerkey.txt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ChangeLog.d/xxx_psa_peerkey.txt b/ChangeLog.d/xxx_psa_peerkey.txt index 1ba151000..d25e4ecbf 100644 --- a/ChangeLog.d/xxx_psa_peerkey.txt +++ b/ChangeLog.d/xxx_psa_peerkey.txt @@ -1,6 +1,8 @@ Security * Fix a remotely exploitable heap buffer overflow in TLS handshake parsing. - In TLS 1.3, all configurations are affected except PSK-only ones. + In TLS 1.3, all configurations are affected except PSK-only ones, and + both clients and servers are affected. In TLS 1.2, the affected configurations are those with - MBEDTLS_USE_PSA_CRYPTO and ECDH enabled but DHM and RSA disabled. + MBEDTLS_USE_PSA_CRYPTO and ECDH enabled but DHM and RSA disabled, + and only servers are affected, not clients. Credit to OSS-Fuzz. From 530c423ad257f187b35b9de841c2dd79529d2cee Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 2 Oct 2023 15:37:23 +0200 Subject: [PATCH 6/8] Improve some debug messages and error codes On a parsing error in TLS, return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE, not a crypto error code. On error paths, emit a level-1 debug message. Report the offending sizes. Downgrade an informational message's level to 3. Signed-off-by: Gilles Peskine --- library/ssl_tls12_server.c | 18 ++++++++++++------ library/ssl_tls13_generic.c | 3 +++ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index ed2fbd1d6..37340fb34 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -3718,25 +3718,31 @@ static int ssl_parse_client_key_exchange(mbedtls_ssl_context *ssl) psa_status_t status = PSA_ERROR_GENERIC_ERROR; mbedtls_ssl_handshake_params *handshake = ssl->handshake; - MBEDTLS_SSL_DEBUG_MSG(1, ("Read the peer's public key.")); + MBEDTLS_SSL_DEBUG_MSG(3, ("Read the peer's public key.")); /* * We must have at least two bytes (1 for length, at least 1 for data) */ if (buf_len < 2) { - MBEDTLS_SSL_DEBUG_MSG(1, ("Invalid buffer length")); - return MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + MBEDTLS_SSL_DEBUG_MSG(1, ("Invalid buffer length: %" MBEDTLS_PRINTF_SIZET, + buf_len)); + return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; } if (data_len < 1 || data_len > buf_len) { - MBEDTLS_SSL_DEBUG_MSG(1, ("Invalid data length")); - return MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + MBEDTLS_SSL_DEBUG_MSG(1, ("Invalid data length: %" MBEDTLS_PRINTF_SIZET + " > %" MBEDTLS_PRINTF_SIZET, + data_len, buf_len)); + return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; } /* Store peer's ECDH public key. */ MBEDTLS_SSL_DEBUG_MSG(3, ("data_len=%zu sizeof(handshake->xxdh_psa_peerkey)=%zu", data_len, sizeof(handshake->xxdh_psa_peerkey))); if (data_len > sizeof(handshake->xxdh_psa_peerkey)) { - MBEDTLS_SSL_DEBUG_MSG(1, ("Invalid data length")); + MBEDTLS_SSL_DEBUG_MSG(1, ("Invalid public key length: %" MBEDTLS_PRINTF_SIZET + " > %" MBEDTLS_PRINTF_SIZET, + data_len, + sizeof(handshake->xxdh_psa_peerkey))); return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; } memcpy(handshake->xxdh_psa_peerkey, p, data_len); diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index dc88c4fdd..7072677f1 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -1518,6 +1518,9 @@ int mbedtls_ssl_tls13_read_public_xxdhe_share(mbedtls_ssl_context *ssl, /* Store peer's ECDH/FFDH public key. */ if (peerkey_len > sizeof(handshake->xxdh_psa_peerkey)) { + MBEDTLS_SSL_DEBUG_MSG(1, ("Invalid public key length: %u > %" MBEDTLS_PRINTF_SIZET, + (unsigned) peerkey_len, + sizeof(handshake->xxdh_psa_peerkey))); return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; } memcpy(handshake->xxdh_psa_peerkey, p, peerkey_len); From 7910cdd47fab98e29af6a1c6c7a4730f4ece8a58 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 2 Oct 2023 15:39:13 +0200 Subject: [PATCH 7/8] Avoid compiler warning about size comparison GCC warns about comparing uint8_t to a size that may be >255. Strangely, casting the uint8_t to a size_t in the comparison expression doesn't avoid the warning. So change the type of the variable. Signed-off-by: Gilles Peskine --- library/ssl_tls12_client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls12_client.c b/library/ssl_tls12_client.c index 02f6cd369..27bbafa06 100644 --- a/library/ssl_tls12_client.c +++ b/library/ssl_tls12_client.c @@ -1727,7 +1727,7 @@ static int ssl_parse_server_ecdh_params(mbedtls_ssl_context *ssl, unsigned char *end) { uint16_t tls_id; - uint8_t ecpoint_len; + size_t ecpoint_len; mbedtls_ssl_handshake_params *handshake = ssl->handshake; psa_key_type_t key_type = PSA_KEY_TYPE_NONE; size_t ec_bits = 0; From 3713bee34c48974d60066b4dab16874e32667f69 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 2 Oct 2023 18:43:18 +0200 Subject: [PATCH 8/8] Remove leftover local debug line Signed-off-by: Gilles Peskine --- library/ssl_tls12_server.c | 1 - 1 file changed, 1 deletion(-) diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index 37340fb34..6ebd5064f 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -3737,7 +3737,6 @@ static int ssl_parse_client_key_exchange(mbedtls_ssl_context *ssl) } /* Store peer's ECDH public key. */ - MBEDTLS_SSL_DEBUG_MSG(3, ("data_len=%zu sizeof(handshake->xxdh_psa_peerkey)=%zu", data_len, sizeof(handshake->xxdh_psa_peerkey))); if (data_len > sizeof(handshake->xxdh_psa_peerkey)) { MBEDTLS_SSL_DEBUG_MSG(1, ("Invalid public key length: %" MBEDTLS_PRINTF_SIZET " > %" MBEDTLS_PRINTF_SIZET,