From 060e284deea9262d9ecb359ffc4a56ac294e01e7 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 91828ef03..7de41da99 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 a3cf1a53b457f67da4bff579b95b5107f17799d3 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 7de41da99..5eb965bb4 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 d6e20692dd3eb45d7cb2470eee5e1f1f74cdeb61 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 5eb965bb4..074230f37 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 4192bba54fd08f44942569791e01e975bdef5a82 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 074230f37..b9a09e9a5 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 96a0c5c48e1692fb1ca4f2c6d93731c9d6aee070 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 a8807f67c..40d7187c3 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 d6077a2ba..4376146b5 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 b5b975ff4..5bfab04ad 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 4938b693f31ec275a39e249ba67650d78f9546ef 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 40d7187c3..ecb2d0390 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 4376146b5..3f375be92 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 5bfab04ad..087874946 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 8a14aaaca5acc10eb662db3ad5391034463c3ac4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 9 Aug 2024 12:00:34 +0200 Subject: [PATCH 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 3f375be92..1355feb88 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8050,35 +8050,19 @@ static int ssl_parse_certificate_verify(mbedtls_ssl_context *ssl, * Secondary checks: always done, but change 'ret' only if it was 0 */ + /* Check curve for ECC certs */ #if defined(MBEDTLS_PK_HAVE_ECC_KEYS) - { - const mbedtls_pk_context *pk = &chain->pk; - - /* If certificate uses an EC key, make sure the curve is OK. - * This is a public key, so it can't be opaque, so can_do() is a good - * enough check to ensure pk_ec() is safe to use here. */ - if (mbedtls_pk_can_do(pk, MBEDTLS_PK_ECKEY)) { - /* and in the unlikely case the above assumption no longer holds - * we are making sure that pk_ec() here does not return a NULL - */ - mbedtls_ecp_group_id grp_id = mbedtls_pk_get_ec_group_id(pk); - if (grp_id == MBEDTLS_ECP_DP_NONE) { - MBEDTLS_SSL_DEBUG_MSG(1, ("invalid group ID")); - return MBEDTLS_ERR_SSL_INTERNAL_ERROR; - } - if (mbedtls_ssl_check_curve(ssl, grp_id) != 0) { - ssl->session_negotiate->verify_result |= - MBEDTLS_X509_BADCERT_BAD_KEY; - - MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate (EC key curve)")); - if (ret == 0) { - ret = MBEDTLS_ERR_SSL_BAD_CERTIFICATE; - } - } + if (mbedtls_pk_can_do(&chain->pk, MBEDTLS_PK_ECKEY) && + mbedtls_ssl_check_curve(ssl, mbedtls_pk_get_ec_group_id(&chain->pk)) != 0) { + MBEDTLS_SSL_DEBUG_MSG(1, ("bad certificate (EC key curve)")); + ssl->session_negotiate->verify_result |= MBEDTLS_X509_BADCERT_BAD_KEY; + if (ret == 0) { + ret = MBEDTLS_ERR_SSL_BAD_CERTIFICATE; } } #endif /* MBEDTLS_PK_HAVE_ECC_KEYS */ + /* Check X.509 usage extensions (keyUsage, extKeyUsage) */ if (mbedtls_ssl_check_cert_usage(chain, ciphersuite_info, ssl->conf->endpoint, From 85b864e1dbdfb66362077e06e43bf1f7317547f0 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 1355feb88..baf8631b9 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 4d4c0c72daeb71778d564f6b46325f72a9fff191 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 e1cc926717fe78f70535f2544512e13f728ccbda 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 baf8631b9..4795e673b 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 b9a09e9a5..e59c407b6 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 a0a781eaddcf8c9a42de4aa3448a241ed8daf696 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 e59c407b6..3f64ef683 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 2b98a4ee3b1922b2802f5f4b0781a7de703b5178 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 3f64ef683..84342d588 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 84442a3bffb6ce0784ce8ae6535111210e1869bd 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 84342d588..a6b59ae80 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 cb7f63266fd6169918f6df6c958a7a6ae142f49a 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 8d5da8f4a3afa1948f8fa26e3387f830a027fc24 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 a6b59ae80..33a9d4f6f 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 523a7e4aaf45ee13aaf4181b23c8f6b4b2a584c1 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 4795e673b..ebd19c366 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 e910ac862788c57fe4aff0e8fcccd27a1bd5369c 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 ebd19c366..c9cca703d 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 dee6ffa961b6bdee43b5c3e767e3cd27fc7d30dc 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 c9cca703d..a7c6cac6a 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 33a9d4f6f..2f89ece9e 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 d37054c82455ce673fe59a1a1b374b9ac1dd7986 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 a7c6cac6a..ad8f3f0e3 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 ce60330dfb4b6b9cc6d85b0b74020fa7d47bd385 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 ecb2d0390..e04f4ee44 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 ad8f3f0e3..ad410dc75 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(MBEDTLS_PK_HAVE_ECC_KEYS) - 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(MBEDTLS_PK_HAVE_ECC_KEYS) + 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_PK_HAVE_ECC_KEYS */ +#endif /* MBEDTLS_SSL_PROTO_TLS1_2 && MBEDTLS_PK_HAVE_ECC_KEYS */ /* 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 f2aa65fd57d21287cfdd142531b069caf096f83d 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 ad410dc75..b2fdcaaee 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 ff28e4c7f449ca5e1b30cb9755714f1e6bd1ee2a 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 2f89ece9e..c3f7c25fd 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 565da768a4e8c4769825a411a22501ef6c5a9829 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 e04f4ee44..762c47832 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 b2fdcaaee..2cff7a9fd 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 c32a4a2128fa4a3ac2a6ea3b6d7a197bcdc7d982 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 2cff7a9fd..8f5c37ad4 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(MBEDTLS_PK_HAVE_ECC_KEYS) - 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 && MBEDTLS_PK_HAVE_ECC_KEYS */ - - /* 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(MBEDTLS_PK_HAVE_ECC_KEYS) + 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 && MBEDTLS_PK_HAVE_ECC_KEYS */ + + /* 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 f4f3e92ac9f754e3bf0f606078e626a7ecc698a1 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.