From 0b23f167ba288083a46deb7655e9d9c1f8821c8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 24 Aug 2017 12:08:33 +0200 Subject: [PATCH] SSL: rework restart state handling As done by previous commits for ECC and ECDSA: - use explicit state assignments rather than increment - always place the state update right before the operation label This will make it easier to add restart support for other operations later if desired. SSL-specific changes: - remove useless states: when the last restartable operation on a message is complete, ssl->state is incremented already, so we don't need any additional state update: ecrs_state is only meant to complement ssl->state - rename remaining states consistently as _ - move some labels closer to the actual operation when possible (no assignment to variables used after the label between its previous and current position) --- include/mbedtls/ssl_internal.h | 20 +++++-------- library/ssl_cli.c | 54 +++++++++++++--------------------- library/ssl_tls.c | 9 ++---- 3 files changed, 30 insertions(+), 53 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 4cfe1540c..036b60a06 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -229,18 +229,14 @@ struct mbedtls_ssl_handshake_params #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) int ecrs_enabled; /*!< Handshake supports EC restart? */ mbedtls_x509_crt_restart_ctx ecrs_ctx; /*!< restart context */ - enum { - ssl_ecrs_init = 0, /*!< just getting started */ - ssl_ecrs_crt_parsed, /*!< server certificate was parsed */ - ssl_ecrs_crt_verified, /*!< server certificate was verified*/ - ssl_ecrs_ske_read, /*!< ServerKeyExchange was read */ - ssl_ecrs_ske_verified, /*!< ServerKeyExchange was verified */ - ssl_ecrs_ecdh_public_done, /*!< wrote ECDHE public share */ - ssl_ecrs_ecdh_completed, /*!< completed ECDHE key exchange */ - ssl_ecrs_keys_derived, /*!< ssl_derive_keys() done */ - ssl_ecrs_pk_sign_done, /*!< done writing CertificateVerify */ - } ecrs_state; /*!< state for restartable ECC */ - size_t ecrs_n; /*!< place for seving a length */ + enum { /* this complements ssl->state with info on intra-state operations */ + ssl_ecrs_none = 0, /*!< nothing going on (yet) */ + ssl_ecrs_crt_verify, /*!< Certificate: crt_verify() */ + ssl_ecrs_ske_start_processing, /*!< ServerKeyExchange: step 1 */ + ssl_ecrs_ske_ecdh_calc_secret, /*!< ServerKeyExchange: ECDH step 2 */ + ssl_ecrs_crt_vrfy_sign, /*!< CertificateVerify: pk_sign() */ + } ecrs_state; /*!< current (or last) operation */ + size_t ecrs_n; /*!< place for saving a length */ #endif #if defined(MBEDTLS_SSL_PROTO_DTLS) unsigned int out_msg_seq; /*!< Outgoing handshake sequence number */ diff --git a/library/ssl_cli.c b/library/ssl_cli.c index d53f7b227..cf83e8fae 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2305,9 +2305,9 @@ static int ssl_parse_server_key_exchange( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) if( ssl->handshake->ecrs_enabled && - ssl->handshake->ecrs_state == ssl_ecrs_ske_read ) + ssl->handshake->ecrs_state == ssl_ecrs_ske_start_processing ) { - goto ske_process; + goto start_processing; } #endif @@ -2317,12 +2317,6 @@ static int ssl_parse_server_key_exchange( mbedtls_ssl_context *ssl ) return( ret ); } -#if defined(MBEDTLS_SSL__ECP_RESTARTABLE) - if( ssl->handshake->ecrs_enabled ) - ssl->handshake->ecrs_state++; - -ske_process: -#endif if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server key exchange message" ) ); @@ -2354,6 +2348,12 @@ ske_process: return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } +#if defined(MBEDTLS_SSL__ECP_RESTARTABLE) + if( ssl->handshake->ecrs_enabled ) + ssl->handshake->ecrs_state = ssl_ecrs_ske_start_processing; + +start_processing: +#endif p = ssl->in_msg + mbedtls_ssl_hs_hdr_len( ssl ); end = ssl->in_msg + ssl->in_hslen; MBEDTLS_SSL_DEBUG_BUF( 3, "server key exchange", p, end - p ); @@ -2630,11 +2630,6 @@ ske_process: MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_pk_verify", ret ); return( ret ); } - -#if defined(MBEDTLS_SSL__ECP_RESTARTABLE) - if( ssl->handshake->ecrs_enabled ) - ssl->handshake->ecrs_state++; -#endif } #endif /* MBEDTLS_KEY_EXCHANGE__WITH_SERVER_SIGNATURE__ENABLED */ @@ -2901,7 +2896,7 @@ static int ssl_write_client_key_exchange( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) if( ssl->handshake->ecrs_enabled ) { - if( ssl->handshake->ecrs_state == ssl_ecrs_ecdh_public_done ) + if( ssl->handshake->ecrs_state == ssl_ecrs_ske_ecdh_calc_secret ) goto ecdh_calc_secret; mbedtls_ecdh_enable_restart( &ssl->handshake->ecdh_ctx ); @@ -2924,7 +2919,7 @@ static int ssl_write_client_key_exchange( mbedtls_ssl_context *ssl ) if( ssl->handshake->ecrs_enabled ) { ssl->handshake->ecrs_n = n; - ssl->handshake->ecrs_state++; + ssl->handshake->ecrs_state = ssl_ecrs_ske_ecdh_calc_secret; } ecdh_calc_secret: @@ -2942,11 +2937,6 @@ ecdh_calc_secret: } MBEDTLS_SSL_DEBUG_MPI( 3, "ECDH: z", &ssl->handshake->ecdh_ctx.z ); - -#if defined(MBEDTLS_SSL__ECP_RESTARTABLE) - if( ssl->handshake->ecrs_enabled ) - ssl->handshake->ecrs_state++; -#endif } else #endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED || @@ -3167,9 +3157,9 @@ static int ssl_write_certificate_verify( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) if( ssl->handshake->ecrs_enabled && - ssl->handshake->ecrs_state == ssl_ecrs_keys_derived ) + ssl->handshake->ecrs_state == ssl_ecrs_crt_vrfy_sign ) { - goto keys_derived; + goto sign; } #endif @@ -3179,12 +3169,6 @@ static int ssl_write_certificate_verify( mbedtls_ssl_context *ssl ) return( ret ); } -#if defined(MBEDTLS_SSL__ECP_RESTARTABLE) - if( ssl->handshake->ecrs_enabled ) - ssl->handshake->ecrs_state++; - -keys_derived: -#endif if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK || ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || @@ -3210,8 +3194,15 @@ keys_derived: } /* - * Make an RSA signature of the handshake digests + * Make a signature of the handshake digests */ +#if defined(MBEDTLS_SSL__ECP_RESTARTABLE) + if( ssl->handshake->ecrs_enabled ) + ssl->handshake->ecrs_state = ssl_ecrs_crt_vrfy_sign; + +sign: +#endif + ssl->handshake->calc_verify( ssl, hash ); #if defined(MBEDTLS_SSL_PROTO_SSL3) || defined(MBEDTLS_SSL_PROTO_TLS1) || \ @@ -3302,11 +3293,6 @@ keys_derived: return( ret ); } -#if defined(MBEDTLS_SSL__ECP_RESTARTABLE) - if( ssl->handshake->ecrs_enabled ) - ssl->handshake->ecrs_state++; -#endif - ssl->out_msg[4 + offset] = (unsigned char)( n >> 8 ); ssl->out_msg[5 + offset] = (unsigned char)( n ); diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f3cde7f0a..6d9420c61 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4554,7 +4554,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) if( ssl->handshake->ecrs_enabled && - ssl->handshake->ecrs_state == ssl_ecrs_crt_parsed ) + ssl->handshake->ecrs_state == ssl_ecrs_crt_verify ) { goto crt_verify; } @@ -4584,7 +4584,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) if( ssl->handshake->ecrs_enabled) - ssl->handshake->ecrs_state++; + ssl->handshake->ecrs_state = ssl_ecrs_crt_verify; crt_verify: if( ssl->handshake->ecrs_enabled) @@ -4726,11 +4726,6 @@ crt_verify: #endif /* MBEDTLS_DEBUG_C */ } -#if defined(MBEDTLS_SSL__ECP_RESTARTABLE) - if( ssl->handshake->ecrs_enabled) - ssl->handshake->ecrs_state++; -#endif - ssl->state++; MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) );