From fed37ed0391f5b25393d5802c5ef434314a53894 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 15 Aug 2017 13:27:41 +0200 Subject: [PATCH] Extract some code to separate function Goals include: - reducing the number of local variables in the main function (so that we don't have to worry about saving/restoring them) - reducing the number exit points in the main function, making it easier to update ssl->state only right before we return --- library/ssl_tls.c | 141 ++++++++++++++++++++++++++-------------------- 1 file changed, 81 insertions(+), 60 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 48bb33683..f91e2a885 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4309,62 +4309,16 @@ write_msg: return( ret ); } -int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) +/* + * Once the certificate message is read, parse it into a cert chain and + * perform basic checks, but leave actual verification to the caller + */ +static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) { - int ret = MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE; + int ret; size_t i, n; - const mbedtls_ssl_ciphersuite_t * const ciphersuite_info = - ssl->transform_negotiate->ciphersuite_info; -#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 uint8_t alert; - MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate" ) ); - - if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_DHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE ) - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); - ssl->state++; - return( 0 ); - } - -#if defined(MBEDTLS_SSL_SRV_C) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK ) - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); - ssl->state++; - return( 0 ); - } - - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && - authmode == MBEDTLS_SSL_VERIFY_NONE ) - { - ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_SKIP_VERIFY; - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); - ssl->state++; - return( 0 ); - } -#endif - - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) - { - /* mbedtls_ssl_read_record may have sent an alert already. We - let it decide whether to alert. */ - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); - return( ret ); - } - - ssl->state++; - #if defined(MBEDTLS_SSL_SRV_C) #if defined(MBEDTLS_SSL_PROTO_SSL3) /* @@ -4384,10 +4338,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) one. The client should know what's going on, so we don't send an alert. */ ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_MISSING; - if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) - return( 0 ); - else - return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); + return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); } } #endif /* MBEDTLS_SSL_PROTO_SSL3 */ @@ -4408,10 +4359,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) one. The client should know what's going on, so we don't send an alert. */ ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_MISSING; - if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) - return( 0 ); - else - return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); + return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); } } #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ @@ -4555,6 +4503,75 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_RENEGOTIATION && MBEDTLS_SSL_CLI_C */ + return( 0 ); +} + +int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) +{ + int ret; + const mbedtls_ssl_ciphersuite_t * const ciphersuite_info = + ssl->transform_negotiate->ciphersuite_info; +#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 + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate" ) ); + + if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || + ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_DHE_PSK || + ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || + ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); + ssl->state++; + return( 0 ); + } + +#if defined(MBEDTLS_SSL_SRV_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && + ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); + ssl->state++; + return( 0 ); + } + + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && + authmode == MBEDTLS_SSL_VERIFY_NONE ) + { + ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_SKIP_VERIFY; + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); + ssl->state++; + return( 0 ); + } +#endif + + if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + { + /* mbedtls_ssl_read_record may have sent an alert already. We + let it decide whether to alert. */ + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); + return( ret ); + } + + if( ( ret = ssl_parse_certificate_chain( ssl ) ) != 0 ) + { +#if defined(MBEDTLS_SSL_SRV_C) + if( ret == MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE && + authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) + { + ret = 0; + } +#endif + + ssl->state++; + return( ret ); + } + if( authmode != MBEDTLS_SSL_VERIFY_NONE ) { mbedtls_x509_crt *ca_chain; @@ -4641,6 +4658,8 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) 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. */ @@ -4683,6 +4702,8 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_DEBUG_C */ } + ssl->state++; + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); return( ret );