From 8a14aaaca5acc10eb662db3ad5391034463c3ac4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 9 Aug 2024 12:00:34 +0200 Subject: [PATCH] Simplify certificate curve check for 1.2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comments were about the time we were using mbedtls_pk_ec(), which can return NULL, which we don't want to propagate to other functions. Now we're using mbedtls_pk_get_ec_group_id() with is a safer interface (and works even when EC is provided by drivers). The check for GROUP_NONE was an heritage from the previous NULL check. However it's actually useless: if NONE were returned (which can't happen or parsing of the certificate would have failed and we wouldn't be here), then mbedtls_ssl_check_curve() would work and just say that the curve wasn't valid, which is OK. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 3f375be92c..1355feb88b 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8050,35 +8050,19 @@ static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, * Secondary checks: always done, but change 'ret' only if it was 0 */ + /* Check curve for ECC certs */ #if defined(MBEDTLS_PK_HAVE_ECC_KEYS) - { - const mbedtls_pk_context *pk = &chain->pk; - - /* If certificate uses an EC key, make sure the curve is OK. - * This is a public key, so it can't be opaque, so can_do() is a good - * enough check to ensure pk_ec() is safe to use here. */ - if (mbedtls_pk_can_do(pk, MBEDTLS_PK_ECKEY)) { - /* and in the unlikely case the above assumption no longer holds - * we are making sure that pk_ec() here does not return a NULL - */ - mbedtls_ecp_group_id grp_id = mbedtls_pk_get_ec_group_id(pk); - if (grp_id == MBEDTLS_ECP_DP_NONE) { - MBEDTLS_SSL_DEBUG_MSG(1, ("invalid group ID")); - return MBEDTLS_ERR_SSL_INTERNAL_ERROR; - } - if (mbedtls_ssl_check_curve(ssl, grp_id) != 0) { - ssl->session_negotiate->verify_result |= - MBEDTLS_X509_BADCERT_BAD_KEY; - - MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate (EC key curve)")); - if (ret == 0) { - ret = MBEDTLS_ERR_SSL_BAD_CERTIFICATE; - } - } + if (mbedtls_pk_can_do(&chain->pk, MBEDTLS_PK_ECKEY) && + mbedtls_ssl_check_curve(ssl, mbedtls_pk_get_ec_group_id(&chain->pk)) != 0) { + MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate (EC key curve)")); + ssl->session_negotiate->verify_result |= MBEDTLS_X509_BADCERT_BAD_KEY; + if (ret == 0) { + ret = MBEDTLS_ERR_SSL_BAD_CERTIFICATE; } } #endif /* MBEDTLS_PK_HAVE_ECC_KEYS */ + /* Check X.509 usage extensions (keyUsage, extKeyUsage) */ if (mbedtls_ssl_check_cert_usage(chain, ciphersuite_info, ssl->conf->endpoint,