From 2b20516b60dcd098b333d710b5ad462d8701b15d Mon Sep 17 00:00:00 2001 From: Jarno Lamsa Date: Tue, 12 Nov 2019 15:36:21 +0200 Subject: [PATCH] Make TLS state changes explicit This is to enable hardening the security when changing states in state machine so that the state cannot be changed by bit flipping. The later commit changes the enumerations so that the states have large hamming distance in between them to prevent this kind of attack. --- library/ssl_cli.c | 20 +++++----- library/ssl_srv.c | 24 ++++++------ library/ssl_tls.c | 98 +++++++++++++++++++++++++++++++++++++++++------ 3 files changed, 109 insertions(+), 33 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 7c03b41f9..2c209d3e9 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -1116,7 +1116,7 @@ static int ssl_write_client_hello( mbedtls_ssl_context *ssl ) ssl->out_msgtype = MBEDTLS_SSL_MSG_HANDSHAKE; ssl->out_msg[0] = MBEDTLS_SSL_HS_CLIENT_HELLO; - ssl->state++; + ssl->state = MBEDTLS_SSL_SERVER_HELLO; #if defined(MBEDTLS_SSL_PROTO_DTLS) if( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) @@ -1839,7 +1839,7 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) else { /* Start a new session */ - ssl->state++; + ssl->state = MBEDTLS_SSL_SERVER_CERTIFICATE; #if defined(MBEDTLS_HAVE_TIME) ssl->session_negotiate->start = mbedtls_time( NULL ); #endif @@ -3143,7 +3143,7 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) if( ! mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate request" ) ); - ssl->state++; + ssl->state = MBEDTLS_SSL_SERVER_HELLO_DONE; return( 0 ); } @@ -3165,7 +3165,7 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) if( ! mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate request" ) ); - ssl->state++; + ssl->state = MBEDTLS_SSL_SERVER_HELLO_DONE; return( 0 ); } @@ -3183,7 +3183,7 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } - ssl->state++; + ssl->state = MBEDTLS_SSL_SERVER_HELLO_DONE; ssl->client_auth = ( ssl->in_msg[0] == MBEDTLS_SSL_HS_CERTIFICATE_REQUEST ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "got %s certificate request", @@ -3340,7 +3340,7 @@ static int ssl_parse_server_hello_done( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO_DONE ); } - ssl->state++; + ssl->state = MBEDTLS_SSL_CLIENT_CERTIFICATE; #if defined(MBEDTLS_SSL_PROTO_DTLS) if( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) @@ -3827,7 +3827,7 @@ static int ssl_write_certificate_verify( mbedtls_ssl_context *ssl ) if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate verify" ) ); - ssl->state++; + ssl->state = MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC; return( 0 ); } @@ -3866,14 +3866,14 @@ static int ssl_write_certificate_verify( mbedtls_ssl_context *ssl ) if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate verify" ) ); - ssl->state++; + ssl->state = MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC; return( 0 ); } if( ssl->client_auth == 0 || mbedtls_ssl_own_cert( ssl ) == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate verify" ) ); - ssl->state++; + ssl->state = MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC; return( 0 ); } @@ -3997,7 +3997,7 @@ sign: ssl->out_msgtype = MBEDTLS_SSL_MSG_HANDSHAKE; ssl->out_msg[0] = MBEDTLS_SSL_HS_CERTIFICATE_VERIFY; - ssl->state++; + ssl->state = MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC; if( ( ret = mbedtls_ssl_write_handshake_msg( ssl ) ) != 0 ) { diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 1ef8f9466..fb64a2be4 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -1360,7 +1360,7 @@ have_ciphersuite_v2: } ssl->in_left = 0; - ssl->state++; + ssl->state = MBEDTLS_SSL_SERVER_HELLO; MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse client hello v2" ) ); @@ -2298,7 +2298,7 @@ have_ciphersuite: mbedtls_ssl_get_ciphersuite_name( mbedtls_ssl_session_get_ciphersuite( ssl->session_negotiate ) ) ) ); - ssl->state++; + ssl->state = MBEDTLS_SSL_SERVER_HELLO; #if defined(MBEDTLS_SSL_PROTO_DTLS) if( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) @@ -2858,7 +2858,7 @@ static int ssl_write_server_hello( mbedtls_ssl_context *ssl ) * New session, create a new session id, * unless we're about to issue a session ticket */ - ssl->state++; + ssl->state = MBEDTLS_SSL_SERVER_CERTIFICATE; #if defined(MBEDTLS_HAVE_TIME) ssl->session_negotiate->start = mbedtls_time( NULL ); @@ -3008,7 +3008,7 @@ static int ssl_write_certificate_request( mbedtls_ssl_context *ssl ) if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate request" ) ); - ssl->state++; + ssl->state = MBEDTLS_SSL_SERVER_HELLO_DONE; return( 0 ); } @@ -3030,7 +3030,7 @@ static int ssl_write_certificate_request( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> write certificate request" ) ); - ssl->state++; + ssl->state = MBEDTLS_SSL_SERVER_HELLO_DONE; #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) if( ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET ) @@ -3693,7 +3693,7 @@ static int ssl_write_server_key_exchange( mbedtls_ssl_context *ssl ) /* Key exchanges not involving ephemeral keys don't use * ServerKeyExchange, so end here. */ MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write server key exchange" ) ); - ssl->state++; + ssl->state = MBEDTLS_SSL_CERTIFICATE_REQUEST; return( 0 ); } #endif /* MBEDTLS_KEY_EXCHANGE__SOME_NON_PFS__ENABLED */ @@ -3751,7 +3751,7 @@ static int ssl_write_server_key_exchange( mbedtls_ssl_context *ssl ) ssl->out_msgtype = MBEDTLS_SSL_MSG_HANDSHAKE; ssl->out_msg[0] = MBEDTLS_SSL_HS_SERVER_KEY_EXCHANGE; - ssl->state++; + ssl->state = MBEDTLS_SSL_CERTIFICATE_REQUEST; if( ( ret = mbedtls_ssl_write_handshake_msg( ssl ) ) != 0 ) { @@ -3773,7 +3773,7 @@ static int ssl_write_server_hello_done( mbedtls_ssl_context *ssl ) ssl->out_msgtype = MBEDTLS_SSL_MSG_HANDSHAKE; ssl->out_msg[0] = MBEDTLS_SSL_HS_SERVER_HELLO_DONE; - ssl->state++; + ssl->state = MBEDTLS_SSL_CLIENT_CERTIFICATE; #if defined(MBEDTLS_SSL_PROTO_DTLS) if( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) @@ -4422,7 +4422,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); - ssl->state++; + ssl->state = MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC; return( 0 ); } @@ -4450,7 +4450,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); - ssl->state++; + ssl->state = MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC; return( 0 ); } @@ -4478,7 +4478,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) if( peer_pk == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); - ssl->state++; + ssl->state = MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC; return( 0 ); } @@ -4490,7 +4490,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) goto exit; } - ssl->state++; + ssl->state = MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC; /* Process the message contents */ if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE || diff --git a/library/ssl_tls.c b/library/ssl_tls.c index c12af9684..fa132ea2d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6740,7 +6740,14 @@ int mbedtls_ssl_write_certificate( mbedtls_ssl_context *ssl ) if( !mbedtls_ssl_ciphersuite_uses_srv_cert( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate" ) ); - ssl->state++; + if( ssl->state == MBEDTLS_SSL_CLIENT_CERTIFICATE ) + { + ssl->state = MBEDTLS_SSL_CLIENT_KEY_EXCHANGE; + } + else if( ssl->state == MBEDTLS_SSL_SERVER_CERTIFICATE ) + { + ssl->state = MBEDTLS_SSL_SERVER_KEY_EXCHANGE; + } return( 0 ); } @@ -6758,7 +6765,14 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) if( !mbedtls_ssl_ciphersuite_uses_srv_cert( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); - ssl->state++; + if( ssl->state == MBEDTLS_SSL_CLIENT_CERTIFICATE ) + { + ssl->state = MBEDTLS_SSL_CLIENT_KEY_EXCHANGE; + } + else if( ssl->state == MBEDTLS_SSL_SERVER_CERTIFICATE ) + { + ssl->state = MBEDTLS_SSL_SERVER_KEY_EXCHANGE; + } return( 0 ); } @@ -6782,7 +6796,14 @@ int mbedtls_ssl_write_certificate( mbedtls_ssl_context *ssl ) if( !mbedtls_ssl_ciphersuite_uses_srv_cert( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate" ) ); - ssl->state++; + if( ssl->state == MBEDTLS_SSL_CLIENT_CERTIFICATE ) + { + ssl->state = MBEDTLS_SSL_CLIENT_KEY_EXCHANGE; + } + else if( ssl->state == MBEDTLS_SSL_SERVER_CERTIFICATE ) + { + ssl->state = MBEDTLS_SSL_SERVER_KEY_EXCHANGE; + } return( 0 ); } @@ -6793,7 +6814,14 @@ int mbedtls_ssl_write_certificate( mbedtls_ssl_context *ssl ) if( ssl->client_auth == 0 ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate" ) ); - ssl->state++; + if( ssl->state == MBEDTLS_SSL_CLIENT_CERTIFICATE ) + { + ssl->state = MBEDTLS_SSL_CLIENT_KEY_EXCHANGE; + } + else if( ssl->state == MBEDTLS_SSL_SERVER_CERTIFICATE ) + { + ssl->state = MBEDTLS_SSL_SERVER_KEY_EXCHANGE; + } return( 0 ); } @@ -6867,7 +6895,14 @@ int mbedtls_ssl_write_certificate( mbedtls_ssl_context *ssl ) write_msg: #endif - ssl->state++; + if( ssl->state == MBEDTLS_SSL_CLIENT_CERTIFICATE ) + { + ssl->state = MBEDTLS_SSL_CLIENT_KEY_EXCHANGE; + } + else if( ssl->state == MBEDTLS_SSL_SERVER_CERTIFICATE ) + { + ssl->state = MBEDTLS_SSL_SERVER_KEY_EXCHANGE; + } if( ( ret = mbedtls_ssl_write_handshake_msg( ssl ) ) != 0 ) { @@ -7523,7 +7558,16 @@ crt_verify: exit: if( ret == 0 ) - ssl->state++; + { + if( ssl->state == MBEDTLS_SSL_CLIENT_CERTIFICATE ) + { + ssl->state = MBEDTLS_SSL_CLIENT_KEY_EXCHANGE; + } + else if( ssl->state == MBEDTLS_SSL_SERVER_CERTIFICATE ) + { + ssl->state = MBEDTLS_SSL_SERVER_KEY_EXCHANGE; + } + } #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) if( ret == MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS ) @@ -7553,7 +7597,14 @@ int mbedtls_ssl_write_change_cipher_spec( mbedtls_ssl_context *ssl ) ssl->out_msglen = 1; ssl->out_msg[0] = 1; - ssl->state++; + if( ssl->state == MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC ) + { + ssl->state = MBEDTLS_SSL_CLIENT_FINISHED; + } + else if( ssl->state == MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC ) + { + ssl->state = MBEDTLS_SSL_SERVER_FINISHED; + } if( ( ret = mbedtls_ssl_write_handshake_msg( ssl ) ) != 0 ) { @@ -7636,7 +7687,14 @@ int mbedtls_ssl_parse_change_cipher_spec( mbedtls_ssl_context *ssl ) } #endif - ssl->state++; + if( ssl->state == MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC ) + { + ssl->state = MBEDTLS_SSL_CLIENT_FINISHED; + } + else if( ssl->state == MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC ) + { + ssl->state = MBEDTLS_SSL_SERVER_FINISHED; + } MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse change cipher spec" ) ); @@ -7742,7 +7800,7 @@ void mbedtls_ssl_handshake_wrapup( mbedtls_ssl_context *ssl ) #endif ssl_handshake_wrapup_free_hs_transform( ssl ); - ssl->state++; + ssl->state = MBEDTLS_SSL_HANDSHAKE_OVER; MBEDTLS_SSL_DEBUG_MSG( 3, ( "<= handshake wrapup" ) ); } @@ -7804,7 +7862,16 @@ int mbedtls_ssl_write_finished( mbedtls_ssl_context *ssl ) } else #endif /* !MBEDTLS_SSL_NO_SESSION_RESUMPTION */ - ssl->state++; + { + if( ssl->state == MBEDTLS_SSL_CLIENT_FINISHED ) + { + ssl->state = MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC; + } + else if( ssl->state == MBEDTLS_SSL_SERVER_FINISHED ) + { + ssl->state = MBEDTLS_SSL_FLUSH_BUFFERS; + } + } /* * Switch to our negotiated transform and session parameters for outbound @@ -7964,7 +8031,16 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) } else #endif /* !MBEDTLS_SSL_NO_SESSION_RESUMPTION */ - ssl->state++; + { + if( ssl->state == MBEDTLS_SSL_CLIENT_FINISHED ) + { + ssl->state = MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC; + } + else if( ssl->state == MBEDTLS_SSL_SERVER_FINISHED ) + { + ssl->state = MBEDTLS_SSL_FLUSH_BUFFERS; + } + } #if defined(MBEDTLS_SSL_PROTO_DTLS) if( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) )