From 0bbb39786dc41e3e503c7b80b4db1ca9f52f06bb Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Sun, 19 Sep 2021 20:27:17 +0800 Subject: [PATCH 01/12] tls13: add labels add client and server cv magic words Signed-off-by: Jerry Yu --- library/ssl_tls13_keys.h | 45 ++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/library/ssl_tls13_keys.h b/library/ssl_tls13_keys.h index 384f433b5..165b58a2d 100644 --- a/library/ssl_tls13_keys.h +++ b/library/ssl_tls13_keys.h @@ -22,25 +22,27 @@ /* This requires MBEDTLS_SSL_TLS1_3_LABEL( idx, name, string ) to be defined at * the point of use. See e.g. the definition of mbedtls_ssl_tls1_3_labels_union * below. */ -#define MBEDTLS_SSL_TLS1_3_LABEL_LIST \ - MBEDTLS_SSL_TLS1_3_LABEL( finished , "finished" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( resumption , "resumption" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( traffic_upd , "traffic upd" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( exporter , "exporter" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( key , "key" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( iv , "iv" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( c_hs_traffic, "c hs traffic" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( c_ap_traffic, "c ap traffic" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( c_e_traffic , "c e traffic" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( s_hs_traffic, "s hs traffic" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( s_ap_traffic, "s ap traffic" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( s_e_traffic , "s e traffic" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( e_exp_master, "e exp master" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( res_master , "res master" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( exp_master , "exp master" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( ext_binder , "ext binder" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( res_binder , "res binder" ) \ - MBEDTLS_SSL_TLS1_3_LABEL( derived , "derived" ) +#define MBEDTLS_SSL_TLS1_3_LABEL_LIST \ + MBEDTLS_SSL_TLS1_3_LABEL( finished , "finished" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( resumption , "resumption" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( traffic_upd , "traffic upd" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( exporter , "exporter" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( key , "key" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( iv , "iv" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( c_hs_traffic, "c hs traffic" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( c_ap_traffic, "c ap traffic" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( c_e_traffic , "c e traffic" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( s_hs_traffic, "s hs traffic" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( s_ap_traffic, "s ap traffic" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( s_e_traffic , "s e traffic" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( e_exp_master, "e exp master" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( res_master , "res master" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( exp_master , "exp master" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( ext_binder , "ext binder" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( res_binder , "res binder" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( derived , "derived" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( client_cv , "TLS 1.3, client CertificateVerify" ) \ + MBEDTLS_SSL_TLS1_3_LABEL( server_cv , "TLS 1.3, server CertificateVerify" ) #define MBEDTLS_SSL_TLS1_3_LABEL( name, string ) \ const unsigned char name [ sizeof(string) - 1 ]; @@ -57,9 +59,12 @@ struct mbedtls_ssl_tls1_3_labels_struct extern const struct mbedtls_ssl_tls1_3_labels_struct mbedtls_ssl_tls1_3_labels; +#define MBEDTLS_SSL_TLS1_3_LBL_LEN( LABEL ) \ + sizeof(mbedtls_ssl_tls1_3_labels.LABEL) + #define MBEDTLS_SSL_TLS1_3_LBL_WITH_LEN( LABEL ) \ mbedtls_ssl_tls1_3_labels.LABEL, \ - sizeof(mbedtls_ssl_tls1_3_labels.LABEL) + MBEDTLS_SSL_TLS1_3_LBL_LEN( LABEL ) #define MBEDTLS_SSL_TLS1_3_KEY_SCHEDULE_MAX_LABEL_LEN \ sizeof( union mbedtls_ssl_tls1_3_labels_union ) From 30b071cb66766efa5b367772a14f375248b64f54 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Sun, 12 Sep 2021 20:16:03 +0800 Subject: [PATCH 02/12] tls13:Add certificate verify Signed-off-by: Jerry Yu --- library/ssl_misc.h | 5 + library/ssl_tls13_client.c | 7 +- library/ssl_tls13_generic.c | 375 +++++++++++++++++++++++++++++++++++- 3 files changed, 382 insertions(+), 5 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index dbef6aa21..56633464b 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -1633,6 +1633,11 @@ int mbedtls_ssl_tls13_start_handshake_msg( mbedtls_ssl_context *ssl, */ int mbedtls_ssl_tls13_process_certificate( mbedtls_ssl_context *ssl ); +/* + * Generic handler of Certificate Verify + */ +int mbedtls_ssl_tls13_process_certificate_verify( mbedtls_ssl_context *ssl ); + /* * Write TLS 1.3 handshake message tail */ diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 0fb09c4ce..6e16c07ba 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1584,7 +1584,12 @@ static int ssl_tls1_3_process_server_certificate( mbedtls_ssl_context *ssl ) */ static int ssl_tls1_3_process_certificate_verify( mbedtls_ssl_context *ssl ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "%s hasn't been implemented", __func__ ) ); + int ret; + + ret = mbedtls_ssl_tls13_process_certificate_verify( ssl ); + if( ret != 0 ) + return( ret ); + mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_SERVER_FINISHED ); return( 0 ); } diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index c8601ce17..e484b7966 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -23,14 +23,15 @@ #if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL) +#include + #include "mbedtls/error.h" #include "mbedtls/debug.h" +#include "mbedtls/oid.h" +#include "mbedtls/platform.h" #include "ssl_misc.h" -#include -#include -#include - +#include "ssl_tls13_keys.h" int mbedtls_ssl_tls1_3_fetch_handshake_msg( mbedtls_ssl_context *ssl, unsigned hs_type, @@ -217,8 +218,374 @@ int mbedtls_ssl_tls13_write_sig_alg_ext( mbedtls_ssl_context *ssl, return( 0 ); } +/* + * The ssl_tls13_create_verify_structure() creates the verify structure. + * As input, it requires the transcript hash. + * + * The caller has to ensure that the buffer has size at least + * SSL_VERIFY_STRUCT_MAX_SIZE bytes. + */ +static void ssl_tls13_create_verify_structure( unsigned char *transcript_hash, + size_t transcript_hash_len, + unsigned char *verify_buffer, + size_t *verify_buffer_len, + int from ) +{ + size_t idx = 0; + + /* RFC 8446, Section 4.4.3: + * + * The digital signature [in the CertificateVerify message] is then + * computed over the concatenation of: + * - A string that consists of octet 32 (0x20) repeated 64 times + * - The context string + * - A single 0 byte which serves as the separator + * - The content to be signed + */ + uint8_t const verify_padding_val = 0x20; + size_t const verify_padding_len = 64; + + memset( verify_buffer + idx, verify_padding_val, verify_padding_len ); + idx += verify_padding_len; + + if( from == MBEDTLS_SSL_IS_CLIENT ) + { + memcpy( verify_buffer + idx, MBEDTLS_SSL_TLS1_3_LBL_WITH_LEN( client_cv ) ); + idx += MBEDTLS_SSL_TLS1_3_LBL_LEN( client_cv ); + } + else + { /* from == MBEDTLS_SSL_IS_SERVER */ + memcpy( verify_buffer + idx, MBEDTLS_SSL_TLS1_3_LBL_WITH_LEN( server_cv ) ); + idx += MBEDTLS_SSL_TLS1_3_LBL_LEN( server_cv ); + } + + verify_buffer[idx++] = 0x0; + + memcpy( verify_buffer + idx, transcript_hash, transcript_hash_len ); + idx += transcript_hash_len; + + *verify_buffer_len = idx; +} + #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ +/* + * STATE HANDLING: Read CertificateVerify + */ +/* Macro to express the length of the verify structure length. + * + * The structure is computed per TLS 1.3 specification as: + * - 64 bytes of octet 32, + * - 33 bytes for the context string + * (which is either "TLS 1.3, client CertificateVerify" + * or "TLS 1.3, server CertificateVerify"), + * - 1 byte for the octet 0x0, which servers as a separator, + * - 32 or 48 bytes for the Transcript-Hash(Handshake Context, Certificate) + * (depending on the size of the transcript_hash) + * + * This results in a total size of + * - 130 bytes for a SHA256-based transcript hash, or + * (64 + 33 + 1 + 32 bytes) + * - 146 bytes for a SHA384-based transcript hash. + * (64 + 33 + 1 + 48 bytes) + * + */ +#define SSL_VERIFY_STRUCT_MAX_SIZE ( 64 + \ + 33 + \ + 1 + \ + MBEDTLS_MD_MAX_SIZE \ + ) +/* Coordinate: Check whether a certificate verify message is expected. + * Returns a negative value on failure, and otherwise + * - SSL_CERTIFICATE_VERIFY_SKIP + * - SSL_CERTIFICATE_VERIFY_READ + * to indicate if the CertificateVerify message should be present or not. + */ +#define SSL_CERTIFICATE_VERIFY_SKIP 0 +#define SSL_CERTIFICATE_VERIFY_READ 1 +static int ssl_tls13_process_certificate_verify_coordinate( + mbedtls_ssl_context *ssl ) +{ + if( mbedtls_ssl_tls1_3_some_psk_enabled( ssl ) ) + return( SSL_CERTIFICATE_VERIFY_SKIP ); + +#if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) + if( ssl->session_negotiate->peer_cert == NULL ) + return( SSL_CERTIFICATE_VERIFY_SKIP ); + return( SSL_CERTIFICATE_VERIFY_READ ); +#else + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); +#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ +} + +#if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) +static int ssl_tls13_process_certificate_verify_parse( mbedtls_ssl_context *ssl, + const unsigned char *buf, + const unsigned char *end, + const unsigned char *verify_buffer, + size_t verify_buffer_len ) +{ + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + const unsigned char *p = buf; + uint16_t algorithm; + const uint16_t *tls13_sig_alg = ssl->conf->tls13_sig_algs; + size_t signature_len; + mbedtls_pk_type_t sig_alg; + mbedtls_md_type_t md_alg; + unsigned char verify_hash[MBEDTLS_TLS1_3_MD_MAX_SIZE]; + size_t verify_hash_len; + + /* + * struct { + * SignatureScheme algorithm; + * opaque signature<0..2^16-1>; + * } CertificateVerify; + */ + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); + algorithm = MBEDTLS_GET_UINT16_BE( p, 0 ); + p += 2; + + /* RFC 8446 section 4.4.3 + * + * If the CertificateVerify message is sent by a server, the signature algorithm + * MUST be one offered in the client's "signature_algorithms" extension unless + * no valid certificate chain can be produced without unsupported algorithms + * + * RFC 8446 section 4.4.2.2 + * + * If the client cannot construct an acceptable chain using the provided + * certificates and decides to abort the handshake, then it MUST abort the handshake + * with an appropriate certificate-related alert (by default, "unsupported_certificate"). + * + * Check if algorithm in offered signature algorithms. Send `unsupported_certificate` + * alert message on failure. + */ + while( 1 ) + { + /* Found algorithm in offered signature algorithms */ + if( *tls13_sig_alg == algorithm ) + break; + + if( *tls13_sig_alg == MBEDTLS_TLS13_SIG_NONE ) + { + /* End of offered signature algorithms list */ + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "signature algorithm(%04x) not in offered" + "signature algorithms ", + ( unsigned int ) algorithm ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; + } + + tls13_sig_alg++; + } + + /* We currently only support ECDSA-based signatures */ + switch( algorithm ) + { + case MBEDTLS_TLS13_SIG_ECDSA_SECP256R1_SHA256: + md_alg = MBEDTLS_MD_SHA256; + sig_alg = MBEDTLS_PK_ECDSA; + break; + case MBEDTLS_TLS13_SIG_ECDSA_SECP384R1_SHA384: + md_alg = MBEDTLS_MD_SHA384; + sig_alg = MBEDTLS_PK_ECDSA; + break; + case MBEDTLS_TLS13_SIG_ECDSA_SECP521R1_SHA512: + md_alg = MBEDTLS_MD_SHA512; + sig_alg = MBEDTLS_PK_ECDSA; + break; + default: + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Certificate Verify: Unknown signature algorithm." ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + } + + MBEDTLS_SSL_DEBUG_MSG( 3, ( "Certificate Verify: Signature algorithm ( %04x )", + ( unsigned int ) algorithm ) ); + + /* + * Check the certificate's key type matches the signature alg + */ + if( !mbedtls_pk_can_do( &ssl->session_negotiate->peer_cert->pk, sig_alg ) ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "signature algorithm doesn't match cert key" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + } + + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); + signature_len = MBEDTLS_GET_UINT16_BE( p, 0 ); + p += 2; + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, signature_len ); + + /* Hash verify buffer with indicated hash function */ + switch( md_alg ) + { +#if defined(MBEDTLS_SHA256_C) + case MBEDTLS_MD_SHA256: + verify_hash_len = 32; + if( ( ret = mbedtls_sha256( verify_buffer, + verify_buffer_len, + verify_hash, + 0 ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_sha256", ret ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + } + break; +#endif /* MBEDTLS_SHA256_C */ + +#if defined(MBEDTLS_SHA384_C) + case MBEDTLS_MD_SHA384: + verify_hash_len = 48; + if( ( ret = mbedtls_sha512( verify_buffer, + verify_buffer_len, + verify_hash, + 1 ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_sha384", ret ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + } + break; +#endif /* MBEDTLS_SHA384_C */ + +#if defined(MBEDTLS_SHA512_C) + case MBEDTLS_MD_SHA512: + verify_hash_len = 64; + if( ( ret = mbedtls_sha512( verify_buffer, + verify_buffer_len, + verify_hash, + 0 ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_sha512", ret ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + } + break; +#endif /* MBEDTLS_SHA512_C */ + + default: + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Certificate Verify: Unknown signature algorithm." ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + } + + MBEDTLS_SSL_DEBUG_BUF( 3, "verify hash", verify_hash, verify_hash_len ); + + if( ( ret = mbedtls_pk_verify_ext( sig_alg, NULL, + &ssl->session_negotiate->peer_cert->pk, + md_alg, verify_hash, verify_hash_len, + buf, signature_len ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_pk_verify_ext", ret ); + + /* RFC 8446 section 4.4.3 + * + * If the verification fails, the receiver MUST terminate the handshake + * with a "decrypt_error" alert. + */ + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECRYPT_ERROR, ret ); + + return( ret ); + } + + return( ret ); +} +#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ + +int mbedtls_ssl_tls13_process_certificate_verify( mbedtls_ssl_context *ssl ) +{ + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + + /* Coordination step */ + MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) ); + + MBEDTLS_SSL_PROC_CHK_NEG( ssl_tls13_process_certificate_verify_coordinate( ssl ) ); + +#if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) // TBD: double-check + if( ret == SSL_CERTIFICATE_VERIFY_READ ) + { + unsigned char verify_buffer[SSL_VERIFY_STRUCT_MAX_SIZE]; + size_t verify_buffer_len; + unsigned char transcript[MBEDTLS_TLS1_3_MD_MAX_SIZE]; + size_t transcript_len; + unsigned char *buf; + size_t buf_len; + + /* Need to calculate the hash of the transcript first + * before reading the message since otherwise it gets + * included in the transcript + */ + ret = mbedtls_ssl_get_handshake_transcript( ssl, + ssl->handshake->ciphersuite_info->mac, + transcript, sizeof( transcript ), + &transcript_len ); + if( ret != 0 ) + { + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR, + MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + return( ret ); + } + + MBEDTLS_SSL_DEBUG_BUF( 3, "handshake hash", transcript, transcript_len ); + + /* Create verify structure */ + ssl_tls13_create_verify_structure( transcript, + transcript_len, + verify_buffer, + &verify_buffer_len, + !ssl->conf->endpoint ); + + MBEDTLS_SSL_PROC_CHK( + mbedtls_ssl_tls1_3_fetch_handshake_msg( ssl, + MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, &buf, &buf_len ) ); + + /* Process the message contents */ + MBEDTLS_SSL_PROC_CHK( + ssl_tls13_process_certificate_verify_parse( ssl, + buf, buf + buf_len, verify_buffer, verify_buffer_len ) ); + + mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( ssl, + MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, buf, buf_len ); + } + else +#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ + if( ret == SSL_CERTIFICATE_VERIFY_SKIP ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); + } + else + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + + +cleanup: + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate verify" ) ); + return( ret ); +} + /* * * STATE HANDLING: Incoming Certificate, client-side only currently. From 26c2d118027ec9a930ffb83da860c2b129fa32ea Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Mon, 25 Oct 2021 12:42:58 +0800 Subject: [PATCH 03/12] Fix various issues Signed-off-by: Jerry Yu --- library/ssl_tls13_generic.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index e484b7966..db99d9de4 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -231,7 +231,7 @@ static void ssl_tls13_create_verify_structure( unsigned char *transcript_hash, size_t *verify_buffer_len, int from ) { - size_t idx = 0; + size_t idx; /* RFC 8446, Section 4.4.3: * @@ -245,8 +245,8 @@ static void ssl_tls13_create_verify_structure( unsigned char *transcript_hash, uint8_t const verify_padding_val = 0x20; size_t const verify_padding_len = 64; - memset( verify_buffer + idx, verify_padding_val, verify_padding_len ); - idx += verify_padding_len; + memset( verify_buffer, verify_padding_val, verify_padding_len ); + idx = verify_padding_len; if( from == MBEDTLS_SSL_IS_CLIENT ) { @@ -290,10 +290,10 @@ static void ssl_tls13_create_verify_structure( unsigned char *transcript_hash, * (64 + 33 + 1 + 48 bytes) * */ -#define SSL_VERIFY_STRUCT_MAX_SIZE ( 64 + \ - 33 + \ - 1 + \ - MBEDTLS_MD_MAX_SIZE \ +#define SSL_VERIFY_STRUCT_MAX_SIZE ( 64 + \ + 33 + \ + 1 + \ + MBEDTLS_TLS1_3_MD_MAX_SIZE \ ) /* Coordinate: Check whether a certificate verify message is expected. * Returns a negative value on failure, and otherwise @@ -530,6 +530,10 @@ int mbedtls_ssl_tls13_process_certificate_verify( mbedtls_ssl_context *ssl ) unsigned char *buf; size_t buf_len; + MBEDTLS_SSL_PROC_CHK( + mbedtls_ssl_tls1_3_fetch_handshake_msg( ssl, + MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, &buf, &buf_len ) ); + /* Need to calculate the hash of the transcript first * before reading the message since otherwise it gets * included in the transcript @@ -555,10 +559,6 @@ int mbedtls_ssl_tls13_process_certificate_verify( mbedtls_ssl_context *ssl ) &verify_buffer_len, !ssl->conf->endpoint ); - MBEDTLS_SSL_PROC_CHK( - mbedtls_ssl_tls1_3_fetch_handshake_msg( ssl, - MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, &buf, &buf_len ) ); - /* Process the message contents */ MBEDTLS_SSL_PROC_CHK( ssl_tls13_process_certificate_verify_parse( ssl, From 133690cceff604a5836e271fb0a5e8055a1884d7 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Mon, 25 Oct 2021 14:01:13 +0800 Subject: [PATCH 04/12] Refactor hash computation Signed-off-by: Jerry Yu --- library/ssl_tls13_generic.c | 45 ++++++++++--------------------------- 1 file changed, 12 insertions(+), 33 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index db99d9de4..24275e5c5 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -432,51 +432,21 @@ static int ssl_tls13_process_certificate_verify_parse( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_SHA256_C) case MBEDTLS_MD_SHA256: verify_hash_len = 32; - if( ( ret = mbedtls_sha256( verify_buffer, - verify_buffer_len, - verify_hash, - 0 ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_sha256", ret ); - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - } + ret = mbedtls_sha256( verify_buffer, verify_buffer_len, verify_hash, 0 ); break; #endif /* MBEDTLS_SHA256_C */ #if defined(MBEDTLS_SHA384_C) case MBEDTLS_MD_SHA384: verify_hash_len = 48; - if( ( ret = mbedtls_sha512( verify_buffer, - verify_buffer_len, - verify_hash, - 1 ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_sha384", ret ); - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - } + ret = mbedtls_sha512( verify_buffer, verify_buffer_len, verify_hash, 1 ); break; #endif /* MBEDTLS_SHA384_C */ #if defined(MBEDTLS_SHA512_C) case MBEDTLS_MD_SHA512: verify_hash_len = 64; - if( ( ret = mbedtls_sha512( verify_buffer, - verify_buffer_len, - verify_hash, - 0 ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_sha512", ret ); - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - } + ret = mbedtls_sha512( verify_buffer, verify_buffer_len, verify_hash, 0 ); break; #endif /* MBEDTLS_SHA512_C */ @@ -488,6 +458,15 @@ static int ssl_tls13_process_certificate_verify_parse( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); } + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "hash computation error", ret ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + } + MBEDTLS_SSL_DEBUG_BUF( 3, "verify hash", verify_hash, verify_hash_len ); if( ( ret = mbedtls_pk_verify_ext( sig_alg, NULL, From 982d9e5db2e5dbd414dbe400d3bfdb1721a47525 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Thu, 14 Oct 2021 15:59:37 +0800 Subject: [PATCH 05/12] Add ssl_tls13_sig_alg_is_offered To keep consistent with cipher_suite check Signed-off-by: Jerry Yu --- library/ssl_tls13_generic.c | 41 +++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 24275e5c5..66c6678ec 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -320,6 +320,18 @@ static int ssl_tls13_process_certificate_verify_coordinate( } #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) +static int ssl_tls13_sig_alg_is_offered( mbedtls_ssl_context *ssl, uint16_t sig_alg ) +{ + const uint16_t *tls13_sig_alg = ssl->conf->tls13_sig_algs; + + for( ; *tls13_sig_alg !=MBEDTLS_TLS13_SIG_NONE ; tls13_sig_alg++ ) + { + if( *tls13_sig_alg == sig_alg ) + return 1; + } + return 0; +} + static int ssl_tls13_process_certificate_verify_parse( mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end, @@ -329,7 +341,6 @@ static int ssl_tls13_process_certificate_verify_parse( mbedtls_ssl_context *ssl, int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; const unsigned char *p = buf; uint16_t algorithm; - const uint16_t *tls13_sig_alg = ssl->conf->tls13_sig_algs; size_t signature_len; mbedtls_pk_type_t sig_alg; mbedtls_md_type_t md_alg; @@ -361,26 +372,16 @@ static int ssl_tls13_process_certificate_verify_parse( mbedtls_ssl_context *ssl, * Check if algorithm in offered signature algorithms. Send `unsupported_certificate` * alert message on failure. */ - while( 1 ) + if( ssl_tls13_sig_alg_is_offered( ssl, algorithm ) == 0 ) { - /* Found algorithm in offered signature algorithms */ - if( *tls13_sig_alg == algorithm ) - break; - - if( *tls13_sig_alg == MBEDTLS_TLS13_SIG_NONE ) - { - /* End of offered signature algorithms list */ - MBEDTLS_SSL_DEBUG_MSG( 1, - ( "signature algorithm(%04x) not in offered" - "signature algorithms ", - ( unsigned int ) algorithm ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; - } - - tls13_sig_alg++; + /* algorithm not in offered signature algorithms list */ + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Received signature algorithm(%04x) is not " + "offered.", + ( unsigned int ) algorithm ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; } /* We currently only support ECDSA-based signatures */ From da8cdf2fa969573039115003bfce08fb5b170c7d Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Mon, 25 Oct 2021 15:06:49 +0800 Subject: [PATCH 06/12] Remove certificate_verify_coordinate Signed-off-by: Jerry Yu --- library/ssl_tls13_generic.c | 134 ++++++++++++++---------------------- 1 file changed, 53 insertions(+), 81 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 66c6678ec..2a8695a47 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -295,29 +295,7 @@ static void ssl_tls13_create_verify_structure( unsigned char *transcript_hash, 1 + \ MBEDTLS_TLS1_3_MD_MAX_SIZE \ ) -/* Coordinate: Check whether a certificate verify message is expected. - * Returns a negative value on failure, and otherwise - * - SSL_CERTIFICATE_VERIFY_SKIP - * - SSL_CERTIFICATE_VERIFY_READ - * to indicate if the CertificateVerify message should be present or not. - */ -#define SSL_CERTIFICATE_VERIFY_SKIP 0 -#define SSL_CERTIFICATE_VERIFY_READ 1 -static int ssl_tls13_process_certificate_verify_coordinate( - mbedtls_ssl_context *ssl ) -{ - if( mbedtls_ssl_tls1_3_some_psk_enabled( ssl ) ) - return( SSL_CERTIFICATE_VERIFY_SKIP ); -#if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) - if( ssl->session_negotiate->peer_cert == NULL ) - return( SSL_CERTIFICATE_VERIFY_SKIP ); - return( SSL_CERTIFICATE_VERIFY_READ ); -#else - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); -#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ -} #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) static int ssl_tls13_sig_alg_is_offered( mbedtls_ssl_context *ssl, uint16_t sig_alg ) @@ -493,77 +471,71 @@ static int ssl_tls13_process_certificate_verify_parse( mbedtls_ssl_context *ssl, int mbedtls_ssl_tls13_process_certificate_verify( mbedtls_ssl_context *ssl ) { - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - /* Coordination step */ +#if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + unsigned char verify_buffer[SSL_VERIFY_STRUCT_MAX_SIZE]; + size_t verify_buffer_len; + unsigned char transcript[MBEDTLS_TLS1_3_MD_MAX_SIZE]; + size_t transcript_len; + unsigned char *buf; + size_t buf_len; + + if( mbedtls_ssl_tls1_3_some_psk_enabled( ssl ) ) + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + + if( ssl->session_negotiate->peer_cert == NULL ) + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) ); - MBEDTLS_SSL_PROC_CHK_NEG( ssl_tls13_process_certificate_verify_coordinate( ssl ) ); + MBEDTLS_SSL_PROC_CHK( + mbedtls_ssl_tls1_3_fetch_handshake_msg( ssl, + MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, &buf, &buf_len ) ); -#if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) // TBD: double-check - if( ret == SSL_CERTIFICATE_VERIFY_READ ) + /* Need to calculate the hash of the transcript first + * before reading the message since otherwise it gets + * included in the transcript + */ + ret = mbedtls_ssl_get_handshake_transcript( ssl, + ssl->handshake->ciphersuite_info->mac, + transcript, sizeof( transcript ), + &transcript_len ); + if( ret != 0 ) { - unsigned char verify_buffer[SSL_VERIFY_STRUCT_MAX_SIZE]; - size_t verify_buffer_len; - unsigned char transcript[MBEDTLS_TLS1_3_MD_MAX_SIZE]; - size_t transcript_len; - unsigned char *buf; - size_t buf_len; - - MBEDTLS_SSL_PROC_CHK( - mbedtls_ssl_tls1_3_fetch_handshake_msg( ssl, - MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, &buf, &buf_len ) ); - - /* Need to calculate the hash of the transcript first - * before reading the message since otherwise it gets - * included in the transcript - */ - ret = mbedtls_ssl_get_handshake_transcript( ssl, - ssl->handshake->ciphersuite_info->mac, - transcript, sizeof( transcript ), - &transcript_len ); - if( ret != 0 ) - { - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR, - MBEDTLS_ERR_SSL_INTERNAL_ERROR ); - return( ret ); - } - - MBEDTLS_SSL_DEBUG_BUF( 3, "handshake hash", transcript, transcript_len ); - - /* Create verify structure */ - ssl_tls13_create_verify_structure( transcript, - transcript_len, - verify_buffer, - &verify_buffer_len, - !ssl->conf->endpoint ); - - /* Process the message contents */ - MBEDTLS_SSL_PROC_CHK( - ssl_tls13_process_certificate_verify_parse( ssl, - buf, buf + buf_len, verify_buffer, verify_buffer_len ) ); - - mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( ssl, - MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, buf, buf_len ); - } - else -#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ - if( ret == SSL_CERTIFICATE_VERIFY_SKIP ) - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); - } - else - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR, + MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + return( ret ); } + MBEDTLS_SSL_DEBUG_BUF( 3, "handshake hash", transcript, transcript_len ); + + /* Create verify structure */ + ssl_tls13_create_verify_structure( transcript, + transcript_len, + verify_buffer, + &verify_buffer_len, + !ssl->conf->endpoint ); + + /* Process the message contents */ + MBEDTLS_SSL_PROC_CHK( + ssl_tls13_process_certificate_verify_parse( ssl, + buf, buf + buf_len, verify_buffer, verify_buffer_len ) ); + + mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( ssl, + MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, buf, buf_len ); cleanup: MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate verify" ) ); return( ret ); +#else + ((void) ssl); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); +#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ } /* From 0b32c502a407a107800786043ea2ad35227e3f16 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Thu, 28 Oct 2021 13:41:59 +0800 Subject: [PATCH 07/12] fix various issues Signed-off-by: Jerry Yu --- library/ssl_tls13_generic.c | 138 ++++++++++++++++-------------------- 1 file changed, 61 insertions(+), 77 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 2a8695a47..d42e463a9 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -218,57 +218,6 @@ int mbedtls_ssl_tls13_write_sig_alg_ext( mbedtls_ssl_context *ssl, return( 0 ); } -/* - * The ssl_tls13_create_verify_structure() creates the verify structure. - * As input, it requires the transcript hash. - * - * The caller has to ensure that the buffer has size at least - * SSL_VERIFY_STRUCT_MAX_SIZE bytes. - */ -static void ssl_tls13_create_verify_structure( unsigned char *transcript_hash, - size_t transcript_hash_len, - unsigned char *verify_buffer, - size_t *verify_buffer_len, - int from ) -{ - size_t idx; - - /* RFC 8446, Section 4.4.3: - * - * The digital signature [in the CertificateVerify message] is then - * computed over the concatenation of: - * - A string that consists of octet 32 (0x20) repeated 64 times - * - The context string - * - A single 0 byte which serves as the separator - * - The content to be signed - */ - uint8_t const verify_padding_val = 0x20; - size_t const verify_padding_len = 64; - - memset( verify_buffer, verify_padding_val, verify_padding_len ); - idx = verify_padding_len; - - if( from == MBEDTLS_SSL_IS_CLIENT ) - { - memcpy( verify_buffer + idx, MBEDTLS_SSL_TLS1_3_LBL_WITH_LEN( client_cv ) ); - idx += MBEDTLS_SSL_TLS1_3_LBL_LEN( client_cv ); - } - else - { /* from == MBEDTLS_SSL_IS_SERVER */ - memcpy( verify_buffer + idx, MBEDTLS_SSL_TLS1_3_LBL_WITH_LEN( server_cv ) ); - idx += MBEDTLS_SSL_TLS1_3_LBL_LEN( server_cv ); - } - - verify_buffer[idx++] = 0x0; - - memcpy( verify_buffer + idx, transcript_hash, transcript_hash_len ); - idx += transcript_hash_len; - - *verify_buffer_len = idx; -} - -#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ - /* * STATE HANDLING: Read CertificateVerify */ @@ -296,8 +245,52 @@ static void ssl_tls13_create_verify_structure( unsigned char *transcript_hash, MBEDTLS_TLS1_3_MD_MAX_SIZE \ ) +/* + * The ssl_tls13_create_verify_structure() creates the verify structure. + * As input, it requires the transcript hash. + * + * The caller has to ensure that the buffer has size at least + * SSL_VERIFY_STRUCT_MAX_SIZE bytes. + */ +static void ssl_tls13_create_verify_structure( unsigned char *transcript_hash, + size_t transcript_hash_len, + unsigned char *verify_buffer, + size_t *verify_buffer_len, + int from ) +{ + size_t idx; + + /* RFC 8446, Section 4.4.3: + * + * The digital signature [in the CertificateVerify message] is then + * computed over the concatenation of: + * - A string that consists of octet 32 (0x20) repeated 64 times + * - The context string + * - A single 0 byte which serves as the separator + * - The content to be signed + */ + memset( verify_buffer, 0x20, 64 ); + idx = 64; + + if( from == MBEDTLS_SSL_IS_CLIENT ) + { + memcpy( verify_buffer + idx, MBEDTLS_SSL_TLS1_3_LBL_WITH_LEN( client_cv ) ); + idx += MBEDTLS_SSL_TLS1_3_LBL_LEN( client_cv ); + } + else + { /* from == MBEDTLS_SSL_IS_SERVER */ + memcpy( verify_buffer + idx, MBEDTLS_SSL_TLS1_3_LBL_WITH_LEN( server_cv ) ); + idx += MBEDTLS_SSL_TLS1_3_LBL_LEN( server_cv ); + } + + verify_buffer[idx++] = 0x0; + + memcpy( verify_buffer + idx, transcript_hash, transcript_hash_len ); + idx += transcript_hash_len; + + *verify_buffer_len = idx; +} -#if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) static int ssl_tls13_sig_alg_is_offered( mbedtls_ssl_context *ssl, uint16_t sig_alg ) { const uint16_t *tls13_sig_alg = ssl->conf->tls13_sig_algs; @@ -310,7 +303,7 @@ static int ssl_tls13_sig_alg_is_offered( mbedtls_ssl_context *ssl, uint16_t sig_ return 0; } -static int ssl_tls13_process_certificate_verify_parse( mbedtls_ssl_context *ssl, +static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end, const unsigned char *verify_buffer, @@ -350,7 +343,7 @@ static int ssl_tls13_process_certificate_verify_parse( mbedtls_ssl_context *ssl, * Check if algorithm in offered signature algorithms. Send `unsupported_certificate` * alert message on failure. */ - if( ssl_tls13_sig_alg_is_offered( ssl, algorithm ) == 0 ) + if( ! ssl_tls13_sig_alg_is_offered( ssl, algorithm ) ) { /* algorithm not in offered signature algorithms list */ MBEDTLS_SSL_DEBUG_MSG( 1, ( "Received signature algorithm(%04x) is not " @@ -429,12 +422,9 @@ static int ssl_tls13_process_certificate_verify_parse( mbedtls_ssl_context *ssl, break; #endif /* MBEDTLS_SHA512_C */ - default: - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Certificate Verify: Unknown signature algorithm." ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + default: + ret = MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; + break; } if( ret != 0 ) @@ -481,13 +471,6 @@ int mbedtls_ssl_tls13_process_certificate_verify( mbedtls_ssl_context *ssl ) unsigned char *buf; size_t buf_len; - if( mbedtls_ssl_tls1_3_some_psk_enabled( ssl ) ) - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); - - if( ssl->session_negotiate->peer_cert == NULL ) - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); - - MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) ); MBEDTLS_SSL_PROC_CHK( @@ -495,9 +478,9 @@ int mbedtls_ssl_tls13_process_certificate_verify( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, &buf, &buf_len ) ); /* Need to calculate the hash of the transcript first - * before reading the message since otherwise it gets - * included in the transcript - */ + * before reading the message since otherwise it gets + * included in the transcript + */ ret = mbedtls_ssl_get_handshake_transcript( ssl, ssl->handshake->ciphersuite_info->mac, transcript, sizeof( transcript ), @@ -514,15 +497,16 @@ int mbedtls_ssl_tls13_process_certificate_verify( mbedtls_ssl_context *ssl ) /* Create verify structure */ ssl_tls13_create_verify_structure( transcript, - transcript_len, - verify_buffer, - &verify_buffer_len, - !ssl->conf->endpoint ); + transcript_len, + verify_buffer, + &verify_buffer_len, + ( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT ) ? + MBEDTLS_SSL_IS_SERVER : + MBEDTLS_SSL_IS_CLIENT ); /* Process the message contents */ - MBEDTLS_SSL_PROC_CHK( - ssl_tls13_process_certificate_verify_parse( ssl, - buf, buf + buf_len, verify_buffer, verify_buffer_len ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls13_parse_certificate_verify( ssl, buf, + buf + buf_len, verify_buffer, verify_buffer_len ) ); mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, buf, buf_len ); From d0fc585b7e686c4837c5b9cb5e26f00c68d16aa6 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Fri, 29 Oct 2021 11:09:06 +0800 Subject: [PATCH 08/12] fix various issues Signed-off-by: Jerry Yu --- library/ssl_tls13_generic.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index d42e463a9..c83c98b18 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -221,14 +221,14 @@ int mbedtls_ssl_tls13_write_sig_alg_ext( mbedtls_ssl_context *ssl, /* * STATE HANDLING: Read CertificateVerify */ -/* Macro to express the length of the verify structure length. +/* Macro to express the maximum length of the verify structure. * * The structure is computed per TLS 1.3 specification as: * - 64 bytes of octet 32, * - 33 bytes for the context string * (which is either "TLS 1.3, client CertificateVerify" * or "TLS 1.3, server CertificateVerify"), - * - 1 byte for the octet 0x0, which servers as a separator, + * - 1 byte for the octet 0x0, which serves as a separator, * - 32 or 48 bytes for the Transcript-Hash(Handshake Context, Certificate) * (depending on the size of the transcript_hash) * @@ -252,7 +252,7 @@ int mbedtls_ssl_tls13_write_sig_alg_ext( mbedtls_ssl_context *ssl, * The caller has to ensure that the buffer has size at least * SSL_VERIFY_STRUCT_MAX_SIZE bytes. */ -static void ssl_tls13_create_verify_structure( unsigned char *transcript_hash, +static void ssl_tls13_create_verify_structure( const unsigned char *transcript_hash, size_t transcript_hash_len, unsigned char *verify_buffer, size_t *verify_buffer_len, @@ -291,23 +291,24 @@ static void ssl_tls13_create_verify_structure( unsigned char *transcript_hash, *verify_buffer_len = idx; } -static int ssl_tls13_sig_alg_is_offered( mbedtls_ssl_context *ssl, uint16_t sig_alg ) +static int ssl_tls13_sig_alg_is_offered( const mbedtls_ssl_context *ssl, + uint16_t sig_alg ) { const uint16_t *tls13_sig_alg = ssl->conf->tls13_sig_algs; - for( ; *tls13_sig_alg !=MBEDTLS_TLS13_SIG_NONE ; tls13_sig_alg++ ) + for( ; *tls13_sig_alg != MBEDTLS_TLS13_SIG_NONE ; tls13_sig_alg++ ) { if( *tls13_sig_alg == sig_alg ) - return 1; + return( 1 ); } - return 0; + return( 0 ); } static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, - const unsigned char *buf, - const unsigned char *end, - const unsigned char *verify_buffer, - size_t verify_buffer_len ) + const unsigned char *buf, + const unsigned char *end, + const unsigned char *verify_buffer, + size_t verify_buffer_len ) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; const unsigned char *p = buf; @@ -315,7 +316,7 @@ static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, size_t signature_len; mbedtls_pk_type_t sig_alg; mbedtls_md_type_t md_alg; - unsigned char verify_hash[MBEDTLS_TLS1_3_MD_MAX_SIZE]; + unsigned char verify_hash[MBEDTLS_MD_MAX_SIZE]; size_t verify_hash_len; /* @@ -340,7 +341,7 @@ static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, * certificates and decides to abort the handshake, then it MUST abort the handshake * with an appropriate certificate-related alert (by default, "unsupported_certificate"). * - * Check if algorithm in offered signature algorithms. Send `unsupported_certificate` + * Check if algorithm is an offered signature algorithm. Send `unsupported_certificate` * alert message on failure. */ if( ! ssl_tls13_sig_alg_is_offered( ssl, algorithm ) ) @@ -352,7 +353,7 @@ static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); } /* We currently only support ECDSA-based signatures */ @@ -441,7 +442,7 @@ static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, if( ( ret = mbedtls_pk_verify_ext( sig_alg, NULL, &ssl->session_negotiate->peer_cert->pk, md_alg, verify_hash, verify_hash_len, - buf, signature_len ) ) != 0 ) + p, signature_len ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_pk_verify_ext", ret ); @@ -455,7 +456,7 @@ static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, return( ret ); } - return( ret ); + return( 0 ); } #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ From daac3593318e57390baa7ba77e312e2ce963bab2 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Fri, 29 Oct 2021 20:01:42 +0800 Subject: [PATCH 09/12] Change check condition order Signed-off-by: Jerry Yu --- tests/ssl-opt.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 993021013..d43f43dba 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -8831,8 +8831,8 @@ run_test "TLS1.3: Test client hello msg work - openssl" \ -c "server hello, chosen ciphersuite: ( 1301 ) - TLS1-3-AES-128-GCM-SHA256" \ -c "ECDH curve: x25519" \ -c "=> ssl_tls1_3_process_server_hello" \ - -c "Certificate verification flags clear" \ - -c "<= parse encrypted extensions" + -c "<= parse encrypted extensions" \ + -c "Certificate verification flags clear" requires_gnutls_tls1_3 requires_gnutls_next_no_ticket @@ -8861,8 +8861,8 @@ run_test "TLS1.3: Test client hello msg work - gnutls" \ -c "server hello, chosen ciphersuite: ( 1301 ) - TLS1-3-AES-128-GCM-SHA256" \ -c "ECDH curve: x25519" \ -c "=> ssl_tls1_3_process_server_hello" \ - -c "Certificate verification flags clear" \ - -c "<= parse encrypted extensions" + -c "<= parse encrypted extensions" \ + -c "Certificate verification flags clear" # Test heap memory usage after handshake requires_config_enabled MBEDTLS_MEMORY_DEBUG From 6f87f2521c8da36e68b074c92774e30aa20fca44 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Fri, 29 Oct 2021 20:12:51 +0800 Subject: [PATCH 10/12] Refactor ssl_tls13_parse_certificate_verify Signed-off-by: Jerry Yu --- library/ssl_tls13_generic.c | 48 ++++++++++++++----------------------- 1 file changed, 18 insertions(+), 30 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index c83c98b18..45692d877 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -341,8 +341,7 @@ static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, * certificates and decides to abort the handshake, then it MUST abort the handshake * with an appropriate certificate-related alert (by default, "unsupported_certificate"). * - * Check if algorithm is an offered signature algorithm. Send `unsupported_certificate` - * alert message on failure. + * Check if algorithm is an offered signature algorithm. */ if( ! ssl_tls13_sig_alg_is_offered( ssl, algorithm ) ) { @@ -350,10 +349,7 @@ static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 1, ( "Received signature algorithm(%04x) is not " "offered.", ( unsigned int ) algorithm ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + goto error; } /* We currently only support ECDSA-based signatures */ @@ -373,10 +369,7 @@ static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, break; default: MBEDTLS_SSL_DEBUG_MSG( 1, ( "Certificate Verify: Unknown signature algorithm." ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + goto error; } MBEDTLS_SSL_DEBUG_MSG( 3, ( "Certificate Verify: Signature algorithm ( %04x )", @@ -388,10 +381,7 @@ static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, if( !mbedtls_pk_can_do( &ssl->session_negotiate->peer_cert->pk, sig_alg ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "signature algorithm doesn't match cert key" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + goto error; } MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); @@ -431,10 +421,7 @@ static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "hash computation error", ret ); - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + goto error; } MBEDTLS_SSL_DEBUG_BUF( 3, "verify hash", verify_hash, verify_hash_len ); @@ -442,21 +429,22 @@ static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, if( ( ret = mbedtls_pk_verify_ext( sig_alg, NULL, &ssl->session_negotiate->peer_cert->pk, md_alg, verify_hash, verify_hash_len, - p, signature_len ) ) != 0 ) + p, signature_len ) ) == 0 ) { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_pk_verify_ext", ret ); - - /* RFC 8446 section 4.4.3 - * - * If the verification fails, the receiver MUST terminate the handshake - * with a "decrypt_error" alert. - */ - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECRYPT_ERROR, ret ); - - return( ret ); + return( 0 ); } + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_pk_verify_ext", ret ); + +error: + /* RFC 8446 section 4.4.3 + * + * If the verification fails, the receiver MUST terminate the handshake + * with a "decrypt_error" alert. + */ + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECRYPT_ERROR, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return( 0 ); } #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ From 834886d2112b54dca7781d2da1a239434bd43f2c Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Sat, 30 Oct 2021 13:26:15 +0800 Subject: [PATCH 11/12] Add certificate verify check Signed-off-by: Jerry Yu --- tests/ssl-opt.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index d43f43dba..037dfa518 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -8832,7 +8832,8 @@ run_test "TLS1.3: Test client hello msg work - openssl" \ -c "ECDH curve: x25519" \ -c "=> ssl_tls1_3_process_server_hello" \ -c "<= parse encrypted extensions" \ - -c "Certificate verification flags clear" + -c "Certificate verification flags clear" \ + -c "<= parse certificate verify" requires_gnutls_tls1_3 requires_gnutls_next_no_ticket @@ -8862,7 +8863,8 @@ run_test "TLS1.3: Test client hello msg work - gnutls" \ -c "ECDH curve: x25519" \ -c "=> ssl_tls1_3_process_server_hello" \ -c "<= parse encrypted extensions" \ - -c "Certificate verification flags clear" + -c "Certificate verification flags clear" \ + -c "<= parse certificate verify" # Test heap memory usage after handshake requires_config_enabled MBEDTLS_MEMORY_DEBUG From 5398c10b897de8f4f29aa897484621b76748bea4 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Fri, 5 Nov 2021 13:32:38 +0800 Subject: [PATCH 12/12] Add return value check for cerificate verify Signed-off-by: Jerry Yu --- library/ssl_tls13_generic.c | 1 + tests/ssl-opt.sh | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 45692d877..75b11c93a 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -503,6 +503,7 @@ int mbedtls_ssl_tls13_process_certificate_verify( mbedtls_ssl_context *ssl ) cleanup: MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate verify" ) ); + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_tls13_process_certificate_verify", ret ); return( ret ); #else ((void) ssl); diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 037dfa518..0e78356bc 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -8833,7 +8833,9 @@ run_test "TLS1.3: Test client hello msg work - openssl" \ -c "=> ssl_tls1_3_process_server_hello" \ -c "<= parse encrypted extensions" \ -c "Certificate verification flags clear" \ - -c "<= parse certificate verify" + -c "=> parse certificate verify" \ + -c "<= parse certificate verify" \ + -c "mbedtls_ssl_tls13_process_certificate_verify() returned 0" requires_gnutls_tls1_3 requires_gnutls_next_no_ticket @@ -8864,7 +8866,9 @@ run_test "TLS1.3: Test client hello msg work - gnutls" \ -c "=> ssl_tls1_3_process_server_hello" \ -c "<= parse encrypted extensions" \ -c "Certificate verification flags clear" \ - -c "<= parse certificate verify" + -c "=> parse certificate verify" \ + -c "<= parse certificate verify" \ + -c "mbedtls_ssl_tls13_process_certificate_verify() returned 0" # Test heap memory usage after handshake requires_config_enabled MBEDTLS_MEMORY_DEBUG