From a6397f0eb37cd03862d9bbdf36eb683fdd9d4d41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 Aug 2024 11:10:47 +0200 Subject: [PATCH 01/25] Add test forcing TLS 1.2 for clearer coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a duplicate from the previous test, except it forces TLS 1.2. The previous test does not force a version, so it picks 1.3 in the default/full config. However we have a build with 1.2 only in all.sh, in which the previous test would pick 1.2. So, there was no test gap and the behaviour was indeed tested with 1.2. However when measuring code coverage with lcov, currently we can only use a single build. So, I'm adding this variant of the test case as a so that the 1.2 code looks covered in the report from basic-build-test.sh. This is for my convenience while I make sure everything is covered before refactoring. Signed-off-by: Manuel Pégourié-Gonnard --- tests/ssl-opt.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 531eb7456..7872b8dfc 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5853,6 +5853,17 @@ run_test "Authentication: server goodcert, client required, no trusted CA" \ -c "! mbedtls_ssl_handshake returned" \ -c "SSL - No CA Chain is set, but required to operate" +requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT +run_test "Authentication: server goodcert, client required, no trusted CA (1.2)" \ + "$P_SRV force_version=tls12" \ + "$P_CLI debug_level=3 auth_mode=required ca_file=none ca_path=none" \ + 1 \ + -c "x509_verify_cert() returned" \ + -c "! The certificate is not correctly signed by the trusted CA" \ + -c "! Certificate verification flags"\ + -c "! mbedtls_ssl_handshake returned" \ + -c "SSL - No CA Chain is set, but required to operate" + # The purpose of the next two tests is to test the client's behaviour when receiving a server # certificate with an unsupported elliptic curve. This should usually not happen because # the client informs the server about the supported curves - it does, though, in the From 946d14a7acb2c17fcbfbc6593acdc49155175cfa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 Aug 2024 11:21:01 +0200 Subject: [PATCH 02/25] Fix ordering of a test case in ssl-opt.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/ssl-opt.sh | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 7872b8dfc..113d53edf 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5809,6 +5809,7 @@ run_test "DER format: with 9 trailing random bytes" \ # Tests for auth_mode, there are duplicated tests using ca callback for authentication # When updating these tests, modify the matching authentication tests accordingly +# The next 3 cases test the 3 auth modes with a badly signed server cert. requires_key_exchange_with_cert_in_tls12_or_tls13_enabled run_test "Authentication: server badcert, client required" \ "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ @@ -5830,6 +5831,16 @@ run_test "Authentication: server badcert, client optional" \ -C "! mbedtls_ssl_handshake returned" \ -C "X509 - Certificate verification failed" +run_test "Authentication: server badcert, client none" \ + "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ + key_file=$DATA_FILES_PATH/server5.key" \ + "$P_CLI force_version=tls12 debug_level=1 auth_mode=none" \ + 0 \ + -C "x509_verify_cert() returned" \ + -C "! The certificate is not correctly signed by the trusted CA" \ + -C "! mbedtls_ssl_handshake returned" \ + -C "X509 - Certificate verification failed" + requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT run_test "Authentication: server goodcert, client optional, no trusted CA" \ "$P_SRV" \ @@ -5889,16 +5900,6 @@ run_test "Authentication: server ECDH p256v1, client optional, p256v1 unsuppo -c "! Certificate verification flags"\ -c "bad server certificate (ECDH curve)" # Expect failure only at ECDH params check -run_test "Authentication: server badcert, client none" \ - "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ - key_file=$DATA_FILES_PATH/server5.key" \ - "$P_CLI force_version=tls12 debug_level=1 auth_mode=none" \ - 0 \ - -C "x509_verify_cert() returned" \ - -C "! The certificate is not correctly signed by the trusted CA" \ - -C "! mbedtls_ssl_handshake returned" \ - -C "X509 - Certificate verification failed" - requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT run_test "Authentication: client SHA256, server required" \ "$P_SRV auth_mode=required" \ From 0274175454a409d3858bc141f03727d46c94aafa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 Aug 2024 12:41:59 +0200 Subject: [PATCH 03/25] Test cert alert NOT_TRUSTED -> UNKNOWN_CA MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In terms of line coverage, this was covered, except we never checked the behaviour was as intended. Signed-off-by: Manuel Pégourié-Gonnard --- tests/ssl-opt.sh | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 113d53edf..d9ea156ce 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5809,36 +5809,55 @@ run_test "DER format: with 9 trailing random bytes" \ # Tests for auth_mode, there are duplicated tests using ca callback for authentication # When updating these tests, modify the matching authentication tests accordingly -# The next 3 cases test the 3 auth modes with a badly signed server cert. +# The next 4 cases test the 3 auth modes with a badly signed server cert. requires_key_exchange_with_cert_in_tls12_or_tls13_enabled run_test "Authentication: server badcert, client required" \ "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ key_file=$DATA_FILES_PATH/server5.key" \ - "$P_CLI debug_level=1 auth_mode=required" \ + "$P_CLI debug_level=3 auth_mode=required" \ 1 \ -c "x509_verify_cert() returned" \ -c "! The certificate is not correctly signed by the trusted CA" \ -c "! mbedtls_ssl_handshake returned" \ + -c "send alert level=2 message=48" \ -c "X509 - Certificate verification failed" + # MBEDTLS_X509_BADCERT_NOT_TRUSTED -> MBEDTLS_SSL_ALERT_MSG_UNKNOWN_CA +# We don't check that the server receives the alert because it might +# detect that its write end of the connection is closed and abort +# before reading the alert message. + +run_test "Authentication: server badcert, client required (1.2)" \ + "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ + key_file=$DATA_FILES_PATH/server5.key" \ + "$P_CLI force_version=tls12 debug_level=3 auth_mode=required" \ + 1 \ + -c "x509_verify_cert() returned" \ + -c "! The certificate is not correctly signed by the trusted CA" \ + -c "! mbedtls_ssl_handshake returned" \ + -c "send alert level=2 message=48" \ + -c "X509 - Certificate verification failed" + # MBEDTLS_X509_BADCERT_NOT_TRUSTED -> MBEDTLS_SSL_ALERT_MSG_UNKNOWN_CA run_test "Authentication: server badcert, client optional" \ "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ key_file=$DATA_FILES_PATH/server5.key" \ - "$P_CLI force_version=tls12 debug_level=1 auth_mode=optional" \ + "$P_CLI force_version=tls12 debug_level=3 auth_mode=optional" \ 0 \ -c "x509_verify_cert() returned" \ -c "! The certificate is not correctly signed by the trusted CA" \ -C "! mbedtls_ssl_handshake returned" \ + -C "send alert level=2 message=48" \ -C "X509 - Certificate verification failed" run_test "Authentication: server badcert, client none" \ "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ key_file=$DATA_FILES_PATH/server5.key" \ - "$P_CLI force_version=tls12 debug_level=1 auth_mode=none" \ + "$P_CLI force_version=tls12 debug_level=3 auth_mode=none" \ 0 \ -C "x509_verify_cert() returned" \ -C "! The certificate is not correctly signed by the trusted CA" \ -C "! mbedtls_ssl_handshake returned" \ + -C "send alert level=2 message=48" \ -C "X509 - Certificate verification failed" requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT From 2ffa53aa28d58d406162f4323fe7aba15ac1d124 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 Aug 2024 12:44:57 +0200 Subject: [PATCH 04/25] Test cert alert REVOKED -> CERT_REVOKED MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/ssl-opt.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index d9ea156ce..7237920dd 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -6609,7 +6609,9 @@ run_test "SNI: CA override with CRL" \ -S "skip parse certificate verify" \ -s "x509_verify_cert() returned" \ -S "! The certificate is not correctly signed by the trusted CA" \ + -s "send alert level=2 message=44" \ -s "The certificate has been revoked (is on a CRL)" + # MBEDTLS_X509_BADCERT_REVOKED -> MBEDTLS_SSL_ALERT_MSG_CERT_REVOKED # Tests for SNI and DTLS @@ -6757,7 +6759,9 @@ run_test "SNI: DTLS, CA override with CRL" \ -S "skip parse certificate verify" \ -s "x509_verify_cert() returned" \ -S "! The certificate is not correctly signed by the trusted CA" \ + -s "send alert level=2 message=44" \ -s "The certificate has been revoked (is on a CRL)" + # MBEDTLS_X509_BADCERT_REVOKED -> MBEDTLS_SSL_ALERT_MSG_CERT_REVOKED # Tests for non-blocking I/O: exercise a variety of handshake flows From 94f70228e9b93cf84b8cb00e544d8a66dc108011 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 9 Aug 2024 11:26:25 +0200 Subject: [PATCH 05/25] Clean up mbedtls_ssl_check_cert_usage() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_misc.h | 10 +++++----- library/ssl_tls.c | 11 +++++++---- library/ssl_tls12_server.c | 2 +- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 120f8ca3c..e00dcfcf4 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -1674,18 +1674,18 @@ static inline mbedtls_x509_crt *mbedtls_ssl_own_cert(mbedtls_ssl_context *ssl) } /* - * Check usage of a certificate wrt extensions: - * keyUsage, extendedKeyUsage (later), and nSCertType (later). + * Check usage of a certificate wrt usage extensions: + * keyUsage and extendedKeyUsage. + * (Note: nSCertType is deprecated and not standard, we don't check it.) * - * Warning: cert_endpoint is the endpoint of the cert (ie, of our peer when we - * check a cert we received from them)! + * Note: recv_endpoint is the receiver's endpoint. * * Return 0 if everything is OK, -1 if not. */ MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, const mbedtls_ssl_ciphersuite_t *ciphersuite, - int cert_endpoint, + int recv_endpoint, uint32_t *flags); #endif /* MBEDTLS_X509_CRT_PARSE_C */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 018eb93d8..865f984b9 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6361,7 +6361,7 @@ const char *mbedtls_ssl_get_curve_name_from_tls_id(uint16_t tls_id) #if defined(MBEDTLS_X509_CRT_PARSE_C) int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, const mbedtls_ssl_ciphersuite_t *ciphersuite, - int cert_endpoint, + int recv_endpoint, uint32_t *flags) { int ret = 0; @@ -6369,7 +6369,10 @@ int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, const char *ext_oid; size_t ext_len; - if (cert_endpoint == MBEDTLS_SSL_IS_SERVER) { + /* Note: don't guard this with MBEDTLS_SSL_CLI_C because the server wants + * to check what a compliant client will think while choosing which cert + * to send to the client. */ + if (recv_endpoint == MBEDTLS_SSL_IS_CLIENT) { /* Server part of the key exchange */ switch (ciphersuite->key_exchange) { case MBEDTLS_KEY_EXCHANGE_RSA: @@ -6406,7 +6409,7 @@ int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, ret = -1; } - if (cert_endpoint == MBEDTLS_SSL_IS_SERVER) { + if (recv_endpoint == MBEDTLS_SSL_IS_CLIENT) { ext_oid = MBEDTLS_OID_SERVER_AUTH; ext_len = MBEDTLS_OID_SIZE(MBEDTLS_OID_SERVER_AUTH); } else { @@ -8061,7 +8064,7 @@ static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, if (mbedtls_ssl_check_cert_usage(chain, ciphersuite_info, - !ssl->conf->endpoint, + ssl->conf->endpoint, &ssl->session_negotiate->verify_result) != 0) { MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate (usage extensions)")); if (ret == 0) { diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index 81ee6002e..e250fc03d 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -756,7 +756,7 @@ static int ssl_pick_cert(mbedtls_ssl_context *ssl, * and decrypting with the same RSA key. */ if (mbedtls_ssl_check_cert_usage(cur->cert, ciphersuite_info, - MBEDTLS_SSL_IS_SERVER, &flags) != 0) { + MBEDTLS_SSL_IS_CLIENT, &flags) != 0) { MBEDTLS_SSL_DEBUG_MSG(3, ("certificate mismatch: " "(extended) key usage extension")); continue; From 7a4aa4d13309add0650277dbdd89440d3a5ba67b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 9 Aug 2024 11:49:12 +0200 Subject: [PATCH 06/25] Make mbedtls_ssl_check_cert_usage() work for 1.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_misc.h | 3 +++ library/ssl_tls.c | 26 ++++++++++++++++++++++---- library/ssl_tls12_server.c | 4 +++- library/ssl_tls13_generic.c | 31 +++++-------------------------- 4 files changed, 33 insertions(+), 31 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index e00dcfcf4..997a38d5d 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -1678,6 +1678,8 @@ static inline mbedtls_x509_crt *mbedtls_ssl_own_cert(mbedtls_ssl_context *ssl) * keyUsage and extendedKeyUsage. * (Note: nSCertType is deprecated and not standard, we don't check it.) * + * Note: if tls_version is 1.3, ciphersuite is ignored and can be NULL. + * * Note: recv_endpoint is the receiver's endpoint. * * Return 0 if everything is OK, -1 if not. @@ -1686,6 +1688,7 @@ MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, const mbedtls_ssl_ciphersuite_t *ciphersuite, int recv_endpoint, + mbedtls_ssl_protocol_version tls_version, uint32_t *flags); #endif /* MBEDTLS_X509_CRT_PARSE_C */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 865f984b9..c1573c117 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6362,6 +6362,7 @@ const char *mbedtls_ssl_get_curve_name_from_tls_id(uint16_t tls_id) int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, const mbedtls_ssl_ciphersuite_t *ciphersuite, int recv_endpoint, + mbedtls_ssl_protocol_version tls_version, uint32_t *flags) { int ret = 0; @@ -6369,11 +6370,17 @@ int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, const char *ext_oid; size_t ext_len; + /* + * keyUsage + */ + /* Note: don't guard this with MBEDTLS_SSL_CLI_C because the server wants * to check what a compliant client will think while choosing which cert * to send to the client. */ - if (recv_endpoint == MBEDTLS_SSL_IS_CLIENT) { - /* Server part of the key exchange */ +#if defined(MBEDTLS_SSL_PROTO_TLS1_2) + if (tls_version == MBEDTLS_SSL_VERSION_TLS1_2 && + recv_endpoint == MBEDTLS_SSL_IS_CLIENT) { + /* TLS 1.2 server part of the key exchange */ switch (ciphersuite->key_exchange) { case MBEDTLS_KEY_EXCHANGE_RSA: case MBEDTLS_KEY_EXCHANGE_RSA_PSK: @@ -6399,8 +6406,14 @@ int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, case MBEDTLS_KEY_EXCHANGE_ECJPAKE: usage = 0; } - } else { - /* Client auth: we only implement rsa_sign and mbedtls_ecdsa_sign for now */ + } else +#endif + { + /* This is either TLS 1.3 autentication, which always uses signatures, + * or 1.2 client auth: rsa_sign and mbedtls_ecdsa_sign are the only + * options we implement, both using signatures. */ + (void) tls_version; + (void) ciphersuite; usage = MBEDTLS_X509_KU_DIGITAL_SIGNATURE; } @@ -6409,6 +6422,10 @@ int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, ret = -1; } + /* + * extKeyUsage + */ + if (recv_endpoint == MBEDTLS_SSL_IS_CLIENT) { ext_oid = MBEDTLS_OID_SERVER_AUTH; ext_len = MBEDTLS_OID_SIZE(MBEDTLS_OID_SERVER_AUTH); @@ -8065,6 +8082,7 @@ static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, if (mbedtls_ssl_check_cert_usage(chain, ciphersuite_info, ssl->conf->endpoint, + MBEDTLS_SSL_VERSION_TLS1_2, &ssl->session_negotiate->verify_result) != 0) { MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate (usage extensions)")); if (ret == 0) { diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index e250fc03d..03722ac33 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -756,7 +756,9 @@ static int ssl_pick_cert(mbedtls_ssl_context *ssl, * and decrypting with the same RSA key. */ if (mbedtls_ssl_check_cert_usage(cur->cert, ciphersuite_info, - MBEDTLS_SSL_IS_CLIENT, &flags) != 0) { + MBEDTLS_SSL_IS_CLIENT, + MBEDTLS_SSL_VERSION_TLS1_2, + &flags) != 0) { MBEDTLS_SSL_DEBUG_MSG(3, ("certificate mismatch: " "(extended) key usage extension")); continue; diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 651a17b5a..8d8af2b19 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -631,8 +631,6 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) int authmode = MBEDTLS_SSL_VERIFY_REQUIRED; mbedtls_x509_crt *ca_chain; mbedtls_x509_crl *ca_crl; - const char *ext_oid; - size_t ext_len; uint32_t verify_result = 0; /* If SNI was used, overwrite authentication mode @@ -714,34 +712,15 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) /* * Secondary checks: always done, but change 'ret' only if it was 0 */ - /* keyUsage */ - if ((mbedtls_x509_crt_check_key_usage( - ssl->session_negotiate->peer_cert, - MBEDTLS_X509_KU_DIGITAL_SIGNATURE) != 0)) { + if (mbedtls_ssl_check_cert_usage(ssl->session_negotiate->peer_cert, + NULL, + ssl->conf->endpoint, + MBEDTLS_SSL_VERSION_TLS1_3, + &verify_result) != 0) { MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate (usage extensions)")); if (ret == 0) { ret = MBEDTLS_ERR_SSL_BAD_CERTIFICATE; } - verify_result |= MBEDTLS_X509_BADCERT_KEY_USAGE; - } - - /* extKeyUsage */ - if (ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT) { - ext_oid = MBEDTLS_OID_SERVER_AUTH; - ext_len = MBEDTLS_OID_SIZE(MBEDTLS_OID_SERVER_AUTH); - } else { - ext_oid = MBEDTLS_OID_CLIENT_AUTH; - ext_len = MBEDTLS_OID_SIZE(MBEDTLS_OID_CLIENT_AUTH); - } - - if ((mbedtls_x509_crt_check_extended_key_usage( - ssl->session_negotiate->peer_cert, - ext_oid, ext_len) != 0)) { - MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate (usage extensions)")); - if (ret == 0) { - ret = MBEDTLS_ERR_SSL_BAD_CERTIFICATE; - } - verify_result |= MBEDTLS_X509_BADCERT_EXT_KEY_USAGE; } /* mbedtls_x509_crt_verify_with_profile is supposed to report a From e5a916fd3cb658dd20b9613320393bca2f966e49 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 07/25] 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 c1573c117..a7631bb37 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(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY) - { - 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 /* PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY */ + /* Check X.509 usage extensions (keyUsage, extKeyUsage) */ if (mbedtls_ssl_check_cert_usage(chain, ciphersuite_info, ssl->conf->endpoint, From 5f9428ac8a478b78942adae47080aa1bb38f045c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 9 Aug 2024 12:40:48 +0200 Subject: [PATCH 08/25] Rm translation code for unused flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don't check the non-standard nsCertType extension, so this flag can't be set, so checking if it's set is useless. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 2 -- library/ssl_tls13_generic.c | 1 - 2 files changed, 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a7631bb37..3bcf4f46d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8105,8 +8105,6 @@ static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_EXT_KEY_USAGE) { alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; - } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_NS_CERT_TYPE) { - alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_BAD_PK) { alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_BAD_KEY) { diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 8d8af2b19..5fdc52705 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -752,7 +752,6 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) MBEDTLS_SSL_PEND_FATAL_ALERT(MBEDTLS_SSL_ALERT_MSG_BAD_CERT, ret); } else if (verify_result & (MBEDTLS_X509_BADCERT_KEY_USAGE | MBEDTLS_X509_BADCERT_EXT_KEY_USAGE | - MBEDTLS_X509_BADCERT_NS_CERT_TYPE | MBEDTLS_X509_BADCERT_BAD_PK | MBEDTLS_X509_BADCERT_BAD_KEY)) { MBEDTLS_SSL_PEND_FATAL_ALERT( From aefc5938b0b73bd3174e6b32c6ebcb4371b29103 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 12 Aug 2024 10:36:40 +0200 Subject: [PATCH 09/25] Add comments about 1.3 server sending no cert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls13_generic.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 5fdc52705..8f8f8c27b 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -472,6 +472,7 @@ int mbedtls_ssl_tls13_parse_certificate(mbedtls_ssl_context *ssl, mbedtls_free(ssl->session_negotiate->peer_cert); } + /* This is used by ssl_tls13_validate_certificate() */ if (certificate_list_len == 0) { ssl->session_negotiate->peer_cert = NULL; ret = 0; @@ -675,6 +676,11 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) #endif /* MBEDTLS_SSL_SRV_C */ #if defined(MBEDTLS_SSL_CLI_C) + /* Regardless of authmode, the server is not allowed to send an empty + * certificate chain. (Last paragraph before 4.4.2.1 in RFC 8446: "The + * server's certificate_list MUST always be non-empty.") With authmode + * optional/none, we continue the handshake if we can't validate the + * server's cert, but we still break it if no certificate was sent. */ if (ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT) { MBEDTLS_SSL_PEND_FATAL_ALERT(MBEDTLS_SSL_ALERT_MSG_NO_CERT, MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE); From 58ab9ba0bd98b992e51390219c39a3c63bdbe5b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 14 Aug 2024 09:47:38 +0200 Subject: [PATCH 10/25] Allow optional authentication of the server in 1.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is for compatibility, for people transitioning from 1.2 to 1.3. See https://github.com/Mbed-TLS/mbedtls/issues/9223 "Mandatory server authentication" and reports linked from there. In the future we're likely to make server authentication mandatory in both 1.2 and 1.3. See https://github.com/Mbed-TLS/mbedtls/issues/7080 Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 24 +----------------------- library/ssl_tls13_generic.c | 19 +++++++------------ tests/ssl-opt.sh | 29 +++++++++++++++++++++++++++-- 3 files changed, 35 insertions(+), 37 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 3bcf4f46d..dd793d194 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1354,29 +1354,6 @@ static int ssl_conf_check(const mbedtls_ssl_context *ssl) return ret; } -#if defined(MBEDTLS_SSL_PROTO_TLS1_3) - /* RFC 8446 section 4.4.3 - * - * If the verification fails, the receiver MUST terminate the handshake with - * a "decrypt_error" alert. - * - * If the client is configured as TLS 1.3 only with optional verify, return - * bad config. - * - */ - if (mbedtls_ssl_conf_tls13_is_ephemeral_enabled( - (mbedtls_ssl_context *) ssl) && - ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT && - ssl->conf->max_tls_version == MBEDTLS_SSL_VERSION_TLS1_3 && - ssl->conf->min_tls_version == MBEDTLS_SSL_VERSION_TLS1_3 && - ssl->conf->authmode == MBEDTLS_SSL_VERIFY_OPTIONAL) { - MBEDTLS_SSL_DEBUG_MSG( - 1, ("Optional verify auth mode " - "is not available for TLS 1.3 client")); - return MBEDTLS_ERR_SSL_BAD_CONFIG; - } -#endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ - if (ssl->conf->f_rng == NULL) { MBEDTLS_SSL_DEBUG_MSG(1, ("no RNG provided")); return MBEDTLS_ERR_SSL_NO_RNG; @@ -8190,6 +8167,7 @@ int mbedtls_ssl_parse_certificate(mbedtls_ssl_context *ssl) { int ret = 0; int crt_expected; + /* Authmode: precedence order is SNI if used else configuration */ #if defined(MBEDTLS_SSL_SRV_C) && defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) const int authmode = ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET ? ssl->handshake->sni_authmode diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 8f8f8c27b..4b027de5a 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -629,22 +629,17 @@ MBEDTLS_CHECK_RETURN_CRITICAL static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) { int ret = 0; - int authmode = MBEDTLS_SSL_VERIFY_REQUIRED; mbedtls_x509_crt *ca_chain; mbedtls_x509_crl *ca_crl; uint32_t verify_result = 0; - /* If SNI was used, overwrite authentication mode - * from the configuration. */ -#if defined(MBEDTLS_SSL_SRV_C) - if (ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER) { -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if (ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET) { - authmode = ssl->handshake->sni_authmode; - } else -#endif - authmode = ssl->conf->authmode; - } + /* Authmode: precedence order is SNI if used else configuration */ +#if defined(MBEDTLS_SSL_SRV_C) && defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + const int authmode = ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET + ? ssl->handshake->sni_authmode + : ssl->conf->authmode; +#else + const int authmode = ssl->conf->authmode; #endif /* diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 7237920dd..18a292bbc 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5839,6 +5839,17 @@ run_test "Authentication: server badcert, client required (1.2)" \ # MBEDTLS_X509_BADCERT_NOT_TRUSTED -> MBEDTLS_SSL_ALERT_MSG_UNKNOWN_CA run_test "Authentication: server badcert, client optional" \ + "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ + key_file=$DATA_FILES_PATH/server5.key" \ + "$P_CLI force_version=tls13 debug_level=3 auth_mode=optional" \ + 0 \ + -c "x509_verify_cert() returned" \ + -c "! The certificate is not correctly signed by the trusted CA" \ + -C "! mbedtls_ssl_handshake returned" \ + -C "send alert level=2 message=48" \ + -C "X509 - Certificate verification failed" + +run_test "Authentication: server badcert, client optional (1.2)" \ "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ key_file=$DATA_FILES_PATH/server5.key" \ "$P_CLI force_version=tls12 debug_level=3 auth_mode=optional" \ @@ -5860,8 +5871,22 @@ run_test "Authentication: server badcert, client none" \ -C "send alert level=2 message=48" \ -C "X509 - Certificate verification failed" -requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT +# TODO: server goodcert, client none, no trusted CA + +requires_key_exchange_with_cert_in_tls12_or_tls13_enabled run_test "Authentication: server goodcert, client optional, no trusted CA" \ + "$P_SRV" \ + "$P_CLI debug_level=3 auth_mode=optional ca_file=none ca_path=none" \ + 0 \ + -c "x509_verify_cert() returned" \ + -c "! The certificate is not correctly signed by the trusted CA" \ + -c "! Certificate verification flags"\ + -C "! mbedtls_ssl_handshake returned" \ + -C "X509 - Certificate verification failed" \ + -C "SSL - No CA Chain is set, but required to operate" + +requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT +run_test "Authentication: server goodcert, client optional, no trusted CA (1.2)" \ "$P_SRV" \ "$P_CLI force_version=tls12 debug_level=3 auth_mode=optional ca_file=none ca_path=none" \ 0 \ @@ -6129,7 +6154,7 @@ requires_full_size_output_buffer run_test "Authentication: server max_int+1 chain, client optional" \ "$P_SRV crt_file=$DATA_FILES_PATH/dir-maxpath/c10.pem \ key_file=$DATA_FILES_PATH/dir-maxpath/10.key" \ - "$P_CLI force_version=tls12 server_name=CA10 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt \ + "$P_CLI server_name=CA10 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt \ auth_mode=optional" \ 1 \ -c "X509 - A fatal error occurred" From 18dd213114cf1c7940ab9e6fd072819a56a5a954 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 14 Aug 2024 10:34:53 +0200 Subject: [PATCH 11/25] Reorder some tests in ssl-opt.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tests above are required then optional then none. Follow the same pattern here. Just moving things around (see git's --color-moved option). Signed-off-by: Manuel Pégourié-Gonnard --- tests/ssl-opt.sh | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 18a292bbc..fef8527cb 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5871,7 +5871,27 @@ run_test "Authentication: server badcert, client none" \ -C "send alert level=2 message=48" \ -C "X509 - Certificate verification failed" -# TODO: server goodcert, client none, no trusted CA +requires_key_exchange_with_cert_in_tls12_or_tls13_enabled +run_test "Authentication: server goodcert, client required, no trusted CA" \ + "$P_SRV" \ + "$P_CLI debug_level=3 auth_mode=required ca_file=none ca_path=none" \ + 1 \ + -c "x509_verify_cert() returned" \ + -c "! The certificate is not correctly signed by the trusted CA" \ + -c "! Certificate verification flags"\ + -c "! mbedtls_ssl_handshake returned" \ + -c "SSL - No CA Chain is set, but required to operate" + +requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT +run_test "Authentication: server goodcert, client required, no trusted CA (1.2)" \ + "$P_SRV force_version=tls12" \ + "$P_CLI debug_level=3 auth_mode=required ca_file=none ca_path=none" \ + 1 \ + -c "x509_verify_cert() returned" \ + -c "! The certificate is not correctly signed by the trusted CA" \ + -c "! Certificate verification flags"\ + -c "! mbedtls_ssl_handshake returned" \ + -c "SSL - No CA Chain is set, but required to operate" requires_key_exchange_with_cert_in_tls12_or_tls13_enabled run_test "Authentication: server goodcert, client optional, no trusted CA" \ @@ -5897,27 +5917,7 @@ run_test "Authentication: server goodcert, client optional, no trusted CA (1. -C "X509 - Certificate verification failed" \ -C "SSL - No CA Chain is set, but required to operate" -requires_key_exchange_with_cert_in_tls12_or_tls13_enabled -run_test "Authentication: server goodcert, client required, no trusted CA" \ - "$P_SRV" \ - "$P_CLI debug_level=3 auth_mode=required ca_file=none ca_path=none" \ - 1 \ - -c "x509_verify_cert() returned" \ - -c "! The certificate is not correctly signed by the trusted CA" \ - -c "! Certificate verification flags"\ - -c "! mbedtls_ssl_handshake returned" \ - -c "SSL - No CA Chain is set, but required to operate" - -requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT -run_test "Authentication: server goodcert, client required, no trusted CA (1.2)" \ - "$P_SRV force_version=tls12" \ - "$P_CLI debug_level=3 auth_mode=required ca_file=none ca_path=none" \ - 1 \ - -c "x509_verify_cert() returned" \ - -c "! The certificate is not correctly signed by the trusted CA" \ - -c "! Certificate verification flags"\ - -c "! mbedtls_ssl_handshake returned" \ - -c "SSL - No CA Chain is set, but required to operate" +# TODO: server goodcert, client none, no trusted CA # The purpose of the next two tests is to test the client's behaviour when receiving a server # certificate with an unsupported elliptic curve. This should usually not happen because From 6901504ddb08d2e20e8a64f62d551106335154b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 14 Aug 2024 10:44:02 +0200 Subject: [PATCH 12/25] Allow no authentication of the server in 1.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See notes about optional two commits ago for why we're doing this. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls13_generic.c | 12 ++++++++++++ tests/ssl-opt.sh | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 4b027de5a..2104567c8 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -684,6 +684,18 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) #endif /* MBEDTLS_SSL_CLI_C */ } + /* + * NONE means we skip all checks + * + * Note: we still check above that the server did send a certificate, + * because only a non-compliant server would fail to do so. NONE means we + * don't care about the server certificate being valid, but we still care + * about the server otherwise following the TLS standard. + */ + if (authmode == MBEDTLS_SSL_VERIFY_NONE) { + return 0; + } + #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) if (ssl->handshake->sni_ca_chain != NULL) { ca_chain = ssl->handshake->sni_ca_chain; diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index fef8527cb..e32365492 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5861,6 +5861,17 @@ run_test "Authentication: server badcert, client optional (1.2)" \ -C "X509 - Certificate verification failed" run_test "Authentication: server badcert, client none" \ + "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ + key_file=$DATA_FILES_PATH/server5.key" \ + "$P_CLI debug_level=3 auth_mode=none" \ + 0 \ + -C "x509_verify_cert() returned" \ + -C "! The certificate is not correctly signed by the trusted CA" \ + -C "! mbedtls_ssl_handshake returned" \ + -C "send alert level=2 message=48" \ + -C "X509 - Certificate verification failed" + +run_test "Authentication: server badcert, client none (1.2)" \ "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ key_file=$DATA_FILES_PATH/server5.key" \ "$P_CLI force_version=tls12 debug_level=3 auth_mode=none" \ @@ -5917,7 +5928,29 @@ run_test "Authentication: server goodcert, client optional, no trusted CA (1. -C "X509 - Certificate verification failed" \ -C "SSL - No CA Chain is set, but required to operate" -# TODO: server goodcert, client none, no trusted CA +requires_key_exchange_with_cert_in_tls12_or_tls13_enabled +run_test "Authentication: server goodcert, client none, no trusted CA" \ + "$P_SRV" \ + "$P_CLI debug_level=3 auth_mode=none ca_file=none ca_path=none" \ + 0 \ + -C "x509_verify_cert() returned" \ + -C "! The certificate is not correctly signed by the trusted CA" \ + -C "! Certificate verification flags"\ + -C "! mbedtls_ssl_handshake returned" \ + -C "X509 - Certificate verification failed" \ + -C "SSL - No CA Chain is set, but required to operate" + +requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT +run_test "Authentication: server goodcert, client none, no trusted CA (1.2)" \ + "$P_SRV" \ + "$P_CLI force_version=tls12 debug_level=3 auth_mode=none ca_file=none ca_path=none" \ + 0 \ + -C "x509_verify_cert() returned" \ + -C "! The certificate is not correctly signed by the trusted CA" \ + -C "! Certificate verification flags"\ + -C "! mbedtls_ssl_handshake returned" \ + -C "X509 - Certificate verification failed" \ + -C "SSL - No CA Chain is set, but required to operate" # The purpose of the next two tests is to test the client's behaviour when receiving a server # certificate with an unsupported elliptic curve. This should usually not happen because From 7a442c9941df1223508b4b68b9efd37b3acb95df Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Wed, 3 Apr 2024 08:57:09 +0200 Subject: [PATCH 13/25] ssl-opt.sh: Fix test case titles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Ronald Cron Signed-off-by: Manuel Pégourié-Gonnard --- tests/ssl-opt.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index e32365492..10cb3b821 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -6359,7 +6359,7 @@ run_test "Authentication, CA callback: server ECDH p256v1, client optional, p requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT -run_test "Authentication, CA callback: client SHA256, server required" \ +run_test "Authentication, CA callback: client SHA384, server required" \ "$P_SRV ca_callback=1 debug_level=3 auth_mode=required" \ "$P_CLI debug_level=3 crt_file=$DATA_FILES_PATH/server6.crt \ key_file=$DATA_FILES_PATH/server6.key \ @@ -6371,7 +6371,7 @@ run_test "Authentication, CA callback: client SHA256, server required" \ requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK requires_any_configs_enabled $TLS1_2_KEY_EXCHANGES_WITH_CERT -run_test "Authentication, CA callback: client SHA384, server required" \ +run_test "Authentication, CA callback: client SHA256, server required" \ "$P_SRV ca_callback=1 debug_level=3 auth_mode=required" \ "$P_CLI debug_level=3 crt_file=$DATA_FILES_PATH/server6.crt \ key_file=$DATA_FILES_PATH/server6.key \ From bfbecf8b34dc362ffe194f5de2eec4031f41a557 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Wed, 3 Apr 2024 09:07:22 +0200 Subject: [PATCH 14/25] tls13: Add support for trusted certificate callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Ronald Cron Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls13_generic.c | 60 +++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 2104567c8..c130de0a8 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -629,6 +629,7 @@ MBEDTLS_CHECK_RETURN_CRITICAL static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) { int ret = 0; + int have_ca_chain = 0; mbedtls_x509_crt *ca_chain; mbedtls_x509_crl *ca_crl; uint32_t verify_result = 0; @@ -696,27 +697,48 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) return 0; } -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if (ssl->handshake->sni_ca_chain != NULL) { - ca_chain = ssl->handshake->sni_ca_chain; - ca_crl = ssl->handshake->sni_ca_crl; - } else -#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ - { - ca_chain = ssl->conf->ca_chain; - ca_crl = ssl->conf->ca_crl; - } - /* * Main check: verify certificate */ - ret = mbedtls_x509_crt_verify_with_profile( - ssl->session_negotiate->peer_cert, - ca_chain, ca_crl, - ssl->conf->cert_profile, - ssl->hostname, - &verify_result, - ssl->conf->f_vrfy, ssl->conf->p_vrfy); +#if defined(MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK) + if (ssl->conf->f_ca_cb != NULL) { + have_ca_chain = 1; + + MBEDTLS_SSL_DEBUG_MSG(3, ("use CA callback for X.509 CRT verification")); + ret = mbedtls_x509_crt_verify_with_ca_cb( + ssl->session_negotiate->peer_cert, + ssl->conf->f_ca_cb, + ssl->conf->p_ca_cb, + ssl->conf->cert_profile, + ssl->hostname, + &verify_result, + ssl->conf->f_vrfy, ssl->conf->p_vrfy); + } else +#endif /* MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK */ + { +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + if (ssl->handshake->sni_ca_chain != NULL) { + ca_chain = ssl->handshake->sni_ca_chain; + ca_crl = ssl->handshake->sni_ca_crl; + } else +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ + { + ca_chain = ssl->conf->ca_chain; + ca_crl = ssl->conf->ca_crl; + } + + if (ca_chain != NULL) { + have_ca_chain = 1; + } + + ret = mbedtls_x509_crt_verify_with_profile( + ssl->session_negotiate->peer_cert, + ca_chain, ca_crl, + ssl->conf->cert_profile, + ssl->hostname, + &verify_result, + ssl->conf->f_vrfy, ssl->conf->p_vrfy); + } if (ret != 0) { MBEDTLS_SSL_DEBUG_RET(1, "x509_verify_cert", ret); @@ -749,7 +771,7 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) ret = 0; } - if (ca_chain == NULL && authmode == MBEDTLS_SSL_VERIFY_REQUIRED) { + if (!have_ca_chain && authmode == MBEDTLS_SSL_VERIFY_REQUIRED) { MBEDTLS_SSL_DEBUG_MSG(1, ("got no CA chain")); ret = MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED; } From 95dd6f57cd11028bccaf9035e4b1587673fd09f2 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Wed, 3 Apr 2024 09:10:02 +0200 Subject: [PATCH 15/25] ssl-opt.sh: Test trusted certificate callback in TLS 1.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Ronald Cron Signed-off-by: Manuel Pégourié-Gonnard --- tests/ssl-opt.sh | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 10cb3b821..553766248 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -2155,7 +2155,7 @@ run_test "TLS: password protected server key, two certificates" \ requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK run_test "CA callback on client" \ "$P_SRV debug_level=3" \ - "$P_CLI force_version=tls12 ca_callback=1 debug_level=3 " \ + "$P_CLI ca_callback=1 debug_level=3 " \ 0 \ -c "use CA callback for X.509 CRT verification" \ -S "error" \ @@ -2165,7 +2165,7 @@ requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK requires_config_enabled MBEDTLS_X509_CRT_PARSE_C requires_hash_alg SHA_256 run_test "CA callback on server" \ - "$P_SRV force_version=tls12 auth_mode=required" \ + "$P_SRV auth_mode=required" \ "$P_CLI ca_callback=1 debug_level=3 crt_file=$DATA_FILES_PATH/server5.crt \ key_file=$DATA_FILES_PATH/server5.key" \ 0 \ @@ -6308,7 +6308,7 @@ requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK run_test "Authentication, CA callback: server badcert, client required" \ "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ key_file=$DATA_FILES_PATH/server5.key" \ - "$P_CLI force_version=tls12 ca_callback=1 debug_level=3 auth_mode=required" \ + "$P_CLI ca_callback=1 debug_level=3 auth_mode=required" \ 1 \ -c "use CA callback for X.509 CRT verification" \ -c "x509_verify_cert() returned" \ @@ -6320,7 +6320,7 @@ requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK run_test "Authentication, CA callback: server badcert, client optional" \ "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ key_file=$DATA_FILES_PATH/server5.key" \ - "$P_CLI force_version=tls12 ca_callback=1 debug_level=3 auth_mode=optional" \ + "$P_CLI ca_callback=1 debug_level=3 auth_mode=optional" \ 0 \ -c "use CA callback for X.509 CRT verification" \ -c "x509_verify_cert() returned" \ @@ -6328,6 +6328,18 @@ run_test "Authentication, CA callback: server badcert, client optional" \ -C "! mbedtls_ssl_handshake returned" \ -C "X509 - Certificate verification failed" +requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK +run_test "Authentication, CA callback: server badcert, client none" \ + "$P_SRV crt_file=$DATA_FILES_PATH/server5-badsign.crt \ + key_file=$DATA_FILES_PATH/server5.key" \ + "$P_CLI ca_callback=1 debug_level=3 auth_mode=none" \ + 0 \ + -C "use CA callback for X.509 CRT verification" \ + -C "x509_verify_cert() returned" \ + -C "! The certificate is not correctly signed by the trusted CA" \ + -C "! mbedtls_ssl_handshake returned" \ + -C "X509 - Certificate verification failed" + # The purpose of the next two tests is to test the client's behaviour when receiving a server # certificate with an unsupported elliptic curve. This should usually not happen because # the client informs the server about the supported curves - it does, though, in the @@ -6383,7 +6395,7 @@ run_test "Authentication, CA callback: client SHA256, server required" \ requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK run_test "Authentication, CA callback: client badcert, server required" \ - "$P_SRV force_version=tls12 ca_callback=1 debug_level=3 auth_mode=required" \ + "$P_SRV ca_callback=1 debug_level=3 auth_mode=required" \ "$P_CLI debug_level=3 crt_file=$DATA_FILES_PATH/server5-badsign.crt \ key_file=$DATA_FILES_PATH/server5.key" \ 1 \ @@ -6398,7 +6410,6 @@ run_test "Authentication, CA callback: client badcert, server required" \ -s "! The certificate is not correctly signed by the trusted CA" \ -s "! mbedtls_ssl_handshake returned" \ -s "send alert level=2 message=48" \ - -c "! mbedtls_ssl_handshake returned" \ -s "X509 - Certificate verification failed" # We don't check that the client receives the alert because it might # detect that its write end of the connection is closed and abort @@ -6406,7 +6417,7 @@ run_test "Authentication, CA callback: client badcert, server required" \ requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK run_test "Authentication, CA callback: client cert not trusted, server required" \ - "$P_SRV force_version=tls12 ca_callback=1 debug_level=3 auth_mode=required" \ + "$P_SRV ca_callback=1 debug_level=3 auth_mode=required" \ "$P_CLI debug_level=3 crt_file=$DATA_FILES_PATH/server5-selfsigned.crt \ key_file=$DATA_FILES_PATH/server5.key" \ 1 \ @@ -6420,12 +6431,11 @@ run_test "Authentication, CA callback: client cert not trusted, server requir -s "x509_verify_cert() returned" \ -s "! The certificate is not correctly signed by the trusted CA" \ -s "! mbedtls_ssl_handshake returned" \ - -c "! mbedtls_ssl_handshake returned" \ -s "X509 - Certificate verification failed" requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK run_test "Authentication, CA callback: client badcert, server optional" \ - "$P_SRV force_version=tls12 ca_callback=1 debug_level=3 auth_mode=optional" \ + "$P_SRV ca_callback=1 debug_level=3 auth_mode=optional" \ "$P_CLI debug_level=3 crt_file=$DATA_FILES_PATH/server5-badsign.crt \ key_file=$DATA_FILES_PATH/server5.key" \ 0 \ @@ -6448,7 +6458,7 @@ requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK run_test "Authentication, CA callback: server max_int chain, client default" \ "$P_SRV crt_file=$DATA_FILES_PATH/dir-maxpath/c09.pem \ key_file=$DATA_FILES_PATH/dir-maxpath/09.key" \ - "$P_CLI force_version=tls12 ca_callback=1 debug_level=3 server_name=CA09 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt" \ + "$P_CLI ca_callback=1 debug_level=3 server_name=CA09 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt" \ 0 \ -c "use CA callback for X.509 CRT verification" \ -C "X509 - A fatal error occurred" @@ -6459,7 +6469,7 @@ requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK run_test "Authentication, CA callback: server max_int+1 chain, client default" \ "$P_SRV crt_file=$DATA_FILES_PATH/dir-maxpath/c10.pem \ key_file=$DATA_FILES_PATH/dir-maxpath/10.key" \ - "$P_CLI force_version=tls12 debug_level=3 ca_callback=1 server_name=CA10 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt" \ + "$P_CLI debug_level=3 ca_callback=1 server_name=CA10 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt" \ 1 \ -c "use CA callback for X.509 CRT verification" \ -c "X509 - A fatal error occurred" @@ -6470,7 +6480,7 @@ requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK run_test "Authentication, CA callback: server max_int+1 chain, client optional" \ "$P_SRV crt_file=$DATA_FILES_PATH/dir-maxpath/c10.pem \ key_file=$DATA_FILES_PATH/dir-maxpath/10.key" \ - "$P_CLI force_version=tls12 ca_callback=1 server_name=CA10 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt \ + "$P_CLI ca_callback=1 server_name=CA10 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt \ debug_level=3 auth_mode=optional" \ 1 \ -c "use CA callback for X.509 CRT verification" \ @@ -6480,7 +6490,7 @@ requires_config_value_equals "MBEDTLS_X509_MAX_INTERMEDIATE_CA" $MAX_IM_CA requires_full_size_output_buffer requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK run_test "Authentication, CA callback: client max_int+1 chain, server optional" \ - "$P_SRV force_version=tls12 ca_callback=1 debug_level=3 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt auth_mode=optional" \ + "$P_SRV ca_callback=1 debug_level=3 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt auth_mode=optional" \ "$P_CLI crt_file=$DATA_FILES_PATH/dir-maxpath/c10.pem \ key_file=$DATA_FILES_PATH/dir-maxpath/10.key" \ 1 \ @@ -6491,7 +6501,7 @@ requires_config_value_equals "MBEDTLS_X509_MAX_INTERMEDIATE_CA" $MAX_IM_CA requires_full_size_output_buffer requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK run_test "Authentication, CA callback: client max_int+1 chain, server required" \ - "$P_SRV force_version=tls12 ca_callback=1 debug_level=3 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt auth_mode=required" \ + "$P_SRV ca_callback=1 debug_level=3 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt auth_mode=required" \ "$P_CLI crt_file=$DATA_FILES_PATH/dir-maxpath/c10.pem \ key_file=$DATA_FILES_PATH/dir-maxpath/10.key" \ 1 \ @@ -6502,7 +6512,7 @@ requires_config_value_equals "MBEDTLS_X509_MAX_INTERMEDIATE_CA" $MAX_IM_CA requires_full_size_output_buffer requires_config_enabled MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK run_test "Authentication, CA callback: client max_int chain, server required" \ - "$P_SRV force_version=tls12 ca_callback=1 debug_level=3 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt auth_mode=required" \ + "$P_SRV ca_callback=1 debug_level=3 ca_file=$DATA_FILES_PATH/dir-maxpath/00.crt auth_mode=required" \ "$P_CLI crt_file=$DATA_FILES_PATH/dir-maxpath/c09.pem \ key_file=$DATA_FILES_PATH/dir-maxpath/09.key" \ 0 \ From 5bdadbb1ebc3703a90881d09c71d7fb51ae90726 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 14 Aug 2024 12:51:00 +0200 Subject: [PATCH 16/25] Restrict the scope of a few variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In particular, make sure pointer variables are initialized right after being declared. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 6 ++---- library/ssl_tls13_generic.c | 4 ++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index dd793d194..e3bb48477 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7949,13 +7949,12 @@ static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, ssl->handshake->ciphersuite_info; int have_ca_chain = 0; - int (*f_vrfy)(void *, mbedtls_x509_crt *, int, uint32_t *); - void *p_vrfy; - if (authmode == MBEDTLS_SSL_VERIFY_NONE) { return 0; } + int (*f_vrfy)(void *, mbedtls_x509_crt *, int, uint32_t *); + void *p_vrfy; if (ssl->f_vrfy != NULL) { MBEDTLS_SSL_DEBUG_MSG(3, ("Use context-specific verification callback")); f_vrfy = ssl->f_vrfy; @@ -7988,7 +7987,6 @@ static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, { mbedtls_x509_crt *ca_chain; mbedtls_x509_crl *ca_crl; - #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) if (ssl->handshake->sni_ca_chain != NULL) { ca_chain = ssl->handshake->sni_ca_chain; diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index c130de0a8..f883a22f4 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -630,8 +630,6 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) { int ret = 0; int have_ca_chain = 0; - mbedtls_x509_crt *ca_chain; - mbedtls_x509_crl *ca_crl; uint32_t verify_result = 0; /* Authmode: precedence order is SNI if used else configuration */ @@ -716,6 +714,8 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) } else #endif /* MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK */ { + mbedtls_x509_crt *ca_chain; + mbedtls_x509_crl *ca_crl; #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) if (ssl->handshake->sni_ca_chain != NULL) { ca_chain = ssl->handshake->sni_ca_chain; From fd800c2416bea26a618a0b94657c7e4f6621d4d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 14 Aug 2024 12:52:59 +0200 Subject: [PATCH 17/25] Improve a variable's name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 8 ++++---- library/ssl_tls13_generic.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e3bb48477..148924ab0 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7947,7 +7947,7 @@ static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, int ret = 0; const mbedtls_ssl_ciphersuite_t *ciphersuite_info = ssl->handshake->ciphersuite_info; - int have_ca_chain = 0; + int have_ca_chain_or_callback = 0; if (authmode == MBEDTLS_SSL_VERIFY_NONE) { return 0; @@ -7971,7 +7971,7 @@ static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, #if defined(MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK) if (ssl->conf->f_ca_cb != NULL) { ((void) rs_ctx); - have_ca_chain = 1; + have_ca_chain_or_callback = 1; MBEDTLS_SSL_DEBUG_MSG(3, ("use CA callback for X.509 CRT verification")); ret = mbedtls_x509_crt_verify_with_ca_cb( @@ -7999,7 +7999,7 @@ static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, } if (ca_chain != NULL) { - have_ca_chain = 1; + have_ca_chain_or_callback = 1; } ret = mbedtls_x509_crt_verify_restartable( @@ -8061,7 +8061,7 @@ static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, ret = 0; } - if (have_ca_chain == 0 && authmode == MBEDTLS_SSL_VERIFY_REQUIRED) { + if (have_ca_chain_or_callback == 0 && authmode == MBEDTLS_SSL_VERIFY_REQUIRED) { MBEDTLS_SSL_DEBUG_MSG(1, ("got no CA chain")); ret = MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED; } diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index f883a22f4..6ea5e01d4 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -629,7 +629,7 @@ MBEDTLS_CHECK_RETURN_CRITICAL static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) { int ret = 0; - int have_ca_chain = 0; + int have_ca_chain_or_callback = 0; uint32_t verify_result = 0; /* Authmode: precedence order is SNI if used else configuration */ @@ -700,7 +700,7 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) */ #if defined(MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK) if (ssl->conf->f_ca_cb != NULL) { - have_ca_chain = 1; + have_ca_chain_or_callback = 1; MBEDTLS_SSL_DEBUG_MSG(3, ("use CA callback for X.509 CRT verification")); ret = mbedtls_x509_crt_verify_with_ca_cb( @@ -728,7 +728,7 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) } if (ca_chain != NULL) { - have_ca_chain = 1; + have_ca_chain_or_callback = 1; } ret = mbedtls_x509_crt_verify_with_profile( @@ -771,7 +771,7 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) ret = 0; } - if (!have_ca_chain && authmode == MBEDTLS_SSL_VERIFY_REQUIRED) { + if (!have_ca_chain_or_callback && authmode == MBEDTLS_SSL_VERIFY_REQUIRED) { MBEDTLS_SSL_DEBUG_MSG(1, ("got no CA chain")); ret = MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED; } From 843a00dec68c5c90fd00ce18dab975115fd9517d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 16 Aug 2024 09:53:41 +0200 Subject: [PATCH 18/25] Add support for context f_vrfy callback in 1.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was only supported in 1.2 for no good reason. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 1 + library/ssl_tls13_generic.c | 17 +++++++++++++++-- tests/ssl-opt.sh | 4 ++-- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 148924ab0..7db0f6dd4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7953,6 +7953,7 @@ static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, return 0; } + /* Verify callback: precedence order is SSL context, else conf struct. */ int (*f_vrfy)(void *, mbedtls_x509_crt *, int, uint32_t *); void *p_vrfy; if (ssl->f_vrfy != NULL) { diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 6ea5e01d4..fb57aa4a7 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -695,6 +695,19 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) return 0; } + /* Verify callback: precedence order is SSL context, else conf struct. */ + int (*f_vrfy)(void *, mbedtls_x509_crt *, int, uint32_t *); + void *p_vrfy; + if (ssl->f_vrfy != NULL) { + MBEDTLS_SSL_DEBUG_MSG(3, ("Use context-specific verification callback")); + f_vrfy = ssl->f_vrfy; + p_vrfy = ssl->p_vrfy; + } else { + MBEDTLS_SSL_DEBUG_MSG(3, ("Use configuration-specific verification callback")); + f_vrfy = ssl->conf->f_vrfy; + p_vrfy = ssl->conf->p_vrfy; + } + /* * Main check: verify certificate */ @@ -710,7 +723,7 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) ssl->conf->cert_profile, ssl->hostname, &verify_result, - ssl->conf->f_vrfy, ssl->conf->p_vrfy); + f_vrfy, p_vrfy); } else #endif /* MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK */ { @@ -737,7 +750,7 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) ssl->conf->cert_profile, ssl->hostname, &verify_result, - ssl->conf->f_vrfy, ssl->conf->p_vrfy); + f_vrfy, p_vrfy); } if (ret != 0) { diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 553766248..6b93fda4b 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -2724,7 +2724,7 @@ run_test "Single supported algorithm sending: openssl client" \ # Tests for certificate verification callback run_test "Configuration-specific CRT verification callback" \ "$P_SRV debug_level=3" \ - "$P_CLI force_version=tls12 context_crt_cb=0 debug_level=3" \ + "$P_CLI context_crt_cb=0 debug_level=3" \ 0 \ -S "error" \ -c "Verify requested for " \ @@ -2734,7 +2734,7 @@ run_test "Configuration-specific CRT verification callback" \ run_test "Context-specific CRT verification callback" \ "$P_SRV debug_level=3" \ - "$P_CLI force_version=tls12 context_crt_cb=1 debug_level=3" \ + "$P_CLI context_crt_cb=1 debug_level=3" \ 0 \ -S "error" \ -c "Verify requested for " \ From 908f57dfbaea05ad2c2e89f6b86da5bddff1871f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 16 Aug 2024 10:01:48 +0200 Subject: [PATCH 19/25] Minor refactoring of generic SSL certificate verif MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename as there was a name collision with a static function in another file: ssl_parse_certificate_verify in ssl_tls12_server.c is the function that parses the CertificateVerify message, which seems appropriate. Here it meant "the 'verify' step after parsing the Certificate message". Use a name that focuses on what it does: verify, not parse. Also, take ciphersuite_info as an argument: when TLS 1.3 calls this function, it can pass NULL as the ciphersuite has no influence there. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 7db0f6dd4..e87f985e5 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7939,14 +7939,13 @@ static int ssl_parse_certificate_coordinate(mbedtls_ssl_context *ssl, } MBEDTLS_CHECK_RETURN_CRITICAL -static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, - int authmode, - mbedtls_x509_crt *chain, - void *rs_ctx) +static int ssl_verify_certificate(mbedtls_ssl_context *ssl, + int authmode, + mbedtls_x509_crt *chain, + const mbedtls_ssl_ciphersuite_t *ciphersuite_info, + void *rs_ctx) { int ret = 0; - const mbedtls_ssl_ciphersuite_t *ciphersuite_info = - ssl->handshake->ciphersuite_info; int have_ca_chain_or_callback = 0; if (authmode == MBEDTLS_SSL_VERIFY_NONE) { @@ -8246,8 +8245,8 @@ crt_verify: } #endif - ret = ssl_parse_certificate_verify(ssl, authmode, - chain, rs_ctx); + ret = ssl_verify_certificate(ssl, authmode, chain, + ssl->handshake->ciphersuite_info, rs_ctx); if (ret != 0) { goto exit; } From 19dd9f59bce259eaf759bc7126b8bd9ec0a79290 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 16 Aug 2024 11:03:42 +0200 Subject: [PATCH 20/25] Merge 1.2 and 1.3 certificate verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_misc.h | 32 ++++++++ library/ssl_tls.c | 45 ++++++----- library/ssl_tls13_generic.c | 153 +----------------------------------- 3 files changed, 62 insertions(+), 168 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 997a38d5d..b40f3355c 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -1673,6 +1673,38 @@ static inline mbedtls_x509_crt *mbedtls_ssl_own_cert(mbedtls_ssl_context *ssl) return key_cert == NULL ? NULL : key_cert->cert; } +/* + * Verify a certificate. + * + * [in/out] ssl: misc. things read + * ssl->session_negotiate->verify_result updated + * [in] authmode: one of MBEDTLS_SSL_VERIFY_{NONE,OPTIONAL,REQUIRED} + * [in] chain: the certificate chain to verify (ie the peer's chain) + * [in] ciphersuite_info: For TLS 1.2, this session's ciphersuite; + * for TLS 1.3, may be left NULL. + * [in] rs_ctx: restart context if restartable ECC is in use; + * leave NULL for no restartable behaviour. + * + * Return: + * - 0 if the certificate is the handshake should continue. Depending on the + * authmode it means: + * - REQUIRED: the certificate was found to be valid, trusted & acceptable. + * ssl->session_negotiate->verify_result is 0. + * - OPTIONAL: the certificate may or may not be acceptable, but + * ssl->session_negotiate->verify_result was updated with the result. + * - NONE: the certificate wasn't even checked. + * - MBEDTLS_ERR_X509_CERT_VERIFY_FAILED or MBEDTLS_ERR_SSL_BAD_CERTIFICATE if + * the certificate was found to be invalid/untrusted/unacceptable and the + * handshake should be aborted (can only happen with REQUIRED). + * - another error code if another error happened (out-of-memory, etc.) + */ +MBEDTLS_CHECK_RETURN_CRITICAL +int mbedtls_ssl_verify_certificate(mbedtls_ssl_context *ssl, + int authmode, + mbedtls_x509_crt *chain, + const mbedtls_ssl_ciphersuite_t *ciphersuite_info, + void *rs_ctx); + /* * Check usage of a certificate wrt usage extensions: * keyUsage and extendedKeyUsage. diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e87f985e5..fb48b95c1 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7938,12 +7938,11 @@ static int ssl_parse_certificate_coordinate(mbedtls_ssl_context *ssl, return SSL_CERTIFICATE_EXPECTED; } -MBEDTLS_CHECK_RETURN_CRITICAL -static int ssl_verify_certificate(mbedtls_ssl_context *ssl, - int authmode, - mbedtls_x509_crt *chain, - const mbedtls_ssl_ciphersuite_t *ciphersuite_info, - void *rs_ctx) +int mbedtls_ssl_verify_certificate(mbedtls_ssl_context *ssl, + int authmode, + mbedtls_x509_crt *chain, + const mbedtls_ssl_ciphersuite_t *ciphersuite_info, + void *rs_ctx) { int ret = 0; int have_ca_chain_or_callback = 0; @@ -8025,23 +8024,32 @@ static int ssl_verify_certificate(mbedtls_ssl_context *ssl, * Secondary checks: always done, but change 'ret' only if it was 0 */ - /* Check curve for ECC certs */ -#if defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY) - 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; + /* With TLS 1.2 and ECC certs, check that the curve used by the + * certificate is on our list of acceptable curves. + * + * With TLS 1.3 this is not needed because the curve is part of the + * signature algorithm (eg ecdsa_secp256r1_sha256) which is checked when + * we validate the signature made with the key associated to this cert. + */ +#if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \ + defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY) + if (ssl->tls_version == MBEDTLS_SSL_VERSION_TLS1_2 && + mbedtls_pk_can_do(&chain->pk, MBEDTLS_PK_ECKEY)) { + if (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 /* PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY */ +#endif /* MBEDTLS_SSL_PROTO_TLS1_2 && PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY */ /* Check X.509 usage extensions (keyUsage, extKeyUsage) */ if (mbedtls_ssl_check_cert_usage(chain, ciphersuite_info, ssl->conf->endpoint, - MBEDTLS_SSL_VERSION_TLS1_2, + ssl->tls_version, &ssl->session_negotiate->verify_result) != 0) { MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate (usage extensions)")); if (ret == 0) { @@ -8245,8 +8253,9 @@ crt_verify: } #endif - ret = ssl_verify_certificate(ssl, authmode, chain, - ssl->handshake->ciphersuite_info, rs_ctx); + ret = mbedtls_ssl_verify_certificate(ssl, authmode, chain, + ssl->handshake->ciphersuite_info, + rs_ctx); if (ret != 0) { goto exit; } diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index fb57aa4a7..3f1f551dd 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -628,10 +628,6 @@ int mbedtls_ssl_tls13_parse_certificate(mbedtls_ssl_context *ssl, MBEDTLS_CHECK_RETURN_CRITICAL static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) { - int ret = 0; - int have_ca_chain_or_callback = 0; - uint32_t verify_result = 0; - /* Authmode: precedence order is SNI if used else configuration */ #if defined(MBEDTLS_SSL_SRV_C) && defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) const int authmode = ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET @@ -683,152 +679,9 @@ static int ssl_tls13_validate_certificate(mbedtls_ssl_context *ssl) #endif /* MBEDTLS_SSL_CLI_C */ } - /* - * NONE means we skip all checks - * - * Note: we still check above that the server did send a certificate, - * because only a non-compliant server would fail to do so. NONE means we - * don't care about the server certificate being valid, but we still care - * about the server otherwise following the TLS standard. - */ - if (authmode == MBEDTLS_SSL_VERIFY_NONE) { - return 0; - } - - /* Verify callback: precedence order is SSL context, else conf struct. */ - int (*f_vrfy)(void *, mbedtls_x509_crt *, int, uint32_t *); - void *p_vrfy; - if (ssl->f_vrfy != NULL) { - MBEDTLS_SSL_DEBUG_MSG(3, ("Use context-specific verification callback")); - f_vrfy = ssl->f_vrfy; - p_vrfy = ssl->p_vrfy; - } else { - MBEDTLS_SSL_DEBUG_MSG(3, ("Use configuration-specific verification callback")); - f_vrfy = ssl->conf->f_vrfy; - p_vrfy = ssl->conf->p_vrfy; - } - - /* - * Main check: verify certificate - */ -#if defined(MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK) - if (ssl->conf->f_ca_cb != NULL) { - have_ca_chain_or_callback = 1; - - MBEDTLS_SSL_DEBUG_MSG(3, ("use CA callback for X.509 CRT verification")); - ret = mbedtls_x509_crt_verify_with_ca_cb( - ssl->session_negotiate->peer_cert, - ssl->conf->f_ca_cb, - ssl->conf->p_ca_cb, - ssl->conf->cert_profile, - ssl->hostname, - &verify_result, - f_vrfy, p_vrfy); - } else -#endif /* MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK */ - { - mbedtls_x509_crt *ca_chain; - mbedtls_x509_crl *ca_crl; -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if (ssl->handshake->sni_ca_chain != NULL) { - ca_chain = ssl->handshake->sni_ca_chain; - ca_crl = ssl->handshake->sni_ca_crl; - } else -#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ - { - ca_chain = ssl->conf->ca_chain; - ca_crl = ssl->conf->ca_crl; - } - - if (ca_chain != NULL) { - have_ca_chain_or_callback = 1; - } - - ret = mbedtls_x509_crt_verify_with_profile( - ssl->session_negotiate->peer_cert, - ca_chain, ca_crl, - ssl->conf->cert_profile, - ssl->hostname, - &verify_result, - f_vrfy, p_vrfy); - } - - if (ret != 0) { - MBEDTLS_SSL_DEBUG_RET(1, "x509_verify_cert", ret); - } - - /* - * Secondary checks: always done, but change 'ret' only if it was 0 - */ - if (mbedtls_ssl_check_cert_usage(ssl->session_negotiate->peer_cert, - NULL, - ssl->conf->endpoint, - MBEDTLS_SSL_VERSION_TLS1_3, - &verify_result) != 0) { - MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate (usage extensions)")); - if (ret == 0) { - ret = MBEDTLS_ERR_SSL_BAD_CERTIFICATE; - } - } - - /* mbedtls_x509_crt_verify_with_profile is supposed to report a - * verification failure through MBEDTLS_ERR_X509_CERT_VERIFY_FAILED, - * with details encoded in the verification flags. All other kinds - * of error codes, including those from the user provided f_vrfy - * functions, are treated as fatal and lead to a failure of - * mbedtls_ssl_tls13_parse_certificate even if verification was optional. - */ - if (authmode == MBEDTLS_SSL_VERIFY_OPTIONAL && - (ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED || - ret == MBEDTLS_ERR_SSL_BAD_CERTIFICATE)) { - ret = 0; - } - - if (!have_ca_chain_or_callback && authmode == MBEDTLS_SSL_VERIFY_REQUIRED) { - MBEDTLS_SSL_DEBUG_MSG(1, ("got no CA chain")); - ret = MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED; - } - - if (ret != 0) { - /* The certificate may have been rejected for several reasons. - Pick one and send the corresponding alert. Which alert to send - may be a subject of debate in some cases. */ - if (verify_result & MBEDTLS_X509_BADCERT_OTHER) { - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_ACCESS_DENIED, ret); - } else if (verify_result & MBEDTLS_X509_BADCERT_CN_MISMATCH) { - MBEDTLS_SSL_PEND_FATAL_ALERT(MBEDTLS_SSL_ALERT_MSG_BAD_CERT, ret); - } else if (verify_result & (MBEDTLS_X509_BADCERT_KEY_USAGE | - MBEDTLS_X509_BADCERT_EXT_KEY_USAGE | - MBEDTLS_X509_BADCERT_BAD_PK | - MBEDTLS_X509_BADCERT_BAD_KEY)) { - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, ret); - } else if (verify_result & MBEDTLS_X509_BADCERT_EXPIRED) { - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_CERT_EXPIRED, ret); - } else if (verify_result & MBEDTLS_X509_BADCERT_REVOKED) { - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_CERT_REVOKED, ret); - } else if (verify_result & MBEDTLS_X509_BADCERT_NOT_TRUSTED) { - MBEDTLS_SSL_PEND_FATAL_ALERT(MBEDTLS_SSL_ALERT_MSG_UNKNOWN_CA, ret); - } else { - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_CERT_UNKNOWN, ret); - } - } - -#if defined(MBEDTLS_DEBUG_C) - if (verify_result != 0) { - MBEDTLS_SSL_DEBUG_MSG(3, ("! Certificate verification flags %08x", - (unsigned int) verify_result)); - } else { - MBEDTLS_SSL_DEBUG_MSG(3, ("Certificate verification flags clear")); - } -#endif /* MBEDTLS_DEBUG_C */ - - ssl->session_negotiate->verify_result = verify_result; - return ret; + return mbedtls_ssl_verify_certificate(ssl, authmode, + ssl->session_negotiate->peer_cert, + NULL, NULL); } #else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ MBEDTLS_CHECK_RETURN_CRITICAL From a040548747959d3f7f3cd4d6f77c82de90ffdfb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 16 Aug 2024 11:19:51 +0200 Subject: [PATCH 21/25] Improve some comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index fb48b95c1..0081e0447 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7944,14 +7944,13 @@ int mbedtls_ssl_verify_certificate(mbedtls_ssl_context *ssl, const mbedtls_ssl_ciphersuite_t *ciphersuite_info, void *rs_ctx) { - int ret = 0; - int have_ca_chain_or_callback = 0; - if (authmode == MBEDTLS_SSL_VERIFY_NONE) { return 0; } - /* Verify callback: precedence order is SSL context, else conf struct. */ + /* + * Primary check: use the appropriate X.509 verification function + */ int (*f_vrfy)(void *, mbedtls_x509_crt *, int, uint32_t *); void *p_vrfy; if (ssl->f_vrfy != NULL) { @@ -7964,9 +7963,8 @@ int mbedtls_ssl_verify_certificate(mbedtls_ssl_context *ssl, p_vrfy = ssl->conf->p_vrfy; } - /* - * Main check: verify certificate - */ + int ret = 0; + int have_ca_chain_or_callback = 0; #if defined(MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK) if (ssl->conf->f_ca_cb != NULL) { ((void) rs_ctx); @@ -8057,18 +8055,22 @@ int mbedtls_ssl_verify_certificate(mbedtls_ssl_context *ssl, } } - /* mbedtls_x509_crt_verify_with_profile is supposed to report a - * verification failure through MBEDTLS_ERR_X509_CERT_VERIFY_FAILED, - * with details encoded in the verification flags. All other kinds - * of error codes, including those from the user provided f_vrfy - * functions, are treated as fatal and lead to a failure of - * ssl_parse_certificate even if verification was optional. */ + /* With authmode optional, we want to keep going it the certificate was + * unacceptable, but still fail on other error (out of memory etc), + * including fatal errors from the f_vrfy callback. + * + * The only acceptable errors are: + * - MBEDTLS_ERR_X509_CERT_VERIFY_FAILED: cert rejected by primary check; + * - MBEDTLS_ERR_SSL_BAD_CERTIFICATE: cert rejected by secondary checks. + * Anything else is a fatal error. */ if (authmode == MBEDTLS_SSL_VERIFY_OPTIONAL && (ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED || ret == MBEDTLS_ERR_SSL_BAD_CERTIFICATE)) { ret = 0; } + /* Return a specific error as this is a user error: inconsistent + * configuration - can't verify without trust anchors. */ if (have_ca_chain_or_callback == 0 && authmode == MBEDTLS_SSL_VERIFY_REQUIRED) { MBEDTLS_SSL_DEBUG_MSG(1, ("got no CA chain")); ret = MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED; From 67072bf39ab95fbd0257a7159eecd00702f64055 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 16 Aug 2024 12:57:34 +0200 Subject: [PATCH 22/25] Fix two dependency declarations in ssl-opt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/ssl-opt.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 6b93fda4b..6afc26a11 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -2722,6 +2722,7 @@ run_test "Single supported algorithm sending: openssl client" \ 0 # Tests for certificate verification callback +requires_key_exchange_with_cert_in_tls12_or_tls13_enabled run_test "Configuration-specific CRT verification callback" \ "$P_SRV debug_level=3" \ "$P_CLI context_crt_cb=0 debug_level=3" \ @@ -2732,6 +2733,7 @@ run_test "Configuration-specific CRT verification callback" \ -C "Use context-specific verification callback" \ -C "error" +requires_key_exchange_with_cert_in_tls12_or_tls13_enabled run_test "Context-specific CRT verification callback" \ "$P_SRV debug_level=3" \ "$P_CLI context_crt_cb=1 debug_level=3" \ From 9e3e991d0451b76387cb7d5c3618550eee195a26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 20 Aug 2024 10:58:20 +0200 Subject: [PATCH 23/25] Fix typos in comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: David Horstmann Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_misc.h | 2 +- library/ssl_tls.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index b40f3355c..cbb18199d 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -1686,7 +1686,7 @@ static inline mbedtls_x509_crt *mbedtls_ssl_own_cert(mbedtls_ssl_context *ssl) * leave NULL for no restartable behaviour. * * Return: - * - 0 if the certificate is the handshake should continue. Depending on the + * - 0 if the handshake should continue. Depending on the * authmode it means: * - REQUIRED: the certificate was found to be valid, trusted & acceptable. * ssl->session_negotiate->verify_result is 0. diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 0081e0447..e4a034ce7 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6386,7 +6386,7 @@ int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, } else #endif { - /* This is either TLS 1.3 autentication, which always uses signatures, + /* This is either TLS 1.3 authentication, which always uses signatures, * or 1.2 client auth: rsa_sign and mbedtls_ecdsa_sign are the only * options we implement, both using signatures. */ (void) tls_version; @@ -8055,8 +8055,8 @@ int mbedtls_ssl_verify_certificate(mbedtls_ssl_context *ssl, } } - /* With authmode optional, we want to keep going it the certificate was - * unacceptable, but still fail on other error (out of memory etc), + /* With authmode optional, we want to keep going if the certificate was + * unacceptable, but still fail on other errors (out of memory etc), * including fatal errors from the f_vrfy callback. * * The only acceptable errors are: From 5398e58fcdaf5f6666d06376f9844f472d073f21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 20 Aug 2024 12:14:43 +0200 Subject: [PATCH 24/25] Fix guards around function now used by 1.3 as well MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Actually moved the function rather than trying to edit guards around it, because the relevant guards are not nearby, the function was part of larger blocks, so it seemed risky. Also, that seems logically correct: the function is no longer part of the "TLS 1.2 handshake functions common to server and client" section, it's part of the "helper functions common to 1.2 and 1.3 server and client" block. Ideally in the future perhaps the file structure should reflect that (`ssl_generic.c` vs `ssl_tls12_generic.c`?) but that's out of scope here. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 536 +++++++++++++++++++++++----------------------- 1 file changed, 270 insertions(+), 266 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e4a034ce7..0f0f2f65c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6335,91 +6335,6 @@ const char *mbedtls_ssl_get_curve_name_from_tls_id(uint16_t tls_id) } #endif -#if defined(MBEDTLS_X509_CRT_PARSE_C) -int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, - const mbedtls_ssl_ciphersuite_t *ciphersuite, - int recv_endpoint, - mbedtls_ssl_protocol_version tls_version, - uint32_t *flags) -{ - int ret = 0; - unsigned int usage = 0; - const char *ext_oid; - size_t ext_len; - - /* - * keyUsage - */ - - /* Note: don't guard this with MBEDTLS_SSL_CLI_C because the server wants - * to check what a compliant client will think while choosing which cert - * to send to the client. */ -#if defined(MBEDTLS_SSL_PROTO_TLS1_2) - if (tls_version == MBEDTLS_SSL_VERSION_TLS1_2 && - recv_endpoint == MBEDTLS_SSL_IS_CLIENT) { - /* TLS 1.2 server part of the key exchange */ - switch (ciphersuite->key_exchange) { - case MBEDTLS_KEY_EXCHANGE_RSA: - case MBEDTLS_KEY_EXCHANGE_RSA_PSK: - usage = MBEDTLS_X509_KU_KEY_ENCIPHERMENT; - break; - - case MBEDTLS_KEY_EXCHANGE_DHE_RSA: - case MBEDTLS_KEY_EXCHANGE_ECDHE_RSA: - case MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA: - usage = MBEDTLS_X509_KU_DIGITAL_SIGNATURE; - break; - - case MBEDTLS_KEY_EXCHANGE_ECDH_RSA: - case MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA: - usage = MBEDTLS_X509_KU_KEY_AGREEMENT; - break; - - /* Don't use default: we want warnings when adding new values */ - case MBEDTLS_KEY_EXCHANGE_NONE: - case MBEDTLS_KEY_EXCHANGE_PSK: - case MBEDTLS_KEY_EXCHANGE_DHE_PSK: - case MBEDTLS_KEY_EXCHANGE_ECDHE_PSK: - case MBEDTLS_KEY_EXCHANGE_ECJPAKE: - usage = 0; - } - } else -#endif - { - /* This is either TLS 1.3 authentication, which always uses signatures, - * or 1.2 client auth: rsa_sign and mbedtls_ecdsa_sign are the only - * options we implement, both using signatures. */ - (void) tls_version; - (void) ciphersuite; - usage = MBEDTLS_X509_KU_DIGITAL_SIGNATURE; - } - - if (mbedtls_x509_crt_check_key_usage(cert, usage) != 0) { - *flags |= MBEDTLS_X509_BADCERT_KEY_USAGE; - ret = -1; - } - - /* - * extKeyUsage - */ - - if (recv_endpoint == MBEDTLS_SSL_IS_CLIENT) { - ext_oid = MBEDTLS_OID_SERVER_AUTH; - ext_len = MBEDTLS_OID_SIZE(MBEDTLS_OID_SERVER_AUTH); - } else { - ext_oid = MBEDTLS_OID_CLIENT_AUTH; - ext_len = MBEDTLS_OID_SIZE(MBEDTLS_OID_CLIENT_AUTH); - } - - if (mbedtls_x509_crt_check_extended_key_usage(cert, ext_oid, ext_len) != 0) { - *flags |= MBEDTLS_X509_BADCERT_EXT_KEY_USAGE; - ret = -1; - } - - return ret; -} -#endif /* MBEDTLS_X509_CRT_PARSE_C */ - #if defined(MBEDTLS_USE_PSA_CRYPTO) int mbedtls_ssl_get_handshake_transcript(mbedtls_ssl_context *ssl, const mbedtls_md_type_t md, @@ -7938,187 +7853,6 @@ static int ssl_parse_certificate_coordinate(mbedtls_ssl_context *ssl, return SSL_CERTIFICATE_EXPECTED; } -int mbedtls_ssl_verify_certificate(mbedtls_ssl_context *ssl, - int authmode, - mbedtls_x509_crt *chain, - const mbedtls_ssl_ciphersuite_t *ciphersuite_info, - void *rs_ctx) -{ - if (authmode == MBEDTLS_SSL_VERIFY_NONE) { - return 0; - } - - /* - * Primary check: use the appropriate X.509 verification function - */ - int (*f_vrfy)(void *, mbedtls_x509_crt *, int, uint32_t *); - void *p_vrfy; - if (ssl->f_vrfy != NULL) { - MBEDTLS_SSL_DEBUG_MSG(3, ("Use context-specific verification callback")); - f_vrfy = ssl->f_vrfy; - p_vrfy = ssl->p_vrfy; - } else { - MBEDTLS_SSL_DEBUG_MSG(3, ("Use configuration-specific verification callback")); - f_vrfy = ssl->conf->f_vrfy; - p_vrfy = ssl->conf->p_vrfy; - } - - int ret = 0; - int have_ca_chain_or_callback = 0; -#if defined(MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK) - if (ssl->conf->f_ca_cb != NULL) { - ((void) rs_ctx); - have_ca_chain_or_callback = 1; - - MBEDTLS_SSL_DEBUG_MSG(3, ("use CA callback for X.509 CRT verification")); - ret = mbedtls_x509_crt_verify_with_ca_cb( - chain, - ssl->conf->f_ca_cb, - ssl->conf->p_ca_cb, - ssl->conf->cert_profile, - ssl->hostname, - &ssl->session_negotiate->verify_result, - f_vrfy, p_vrfy); - } else -#endif /* MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK */ - { - mbedtls_x509_crt *ca_chain; - mbedtls_x509_crl *ca_crl; -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if (ssl->handshake->sni_ca_chain != NULL) { - ca_chain = ssl->handshake->sni_ca_chain; - ca_crl = ssl->handshake->sni_ca_crl; - } else -#endif - { - ca_chain = ssl->conf->ca_chain; - ca_crl = ssl->conf->ca_crl; - } - - if (ca_chain != NULL) { - have_ca_chain_or_callback = 1; - } - - ret = mbedtls_x509_crt_verify_restartable( - chain, - ca_chain, ca_crl, - ssl->conf->cert_profile, - ssl->hostname, - &ssl->session_negotiate->verify_result, - f_vrfy, p_vrfy, rs_ctx); - } - - if (ret != 0) { - MBEDTLS_SSL_DEBUG_RET(1, "x509_verify_cert", ret); - } - -#if defined(MBEDTLS_SSL_ECP_RESTARTABLE_ENABLED) - if (ret == MBEDTLS_ERR_ECP_IN_PROGRESS) { - return MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS; - } -#endif - - /* - * Secondary checks: always done, but change 'ret' only if it was 0 - */ - - /* With TLS 1.2 and ECC certs, check that the curve used by the - * certificate is on our list of acceptable curves. - * - * With TLS 1.3 this is not needed because the curve is part of the - * signature algorithm (eg ecdsa_secp256r1_sha256) which is checked when - * we validate the signature made with the key associated to this cert. - */ -#if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \ - defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY) - if (ssl->tls_version == MBEDTLS_SSL_VERSION_TLS1_2 && - mbedtls_pk_can_do(&chain->pk, MBEDTLS_PK_ECKEY)) { - if (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_SSL_PROTO_TLS1_2 && PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY */ - - /* Check X.509 usage extensions (keyUsage, extKeyUsage) */ - if (mbedtls_ssl_check_cert_usage(chain, - ciphersuite_info, - ssl->conf->endpoint, - ssl->tls_version, - &ssl->session_negotiate->verify_result) != 0) { - MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate (usage extensions)")); - if (ret == 0) { - ret = MBEDTLS_ERR_SSL_BAD_CERTIFICATE; - } - } - - /* With authmode optional, we want to keep going if the certificate was - * unacceptable, but still fail on other errors (out of memory etc), - * including fatal errors from the f_vrfy callback. - * - * The only acceptable errors are: - * - MBEDTLS_ERR_X509_CERT_VERIFY_FAILED: cert rejected by primary check; - * - MBEDTLS_ERR_SSL_BAD_CERTIFICATE: cert rejected by secondary checks. - * Anything else is a fatal error. */ - if (authmode == MBEDTLS_SSL_VERIFY_OPTIONAL && - (ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED || - ret == MBEDTLS_ERR_SSL_BAD_CERTIFICATE)) { - ret = 0; - } - - /* Return a specific error as this is a user error: inconsistent - * configuration - can't verify without trust anchors. */ - if (have_ca_chain_or_callback == 0 && authmode == MBEDTLS_SSL_VERIFY_REQUIRED) { - MBEDTLS_SSL_DEBUG_MSG(1, ("got no CA chain")); - ret = MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED; - } - - if (ret != 0) { - uint8_t alert; - - /* The certificate may have been rejected for several reasons. - Pick one and send the corresponding alert. Which alert to send - may be a subject of debate in some cases. */ - if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_OTHER) { - alert = MBEDTLS_SSL_ALERT_MSG_ACCESS_DENIED; - } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_CN_MISMATCH) { - alert = MBEDTLS_SSL_ALERT_MSG_BAD_CERT; - } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_KEY_USAGE) { - alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; - } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_EXT_KEY_USAGE) { - alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; - } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_BAD_PK) { - alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; - } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_BAD_KEY) { - alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; - } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_EXPIRED) { - alert = MBEDTLS_SSL_ALERT_MSG_CERT_EXPIRED; - } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_REVOKED) { - alert = MBEDTLS_SSL_ALERT_MSG_CERT_REVOKED; - } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_NOT_TRUSTED) { - alert = MBEDTLS_SSL_ALERT_MSG_UNKNOWN_CA; - } else { - alert = MBEDTLS_SSL_ALERT_MSG_CERT_UNKNOWN; - } - mbedtls_ssl_send_alert_message(ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - alert); - } - -#if defined(MBEDTLS_DEBUG_C) - if (ssl->session_negotiate->verify_result != 0) { - MBEDTLS_SSL_DEBUG_MSG(3, ("! Certificate verification flags %08x", - (unsigned int) ssl->session_negotiate->verify_result)); - } else { - MBEDTLS_SSL_DEBUG_MSG(3, ("Certificate verification flags clear")); - } -#endif /* MBEDTLS_DEBUG_C */ - - return ret; -} - #if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) MBEDTLS_CHECK_RETURN_CRITICAL static int ssl_remember_peer_crt_digest(mbedtls_ssl_context *ssl, @@ -9923,4 +9657,274 @@ int mbedtls_ssl_session_set_ticket_alpn(mbedtls_ssl_session *session, return 0; } #endif /* MBEDTLS_SSL_SRV_C && MBEDTLS_SSL_EARLY_DATA && MBEDTLS_SSL_ALPN */ + +/* + * The following functions are used by 1.2 and 1.3, client and server. + */ +#if defined(MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED) +int mbedtls_ssl_check_cert_usage(const mbedtls_x509_crt *cert, + const mbedtls_ssl_ciphersuite_t *ciphersuite, + int recv_endpoint, + mbedtls_ssl_protocol_version tls_version, + uint32_t *flags) +{ + int ret = 0; + unsigned int usage = 0; + const char *ext_oid; + size_t ext_len; + + /* + * keyUsage + */ + + /* Note: don't guard this with MBEDTLS_SSL_CLI_C because the server wants + * to check what a compliant client will think while choosing which cert + * to send to the client. */ +#if defined(MBEDTLS_SSL_PROTO_TLS1_2) + if (tls_version == MBEDTLS_SSL_VERSION_TLS1_2 && + recv_endpoint == MBEDTLS_SSL_IS_CLIENT) { + /* TLS 1.2 server part of the key exchange */ + switch (ciphersuite->key_exchange) { + case MBEDTLS_KEY_EXCHANGE_RSA: + case MBEDTLS_KEY_EXCHANGE_RSA_PSK: + usage = MBEDTLS_X509_KU_KEY_ENCIPHERMENT; + break; + + case MBEDTLS_KEY_EXCHANGE_DHE_RSA: + case MBEDTLS_KEY_EXCHANGE_ECDHE_RSA: + case MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA: + usage = MBEDTLS_X509_KU_DIGITAL_SIGNATURE; + break; + + case MBEDTLS_KEY_EXCHANGE_ECDH_RSA: + case MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA: + usage = MBEDTLS_X509_KU_KEY_AGREEMENT; + break; + + /* Don't use default: we want warnings when adding new values */ + case MBEDTLS_KEY_EXCHANGE_NONE: + case MBEDTLS_KEY_EXCHANGE_PSK: + case MBEDTLS_KEY_EXCHANGE_DHE_PSK: + case MBEDTLS_KEY_EXCHANGE_ECDHE_PSK: + case MBEDTLS_KEY_EXCHANGE_ECJPAKE: + usage = 0; + } + } else +#endif + { + /* This is either TLS 1.3 authentication, which always uses signatures, + * or 1.2 client auth: rsa_sign and mbedtls_ecdsa_sign are the only + * options we implement, both using signatures. */ + (void) tls_version; + (void) ciphersuite; + usage = MBEDTLS_X509_KU_DIGITAL_SIGNATURE; + } + + if (mbedtls_x509_crt_check_key_usage(cert, usage) != 0) { + *flags |= MBEDTLS_X509_BADCERT_KEY_USAGE; + ret = -1; + } + + /* + * extKeyUsage + */ + + if (recv_endpoint == MBEDTLS_SSL_IS_CLIENT) { + ext_oid = MBEDTLS_OID_SERVER_AUTH; + ext_len = MBEDTLS_OID_SIZE(MBEDTLS_OID_SERVER_AUTH); + } else { + ext_oid = MBEDTLS_OID_CLIENT_AUTH; + ext_len = MBEDTLS_OID_SIZE(MBEDTLS_OID_CLIENT_AUTH); + } + + if (mbedtls_x509_crt_check_extended_key_usage(cert, ext_oid, ext_len) != 0) { + *flags |= MBEDTLS_X509_BADCERT_EXT_KEY_USAGE; + ret = -1; + } + + return ret; +} + +int mbedtls_ssl_verify_certificate(mbedtls_ssl_context *ssl, + int authmode, + mbedtls_x509_crt *chain, + const mbedtls_ssl_ciphersuite_t *ciphersuite_info, + void *rs_ctx) +{ + if (authmode == MBEDTLS_SSL_VERIFY_NONE) { + return 0; + } + + /* + * Primary check: use the appropriate X.509 verification function + */ + int (*f_vrfy)(void *, mbedtls_x509_crt *, int, uint32_t *); + void *p_vrfy; + if (ssl->f_vrfy != NULL) { + MBEDTLS_SSL_DEBUG_MSG(3, ("Use context-specific verification callback")); + f_vrfy = ssl->f_vrfy; + p_vrfy = ssl->p_vrfy; + } else { + MBEDTLS_SSL_DEBUG_MSG(3, ("Use configuration-specific verification callback")); + f_vrfy = ssl->conf->f_vrfy; + p_vrfy = ssl->conf->p_vrfy; + } + + int ret = 0; + int have_ca_chain_or_callback = 0; +#if defined(MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK) + if (ssl->conf->f_ca_cb != NULL) { + ((void) rs_ctx); + have_ca_chain_or_callback = 1; + + MBEDTLS_SSL_DEBUG_MSG(3, ("use CA callback for X.509 CRT verification")); + ret = mbedtls_x509_crt_verify_with_ca_cb( + chain, + ssl->conf->f_ca_cb, + ssl->conf->p_ca_cb, + ssl->conf->cert_profile, + ssl->hostname, + &ssl->session_negotiate->verify_result, + f_vrfy, p_vrfy); + } else +#endif /* MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK */ + { + mbedtls_x509_crt *ca_chain; + mbedtls_x509_crl *ca_crl; +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + if (ssl->handshake->sni_ca_chain != NULL) { + ca_chain = ssl->handshake->sni_ca_chain; + ca_crl = ssl->handshake->sni_ca_crl; + } else +#endif + { + ca_chain = ssl->conf->ca_chain; + ca_crl = ssl->conf->ca_crl; + } + + if (ca_chain != NULL) { + have_ca_chain_or_callback = 1; + } + + ret = mbedtls_x509_crt_verify_restartable( + chain, + ca_chain, ca_crl, + ssl->conf->cert_profile, + ssl->hostname, + &ssl->session_negotiate->verify_result, + f_vrfy, p_vrfy, rs_ctx); + } + + if (ret != 0) { + MBEDTLS_SSL_DEBUG_RET(1, "x509_verify_cert", ret); + } + +#if defined(MBEDTLS_SSL_ECP_RESTARTABLE_ENABLED) + if (ret == MBEDTLS_ERR_ECP_IN_PROGRESS) { + return MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS; + } +#endif + + /* + * Secondary checks: always done, but change 'ret' only if it was 0 + */ + + /* With TLS 1.2 and ECC certs, check that the curve used by the + * certificate is on our list of acceptable curves. + * + * With TLS 1.3 this is not needed because the curve is part of the + * signature algorithm (eg ecdsa_secp256r1_sha256) which is checked when + * we validate the signature made with the key associated to this cert. + */ +#if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \ + defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY) + if (ssl->tls_version == MBEDTLS_SSL_VERSION_TLS1_2 && + mbedtls_pk_can_do(&chain->pk, MBEDTLS_PK_ECKEY)) { + if (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_SSL_PROTO_TLS1_2 && PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY */ + + /* Check X.509 usage extensions (keyUsage, extKeyUsage) */ + if (mbedtls_ssl_check_cert_usage(chain, + ciphersuite_info, + ssl->conf->endpoint, + ssl->tls_version, + &ssl->session_negotiate->verify_result) != 0) { + MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate (usage extensions)")); + if (ret == 0) { + ret = MBEDTLS_ERR_SSL_BAD_CERTIFICATE; + } + } + + /* With authmode optional, we want to keep going if the certificate was + * unacceptable, but still fail on other errors (out of memory etc), + * including fatal errors from the f_vrfy callback. + * + * The only acceptable errors are: + * - MBEDTLS_ERR_X509_CERT_VERIFY_FAILED: cert rejected by primary check; + * - MBEDTLS_ERR_SSL_BAD_CERTIFICATE: cert rejected by secondary checks. + * Anything else is a fatal error. */ + if (authmode == MBEDTLS_SSL_VERIFY_OPTIONAL && + (ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED || + ret == MBEDTLS_ERR_SSL_BAD_CERTIFICATE)) { + ret = 0; + } + + /* Return a specific error as this is a user error: inconsistent + * configuration - can't verify without trust anchors. */ + if (have_ca_chain_or_callback == 0 && authmode == MBEDTLS_SSL_VERIFY_REQUIRED) { + MBEDTLS_SSL_DEBUG_MSG(1, ("got no CA chain")); + ret = MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED; + } + + if (ret != 0) { + uint8_t alert; + + /* The certificate may have been rejected for several reasons. + Pick one and send the corresponding alert. Which alert to send + may be a subject of debate in some cases. */ + if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_OTHER) { + alert = MBEDTLS_SSL_ALERT_MSG_ACCESS_DENIED; + } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_CN_MISMATCH) { + alert = MBEDTLS_SSL_ALERT_MSG_BAD_CERT; + } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_KEY_USAGE) { + alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; + } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_EXT_KEY_USAGE) { + alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; + } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_BAD_PK) { + alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; + } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_BAD_KEY) { + alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; + } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_EXPIRED) { + alert = MBEDTLS_SSL_ALERT_MSG_CERT_EXPIRED; + } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_REVOKED) { + alert = MBEDTLS_SSL_ALERT_MSG_CERT_REVOKED; + } else if (ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_NOT_TRUSTED) { + alert = MBEDTLS_SSL_ALERT_MSG_UNKNOWN_CA; + } else { + alert = MBEDTLS_SSL_ALERT_MSG_CERT_UNKNOWN; + } + mbedtls_ssl_send_alert_message(ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + alert); + } + +#if defined(MBEDTLS_DEBUG_C) + if (ssl->session_negotiate->verify_result != 0) { + MBEDTLS_SSL_DEBUG_MSG(3, ("! Certificate verification flags %08x", + (unsigned int) ssl->session_negotiate->verify_result)); + } else { + MBEDTLS_SSL_DEBUG_MSG(3, ("Certificate verification flags clear")); + } +#endif /* MBEDTLS_DEBUG_C */ + + return ret; +} +#endif /* MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED */ + #endif /* MBEDTLS_SSL_TLS_C */ From b721cccd8248612f467eff2ddfb78de8fe5b3e00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 20 Aug 2024 22:00:02 +0200 Subject: [PATCH 25/25] Add a ChangeLog entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/tls13-cert-regressions.txt | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 ChangeLog.d/tls13-cert-regressions.txt diff --git a/ChangeLog.d/tls13-cert-regressions.txt b/ChangeLog.d/tls13-cert-regressions.txt new file mode 100644 index 000000000..8dd8a327d --- /dev/null +++ b/ChangeLog.d/tls13-cert-regressions.txt @@ -0,0 +1,18 @@ +Bugfix + * Fixed a regression introduced in 3.6.0 where the CA callback set with + mbedtls_ssl_conf_ca_cb() would stop working when connections were + upgraded to TLS 1.3. Fixed by adding support for the CA callback with TLS + 1.3. + * Fixed a regression introduced in 3.6.0 where clients that relied on + optional/none authentication mode, by calling mbedtls_ssl_conf_authmode() + with MBEDTLS_SSL_VERIFY_OPTIONAL or MBEDTLS_SSL_VERIFY_NONE, would stop + working when connections were upgraded to TLS 1.3. Fixed by adding + support for optional/none with TLS 1.3 as well. Note that the TLS 1.3 + standard makes server authentication mandatory; users are advised not to + use authmode none, and to carefully check the results when using optional + mode. + * Fixed a regression introduced in 3.6.0 where context-specific certificate + verify callbacks, set with mbedtls_ssl_set_verify() as opposed to + mbedtls_ssl_conf_verify(), would stop working when connections were + upgraded to TLS 1.3. Fixed by adding support for context-specific verify + callback in TLS 1.3.