From 5882dd08562e1b508383c9dce989db198f7a920b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 6 Jun 2019 16:25:57 +0100 Subject: [PATCH 1/2] Remove CRT digest from SSL session if !RENEGO + !KEEP_PEER_CERT If `MBEDTLS_SSL_KEEP_PEER_CERTIFICATE` is not set, `mbedtls_ssl_session` contains the digest of the peer's certificate for the sole purpose of detecting a CRT change on renegotiation. Hence, it is not needed if renegotiation is disabled. This commit removes the `peer_cert_digest` fields (and friends) from `mbedtls_ssl_session` if `!MBEDTLS_SSL_KEEP_PEER_CERTIFICATE + !MBEDTLS_SSL_RENEGOTIATION`, which is a sensible configuration for constrained devices. Apart from straightforward replacements of `if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)` by `if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) && \ defined(MBEDTLS_SSL_RENEGOTIATION)`, there's one notable change: On the server-side, the CertificateVerify parsing function is a no-op if the client hasn't sent a certificate. So far, this was determined by either looking at the peer CRT or the peer CRT digest in the SSL session structure (depending on the setting of `MBEDTLS_SSL_KEEP_PEER_CERTIFICATE`), which now no longer works if `MBEDTLS_SSL_KEEP_PEER_CERTIFICATE` is unset. Instead, this function now checks whether the temporary copy of the peer's public key within the handshake structure is initialized or not (which is also a beneficial simplification in its own right, because the pubkey is all the function needs anyway). --- include/mbedtls/ssl.h | 4 +-- library/ssl_cli.c | 17 +++++++---- library/ssl_srv.c | 42 +++++++++++++--------------- library/ssl_tls.c | 40 ++++++++++++++++---------- tests/suites/test_suite_ssl.function | 22 +++++++-------- 5 files changed, 71 insertions(+), 54 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 9a36009a7..eeb03e145 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -856,13 +856,13 @@ struct mbedtls_ssl_session #if defined(MBEDTLS_X509_CRT_PARSE_C) #if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) mbedtls_x509_crt *peer_cert; /*!< peer X.509 cert chain */ -#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#elif defined(MBEDTLS_SSL_RENEGOTIATION) /*! The digest of the peer's end-CRT. This must be kept to detect CRT * changes during renegotiation, mitigating the triple handshake attack. */ unsigned char *peer_cert_digest; size_t peer_cert_digest_len; mbedtls_md_type_t peer_cert_digest_type; -#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ uint32_t verify_result; /*!< verification result */ diff --git a/library/ssl_cli.c b/library/ssl_cli.c index e5185d4c5..174e8b150 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2288,7 +2288,7 @@ static int ssl_write_encrypted_pms( mbedtls_ssl_context *ssl, int ret; size_t len_bytes = ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ? 0 : 2; unsigned char *p = ssl->handshake->premaster + pms_offset; - mbedtls_pk_context * peer_pk; + mbedtls_pk_context *peer_pk = NULL; if( offset + len_bytes > MBEDTLS_SSL_OUT_CONTENT_LEN ) { @@ -2315,16 +2315,23 @@ static int ssl_write_encrypted_pms( mbedtls_ssl_context *ssl, ssl->handshake->pmslen = 48; #if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) - peer_pk = &ssl->handshake->peer_pubkey; + /* Because the peer CRT pubkey is embedded into the handshake + * params currently, and there's no 'is_init' functions for PK + * contexts, we need to break the abstraction and peek into + * the PK context to see if it has been initialized. */ + if( ssl->handshake->peer_pubkey.pk_info != NULL ) + peer_pk = &ssl->handshake->peer_pubkey; #else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ - if( ssl->session_negotiate->peer_cert == NULL ) + if( ssl->session_negotiate->peer_cert != NULL ) + peer_pk = &ssl->session_negotiate->peer_cert->pk; +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + + if( peer_pk == NULL ) { /* Should never happen */ MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } - peer_pk = &ssl->session_negotiate->peer_cert->pk; -#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ /* * Now write it out, encrypted diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 06fe3ee2c..a8821f319 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -4161,7 +4161,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) mbedtls_md_type_t md_alg; const mbedtls_ssl_ciphersuite_t *ciphersuite_info = ssl->handshake->ciphersuite_info; - mbedtls_pk_context * peer_pk; + mbedtls_pk_context *peer_pk = NULL; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) ); @@ -4172,21 +4172,30 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) return( 0 ); } -#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) - if( ssl->session_negotiate->peer_cert == NULL ) + /* Skip if we haven't received a certificate from the client. + * If MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is set, this can be + * inferred from the setting of mbedtls_ssl_session::peer_cert. + * If MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is not set, it can + * be inferred from whether we've held back the peer CRT's + * public key in mbedtls_ssl_handshake_params::peer_pubkey. */ +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + /* Because the peer CRT pubkey is embedded into the handshake + * params currently, and there's no 'is_init' functions for PK + * contexts, we need to break the abstraction and peek into + * the PK context to see if it has been initialized. */ + if( ssl->handshake->peer_pubkey.pk_info != NULL ) + peer_pk = &ssl->handshake->peer_pubkey; +#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + if( ssl->session_negotiate->peer_cert != NULL ) + peer_pk = &ssl->session_negotiate->peer_cert->pk; +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + + if( peer_pk == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); ssl->state++; return( 0 ); } -#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ - if( ssl->session_negotiate->peer_cert_digest == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); - ssl->state++; - return( 0 ); - } -#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ /* Read the message without adding it to the checksum */ ret = mbedtls_ssl_read_record( ssl, 0 /* no checksum update */ ); @@ -4208,17 +4217,6 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) i = mbedtls_ssl_hs_hdr_len( ssl ); -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) - peer_pk = &ssl->handshake->peer_pubkey; -#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ - if( ssl->session_negotiate->peer_cert == NULL ) - { - /* Should never happen */ - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); - } - peer_pk = &ssl->session_negotiate->peer_cert->pk; -#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ - /* * struct { * SignatureAndHashAlgorithm algorithm; -- TLS 1.2 only diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 364216287..5ff66eb7d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -395,7 +395,7 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, return( ret ); } } -#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#elif defined(MBEDTLS_SSL_RENEGOTIATION) if( src->peer_cert_digest != NULL ) { dst->peer_cert_digest = @@ -408,7 +408,7 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, dst->peer_cert_digest_type = src->peer_cert_digest_type; dst->peer_cert_digest_len = src->peer_cert_digest_len; } -#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ @@ -5994,7 +5994,7 @@ static void ssl_clear_peer_cert( mbedtls_ssl_session *session ) mbedtls_free( session->peer_cert ); session->peer_cert = NULL; } -#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#elif defined(MBEDTLS_SSL_RENEGOTIATION) if( session->peer_cert_digest != NULL ) { /* Zeroization is not necessary. */ @@ -6003,7 +6003,9 @@ static void ssl_clear_peer_cert( mbedtls_ssl_session *session ) session->peer_cert_digest_type = MBEDTLS_MD_NONE; session->peer_cert_digest_len = 0; } -#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#else + ((void) session); +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */ } #endif /* MBEDTLS_X509_CRT_PARSE_C */ @@ -6179,7 +6181,7 @@ static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, return( memcmp( peer_crt->raw.p, crt_buf, crt_buf_len ) ); } -#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#elif defined(MBEDTLS_SSL_RENEGOTIATION) static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, unsigned char *crt_buf, size_t crt_buf_len ) @@ -6207,7 +6209,7 @@ static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, return( memcmp( tmp_digest, peer_cert_digest, digest_len ) ); } -#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */ #endif /* MBEDTLS_SSL_RENEGOTIATION && MBEDTLS_SSL_CLI_C */ /* @@ -6589,6 +6591,8 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl, } #if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + +#if defined(MBEDTLS_SSL_RENEGOTIATION) static int ssl_remember_peer_crt_digest( mbedtls_ssl_context *ssl, unsigned char *start, size_t len ) { @@ -6619,6 +6623,7 @@ static int ssl_remember_peer_crt_digest( mbedtls_ssl_context *ssl, return( ret ); } +#endif /* MBEDTLS_SSL_RENEGOTIATION */ static int ssl_remember_peer_pubkey( mbedtls_ssl_context *ssl, unsigned char *start, size_t len ) @@ -6733,16 +6738,21 @@ crt_verify: #if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) { - unsigned char *crt_start, *pk_start; - size_t crt_len, pk_len; + size_t pk_len; + unsigned char *pk_start; /* We parse the CRT chain without copying, so * these pointers point into the input buffer, * and are hence still valid after freeing the * CRT chain. */ +#if defined(MBEDTLS_SSL_RENEGOTIATION) + unsigned char *crt_start; + size_t crt_len; + crt_start = chain->raw.p; crt_len = chain->raw.len; +#endif /* MBEDTLS_SSL_RENEGOTIATION */ pk_start = chain->pk_raw.p; pk_len = chain->pk_raw.len; @@ -6753,9 +6763,11 @@ crt_verify: mbedtls_free( chain ); chain = NULL; +#if defined(MBEDTLS_SSL_RENEGOTIATION) ret = ssl_remember_peer_crt_digest( ssl, crt_start, crt_len ); if( ret != 0 ) goto exit; +#endif /* MBEDTLS_SSL_RENEGOTIATION */ ret = ssl_remember_peer_pubkey( ssl, pk_start, pk_len ); if( ret != 0 ) @@ -9275,7 +9287,7 @@ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, } } -#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#elif defined(MBEDTLS_SSL_RENEGOTIATION) /* Digest of peer certificate */ if( session->peer_cert_digest != NULL ) { @@ -9298,7 +9310,7 @@ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, *p++ = 0; } } -#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ /* @@ -9443,9 +9455,9 @@ static int ssl_session_load( mbedtls_ssl_session *session, #if defined(MBEDTLS_X509_CRT_PARSE_C) #if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) session->peer_cert = NULL; -#else +#elif defined(MBEDTLS_SSL_RENEGOTIATION) session->peer_cert_digest = NULL; -#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */ #endif #if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) session->ticket = NULL; @@ -9491,7 +9503,7 @@ static int ssl_session_load( mbedtls_ssl_session *session, p += cert_len; } -#else /* defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#elif defined(MBEDTLS_SSL_RENEGOTIATION) /* Deserialize CRT digest from the end of the ticket. */ if( 2 > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); @@ -9520,7 +9532,7 @@ static int ssl_session_load( mbedtls_ssl_session *session, session->peer_cert_digest_len ); p += session->peer_cert_digest_len; } -#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ /* diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index b59c204e2..6acbbef59 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -295,7 +295,14 @@ static int ssl_populate_session( mbedtls_ssl_session *session, if( ret != 0 ) return( ret ); -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + /* Move temporary CRT. */ + session->peer_cert = mbedtls_calloc( 1, sizeof( *session->peer_cert ) ); + if( session->peer_cert == NULL ) + return( -1 ); + *session->peer_cert = tmp_crt; + memset( &tmp_crt, 0, sizeof( tmp_crt ) ); +#elif defined(MBEDTLS_SSL_RENEGOTIATION) /* Calculate digest of temporary CRT. */ session->peer_cert_digest = mbedtls_calloc( 1, MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN ); @@ -311,14 +318,7 @@ static int ssl_populate_session( mbedtls_ssl_session *session, MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE; session->peer_cert_digest_len = MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN; -#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ - /* Move temporary CRT. */ - session->peer_cert = mbedtls_calloc( 1, sizeof( *session->peer_cert ) ); - if( session->peer_cert == NULL ) - return( -1 ); - *session->peer_cert = tmp_crt; - memset( &tmp_crt, 0, sizeof( tmp_crt ) ); -#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */ mbedtls_x509_crt_free( &tmp_crt ); } @@ -717,7 +717,7 @@ void ssl_serialize_session_save_load( int ticket_len, char *crt_file ) restored.peer_cert->raw.p, original.peer_cert->raw.len ) == 0 ); } -#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#elif defined(MBEDTLS_SSL_RENEGOTIATION) TEST_ASSERT( original.peer_cert_digest_type == restored.peer_cert_digest_type ); TEST_ASSERT( original.peer_cert_digest_len == @@ -730,7 +730,7 @@ void ssl_serialize_session_save_load( int ticket_len, char *crt_file ) restored.peer_cert_digest, original.peer_cert_digest_len ) == 0 ); } -#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ TEST_ASSERT( original.verify_result == restored.verify_result ); From e256f7c9ae80b122a651af697b4f38dc3e0bbd98 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 7 Jun 2019 11:14:53 +0100 Subject: [PATCH 2/2] Add test for !KEEP_PEER_CERTIFICATE + !RENEGOTIAITON to all.sh --- tests/scripts/all.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 628a05644..42734c5c9 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -901,6 +901,23 @@ component_test_asan_remove_peer_certificate () { if_build_succeeded tests/compat.sh } +component_test_asan_remove_peer_certificate_no_renego () { + msg "build: default config with MBEDTLS_SSL_KEEP_PEER_CERTIFICATE and MBEDTLS_SSL_RENEGOTIATION disabled (ASan build)" + scripts/config.pl unset MBEDTLS_SSL_KEEP_PEER_CERTIFICATE + scripts/config.pl unset MBEDTLS_SSL_RENEGOTIATION + CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . + make + + msg "test: !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE" + make test + + msg "test: ssl-opt.sh, !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE + !MBEDTLS_SSL_RENEGOTIATION" + if_build_succeeded tests/ssl-opt.sh + + msg "test: compat.sh, !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE + !MBEDTLS_SSL_RENEGOTIATION" + if_build_succeeded tests/compat.sh +} + component_test_no_max_fragment_length_small_ssl_out_content_len () { msg "build: no MFL extension, small SSL_OUT_CONTENT_LEN (ASan build)" scripts/config.pl unset MBEDTLS_SSL_MAX_FRAGMENT_LENGTH