From 3be264e2c33a7de193ede8b7987f8609093c870a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 9 Jul 2019 17:27:32 +0100 Subject: [PATCH 01/40] Remove redundant length-0 checks for incoming unprotected records --- library/ssl_tls.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 0386ea0e8..5f81939c4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5027,8 +5027,7 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) /* Check length against bounds of the current transform and version */ if( ssl->transform_in == NULL ) { - if( ssl->in_msglen < 1 || - ssl->in_msglen > MBEDTLS_SSL_IN_CONTENT_LEN ) + if( ssl->in_msglen > MBEDTLS_SSL_IN_CONTENT_LEN ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad message length" ) ); return( MBEDTLS_ERR_SSL_INVALID_RECORD ); From d96e10bf234d0338393ab9bae26c80d496aa00a6 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 9 Jul 2019 17:30:02 +0100 Subject: [PATCH 02/40] Check architectural bound for max record payload len in one place The previous code performed architectural maximum record length checks both before and after record decryption. Since MBEDTLS_SSL_IN_CONTENT_LEN bounds the maximum length of the record plaintext, it suffices to check only once after (potential) decryption. This must not be confused with the internal check that the record length is small enough to make the record fit into the internal input buffer; this is done in mbedtls_ssl_fetch_input(). --- library/ssl_tls.c | 47 ++++++++++------------------------------------- 1 file changed, 10 insertions(+), 37 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5f81939c4..9682bbbb1 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5025,43 +5025,13 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) /* Check length against bounds of the current transform and version */ - if( ssl->transform_in == NULL ) - { - if( ssl->in_msglen > MBEDTLS_SSL_IN_CONTENT_LEN ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad message length" ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); - } - } - else + if( ssl->transform_in != NULL ) { if( ssl->in_msglen < ssl->transform_in->minlen ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad message length" ) ); return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } - -#if defined(MBEDTLS_SSL_PROTO_SSL3) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 && - ssl->in_msglen > ssl->transform_in->minlen + MBEDTLS_SSL_IN_CONTENT_LEN ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad message length" ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); - } -#endif -#if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_2) - /* - * TLS encrypted messages can have up to 256 bytes of padding - */ - if( ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_1 && - ssl->in_msglen > ssl->transform_in->minlen + - MBEDTLS_SSL_IN_CONTENT_LEN + 256 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad message length" ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); - } -#endif } return( 0 ); @@ -5166,12 +5136,7 @@ static int ssl_prepare_record_content( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ - if( ssl->in_msglen > MBEDTLS_SSL_IN_CONTENT_LEN ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad message length" ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); - } - else if( ssl->in_msglen == 0 ) + if( ssl->in_msglen == 0 ) { #if defined(MBEDTLS_SSL_PROTO_TLS1_2) if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 @@ -5244,6 +5209,14 @@ static int ssl_prepare_record_content( mbedtls_ssl_context *ssl ) } #endif + /* Check actual (decrypted) record content length against + * configured maximum. */ + if( ssl->in_msglen > MBEDTLS_SSL_IN_CONTENT_LEN ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad message length" ) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + } + return( 0 ); } From 47ebaa2205ce8b6b860fdfe48957df7d48d92ef3 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 09:45:44 +0100 Subject: [PATCH 03/40] Remove assertion in mbedtls_ssl_decrypt_buf() mbedtls_ssl_decrypt_buf() asserts that the passed transform is not NULL, but the function is only invoked in a single place, and this invocation is clearly visible to be within a branch ensuring that the incoming transform isn't NULL. Remove the assertion for the benefit of code-size. --- library/ssl_tls.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 9682bbbb1..decb0b482 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2572,11 +2572,6 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, #endif MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> decrypt buf" ) ); - if( transform == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "no transform provided to decrypt_buf" ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); - } if( rec == NULL || rec->buf == NULL || rec->buf_len < rec->data_offset || From e2b786d40fcc17cf6231d177609b122894f26398 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 09:49:56 +0100 Subject: [PATCH 04/40] Remove misleading comment in mbedtls_ssl_decrypt_buf() The comment doesn't seem to relate to the code that follows. --- library/ssl_tls.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index decb0b482..ba8ba1997 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2627,9 +2627,6 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, unsigned char iv[12]; size_t explicit_iv_len = transform->ivlen - transform->fixed_ivlen; - /* - * Compute and update sizes - */ if( rec->data_len < explicit_iv_len + transform->taglen ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "msglen (%d) < explicit_iv_len (%d) " From c957e3b5f882214e8c81cdd28e0fd23b7e238901 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 11:37:19 +0100 Subject: [PATCH 05/40] Remove redundant length check during record header parsing The check is in terms of the internal input buffer length and is hence likely to be originally intended to protect against overflow of the input buffer when fetching data from the underlying transport in mbedtls_ssl_fetch_input(). For locality of reasoning, it's better to perform such a check close to where it's needed, and in fact, mbedtls_ssl_fetch_input() _does_ contain an equivalent bounds check, too, rendering the bounds check in question redundant. --- library/ssl_tls.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index ba8ba1997..51dc603b9 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4935,13 +4935,6 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) * the presence of a CID. */ ssl->in_msglen = ( ssl->in_len[0] << 8 ) | ssl->in_len[1]; - if( ssl->in_msglen > MBEDTLS_SSL_IN_BUFFER_LEN - - (size_t)( ssl->in_msg - ssl->in_buf ) ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad message length" ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); - } - MBEDTLS_SSL_DEBUG_MSG( 3, ( "input record: msgtype = %d, " "version = [%d:%d], msglen = %d", ssl->in_msgtype, From d96a652d8048b18c5f564e7841b298104d206354 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 13:55:25 +0100 Subject: [PATCH 06/40] Improve documentation of mbedtls_ssl_decrypt_buf() --- library/ssl_tls.c | 71 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 17 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 51dc603b9..0bfca92b5 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2627,6 +2627,13 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, unsigned char iv[12]; size_t explicit_iv_len = transform->ivlen - transform->fixed_ivlen; + /* + * Prepare IV from explicit and implicit data. + */ + + /* Check that there's enough space for the explicit IV + * (at the beginning of the record) and the MAC (at the + * end of the record). */ if( rec->data_len < explicit_iv_len + transform->taglen ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "msglen (%d) < explicit_iv_len (%d) " @@ -2635,17 +2642,20 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_INVALID_MAC ); } - /* - * Prepare IV - */ +#if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CCM_C) if( transform->ivlen == 12 && transform->fixed_ivlen == 4 ) { - /* GCM and CCM: fixed || explicit (transmitted) */ - memcpy( iv, transform->iv_dec, transform->fixed_ivlen ); - memcpy( iv + transform->fixed_ivlen, data, 8 ); + /* GCM and CCM: fixed || explicit */ + /* Fixed */ + memcpy( iv, transform->iv_dec, transform->fixed_ivlen ); + /* Explicit */ + memcpy( iv + transform->fixed_ivlen, data, 8 ); } - else if( transform->ivlen == 12 && transform->fixed_ivlen == 12 ) + else +#endif /* MBEDTLS_GCM_C || MBEDTLS_CCM_C */ +#if defined(MBEDTLS_CHACHAPOLY_C) + if( transform->ivlen == 12 && transform->fixed_ivlen == 12 ) { /* ChachaPoly: fixed XOR sequence number */ unsigned char i; @@ -2656,12 +2666,15 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, iv[i+4] ^= rec->ctr[i]; } else +#endif /* MBEDTLS_CHACHAPOLY_C */ { /* Reminder if we ever add an AEAD mode with a different size */ MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } + /* Group changes to data, data_len, and add_data, because + * add_data depends on data_len. */ data += explicit_iv_len; rec->data_offset += explicit_iv_len; rec->data_len -= explicit_iv_len + transform->taglen; @@ -2670,6 +2683,12 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_BUF( 4, "additional data used for AEAD", add_data, add_data_len ); + /* Because of the check above, we know that there are + * explicit_iv_len Bytes preceeding data, and taglen + * bytes following data + data_len. This justifies + * the memcpy, debug message and invocation of + * mbedtls_cipher_auth_decrypt() below. */ + memcpy( transform->iv_dec + transform->fixed_ivlen, data - explicit_iv_len, explicit_iv_len ); @@ -2677,7 +2696,6 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_BUF( 4, "TAG used", data + rec->data_len, transform->taglen ); - /* * Decrypt and authenticate */ @@ -2698,6 +2716,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, } auth_done++; + /* Double-check that AEAD decryption doesn't change content length. */ if( olen != rec->data_len ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); @@ -2764,11 +2783,20 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 3, ( "using encrypt then mac" ) ); - /* Safe due to the check data_len >= minlen + maclen + 1 above. */ + /* Update data_len in tandem with add_data. + * + * The subtraction is safe because of the previous check + * data_len >= minlen + maclen + 1. + * + * Afterwards, we know that data + data_len is followed by at + * least maclen Bytes, which justifies the call to + * mbedtls_ssl_safer_memcmp() below. + * + * Further, we still know that data_len > minlen */ rec->data_len -= transform->maclen; - ssl_extract_add_data_from_record( add_data, &add_data_len, rec ); + /* Calculate expected MAC. */ MBEDTLS_SSL_DEBUG_BUF( 4, "MAC'd meta-data", add_data, add_data_len ); mbedtls_md_hmac_update( &transform->md_ctx_dec, add_data, @@ -2783,6 +2811,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_BUF( 4, "expected mac", mac_expect, transform->maclen ); + /* Compare expected MAC with MAC at the end of the record. */ if( mbedtls_ssl_safer_memcmp( data + rec->data_len, mac_expect, transform->maclen ) != 0 ) { @@ -2796,6 +2825,10 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, /* * Check length sanity */ + + /* We know from above that data_len > minlen >= 0, + * so the following check in particular implies that + * data_len >= minlen + ivlen ( = minlen or 2 * minlen ). */ if( rec->data_len % transform->ivlen != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "msglen (%d) %% ivlen (%d) != 0", @@ -2809,9 +2842,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, */ if( transform->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_2 ) { - /* This is safe because data_len >= minlen + maclen + 1 initially, - * and at this point we have at most subtracted maclen (note that - * minlen == transform->ivlen here). */ + /* Safe because data_len >= minlen + ivlen = 2 * ivlen. */ memcpy( transform->iv_dec, data, transform->ivlen ); data += transform->ivlen; @@ -2820,6 +2851,8 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, } #endif /* MBEDTLS_SSL_PROTO_TLS1_1 || MBEDTLS_SSL_PROTO_TLS1_2 */ + /* We still have data_len % ivlen == 0 and data_len >= ivlen here. */ + if( ( ret = mbedtls_cipher_crypt( &transform->cipher_ctx_dec, transform->iv_dec, transform->ivlen, data, rec->data_len, data, &olen ) ) != 0 ) @@ -2828,6 +2861,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, return( ret ); } + /* Double-check that length hasn't changed during decryption. */ if( rec->data_len != olen ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); @@ -2838,7 +2872,10 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, if( transform->minor_ver < MBEDTLS_SSL_MINOR_VERSION_2 ) { /* - * Save IV in SSL3 and TLS1 + * Save IV in SSL3 and TLS1, where CBC decryption of consecutive + * records is equivalent to CBC decryption of the concatenation + * of the records; in other words, IVs are maintained across + * record decryptions. */ memcpy( transform->iv_dec, transform->cipher_ctx_dec.iv, transform->ivlen ); @@ -2847,7 +2884,8 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, /* Safe since data_len >= minlen + maclen + 1, so after having * subtracted at most minlen and maclen up to this point, - * data_len > 0. */ + * data_len > 0 (because of data_len % ivlen == 0, it's actually + * >= ivlen ). */ padlen = data[rec->data_len - 1]; if( auth_done == 1 ) @@ -2975,7 +3013,6 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, * hence data_len >= maclen in any case. */ rec->data_len -= transform->maclen; - ssl_extract_add_data_from_record( add_data, &add_data_len, rec ); #if defined(MBEDTLS_SSL_PROTO_SSL3) @@ -3028,7 +3065,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, * in_msglen over all padlen values. * * They're independent of padlen, since we previously did - * in_msglen -= padlen. + * data_len -= padlen. * * Note that max_len + maclen is never more than the buffer * length, as we previously did in_msglen -= maclen too. From 20016654c36f73554a8b584fdcf4288f2a553e8e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 11:44:13 +0100 Subject: [PATCH 07/40] Remove unnecessary backup of explicit IV in AEAD record decryption There is no need to hold back the explicit IV for AEAD ciphers. --- library/ssl_tls.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 0bfca92b5..9431212de 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2686,12 +2686,9 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, /* Because of the check above, we know that there are * explicit_iv_len Bytes preceeding data, and taglen * bytes following data + data_len. This justifies - * the memcpy, debug message and invocation of + * the debug message and the invocation of * mbedtls_cipher_auth_decrypt() below. */ - memcpy( transform->iv_dec + transform->fixed_ivlen, - data - explicit_iv_len, explicit_iv_len ); - MBEDTLS_SSL_DEBUG_BUF( 4, "IV used", iv, transform->ivlen ); MBEDTLS_SSL_DEBUG_BUF( 4, "TAG used", data + rec->data_len, transform->taglen ); From 4894873b92eb169a59134fbd4a5a2e3a246d9b62 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 13:55:17 +0100 Subject: [PATCH 08/40] Remove redundant minimum length check Availability of sufficient incoming data should be checked when it is needed, which is in mbedtls_ssl_fetch_input(), and this function has the necessary bounds checks in place. --- library/ssl_tls.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 9431212de..e5881da74 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5042,17 +5042,6 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_PROTO_DTLS */ - - /* Check length against bounds of the current transform and version */ - if( ssl->transform_in != NULL ) - { - if( ssl->in_msglen < ssl->transform_in->minlen ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad message length" ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); - } - } - return( 0 ); } From 2fddd3765ea998bb9f40b52dc1baaf843b9889bf Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 14:37:41 +0100 Subject: [PATCH 09/40] Check same-port-reconnect from client outside of record hdr parsing Previously, `ssl_handle_possible_reconnect()` was part of `ssl_parse_record_header()`, which was required to return a non-zero error code to indicate a record which should not be further processed because it was invalid, unexpected, duplicate, .... In this case, some error codes would lead to some actions to be taken, e.g. `MBEDTLS_ERR_SSL_EARLY_MESSAGE` to potential buffering of the record, but eventually, the record would be dropped regardless of the precise value of the error code. The error code `MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED` returned from `ssl_handle_possible_reconnect()` did not receive any special treatment and lead to silent dopping of the record - in particular, it was never returned to the user. In the new logic this commit introduces, `ssl_handle_possible_reconnect()` is part of `ssl_check_client_reconnect()` which is triggered _after_ `ssl_parse_record_header()` found an unexpected record, which is already in the code-path eventually dropping the record; we want to leave this code-path only if a valid cookie has been found and we want to reset, but do nothing otherwise. That's why `ssl_handle_possible_reconnect()` now returns `0` unless a valid cookie has been found or a fatal error occurred. --- library/ssl_tls.c | 101 ++++++++++++++++++++++++++-------------------- 1 file changed, 57 insertions(+), 44 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e5881da74..204fa43e4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4691,9 +4691,6 @@ static int ssl_check_dtls_clihlo_cookie( size_t sid_len, cookie_len; unsigned char *p; - if( f_cookie_write == NULL || f_cookie_check == NULL ) - return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - /* * Structure of ClientHello with record and handshake headers, * and expected values. We don't need to check a lot, more checks will be @@ -4819,6 +4816,14 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) int ret; size_t len; + if( ssl->conf->f_cookie_write == NULL || + ssl->conf->f_cookie_check == NULL ) + { + /* If we can't use cookies to verify reachability of the peer, + * drop the record. */ + return( 0 ); + } + ret = ssl_check_dtls_clihlo_cookie( ssl->conf->f_cookie_write, ssl->conf->f_cookie_check, @@ -4835,8 +4840,7 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) * If the error is permanent we'll catch it later, * if it's not, then hopefully it'll work next time. */ (void) ssl->f_send( ssl->p_bio, ssl->out_buf, len ); - - return( MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED ); + ret = 0; } if( ret == 0 ) @@ -4991,49 +4995,22 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) { unsigned int rec_epoch = ( ssl->in_ctr[0] << 8 ) | ssl->in_ctr[1]; - /* Check epoch (and sequence number) with DTLS */ - if( rec_epoch != ssl->in_epoch ) + if( rec_epoch == (unsigned) ssl->in_epoch + 1 ) + { + /* Consider buffering the record. */ + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Consider record for buffering" ) ); + return( MBEDTLS_ERR_SSL_EARLY_MESSAGE ); + } + else if( rec_epoch != ssl->in_epoch ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "record from another epoch: " "expected %d, received %d", ssl->in_epoch, rec_epoch ) ); - -#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && defined(MBEDTLS_SSL_SRV_C) - /* - * Check for an epoch 0 ClientHello. We can't use in_msg here to - * access the first byte of record content (handshake type), as we - * have an active transform (possibly iv_len != 0), so use the - * fact that the record header len is 13 instead. - */ - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && - ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER && - rec_epoch == 0 && - ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && - ssl->in_left > 13 && - ssl->in_buf[13] == MBEDTLS_SSL_HS_CLIENT_HELLO ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "possible client reconnect " - "from the same port" ) ); - return( ssl_handle_possible_reconnect( ssl ) ); - } - else -#endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE && MBEDTLS_SSL_SRV_C */ - { - /* Consider buffering the record. */ - if( rec_epoch == (unsigned int) ssl->in_epoch + 1 ) - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "Consider record for buffering" ) ); - return( MBEDTLS_ERR_SSL_EARLY_MESSAGE ); - } - - return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ); - } + return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ); } - #if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY) /* Replay detection only works for the current epoch */ - if( rec_epoch == ssl->in_epoch && - mbedtls_ssl_dtls_replay_check( ssl ) != 0 ) + else if( mbedtls_ssl_dtls_replay_check( ssl ) != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "replayed record" ) ); return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ); @@ -5045,6 +5022,34 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) return( 0 ); } + +#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && defined(MBEDTLS_SSL_SRV_C) +static int ssl_check_client_reconnect( mbedtls_ssl_context *ssl ) +{ + unsigned int rec_epoch = ( ssl->in_ctr[0] << 8 ) | ssl->in_ctr[1]; + + /* + * Check for an epoch 0 ClientHello. We can't use in_msg here to + * access the first byte of record content (handshake type), as we + * have an active transform (possibly iv_len != 0), so use the + * fact that the record header len is 13 instead. + */ + if( rec_epoch == 0 && + ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && + ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER && + ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && + ssl->in_left > 13 && + ssl->in_buf[13] == MBEDTLS_SSL_HS_CLIENT_HELLO ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "possible client reconnect " + "from the same port" ) ); + return( ssl_handle_possible_reconnect( ssl ) ); + } + + return( 0 ); +} +#endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE && MBEDTLS_SSL_SRV_C */ + /* * If applicable, decrypt (and decompress) record content */ @@ -5926,8 +5931,7 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) if( ( ret = ssl_parse_record_header( ssl ) ) != 0 ) { #if defined(MBEDTLS_SSL_PROTO_DTLS) - if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && - ret != MBEDTLS_ERR_SSL_CLIENT_RECONNECT ) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) { if( ret == MBEDTLS_ERR_SSL_EARLY_MESSAGE ) { @@ -5941,6 +5945,12 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) if( ret == MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ) { +#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && defined(MBEDTLS_SSL_SRV_C) + ret = ssl_check_client_reconnect( ssl ); + if( ret != 0 ) + return( ret ); +#endif + /* Skip unexpected record (but not whole datagram) */ ssl->next_record_offset = ssl->in_msglen + mbedtls_ssl_in_hdr_len( ssl ); @@ -5961,8 +5971,11 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) /* Get next record */ return( MBEDTLS_ERR_SSL_CONTINUE_PROCESSING ); } + else #endif - return( ret ); + { + return( ret ); + } } /* From e538d8287e31e70e20002c3a3256a0994b236f18 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 14:50:10 +0100 Subject: [PATCH 10/40] Move size-check for DTLS record header with CID to DTLS-only branch --- library/ssl_tls.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 204fa43e4..14a5a49ee 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4924,6 +4924,18 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) * fixed in the configuration. */ ssl->in_len = ssl->in_cid + ssl->conf->cid_len; ssl->in_iv = ssl->in_msg = ssl->in_len + 2; + + /* Now that the total length of the record header is known, ensure + * that the current datagram is large enough to hold it. + * This would fail, for example, if we received a datagram of + * size 13 + n Bytes where n is less than the size of incoming CIDs. + */ + ret = mbedtls_ssl_fetch_input( ssl, mbedtls_ssl_in_hdr_len( ssl ) ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_fetch_input", ret ); + return( ret ); + } } else #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ @@ -4955,16 +4967,6 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } - /* Now that the total length of the record header is known, ensure - * that the current datagram is large enough to hold it. - * This would fail, for example, if we received a datagram of - * size 13 + n Bytes where n is less than the size of incoming CIDs. */ - ret = mbedtls_ssl_fetch_input( ssl, mbedtls_ssl_in_hdr_len( ssl ) ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_fetch_input", ret ); - return( ret ); - } MBEDTLS_SSL_DEBUG_BUF( 4, "input record header", ssl->in_hdr, mbedtls_ssl_in_hdr_len( ssl ) ); /* Parse and validate record length From 59be60e98b6732ca4bdafdf7b3ce553e90832f72 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 14:53:43 +0100 Subject: [PATCH 11/40] Don't call ssl_fetch_input for record hdr size check in DTLS In DTLS, if mbedtls_ssl_fetch_input() is called multiple times without resetting the input buffer in between, the non-initial calls are functionally equivalent to mere bounds checks ensuring that the incoming datagram is large enough to hold the requested data. In the interest of code-size and modularity (removing a call to a non-const function which is logically const in this instance), this commit replaces such a call to mbedtls_ssl_fetch_input() by an explicit bounds check in ssl_parse_record_header(). --- library/ssl_tls.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 14a5a49ee..bae772a20 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4894,7 +4894,6 @@ static int ssl_check_record_type( uint8_t record_type ) static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) { int major_ver, minor_ver; - int ret; /* Parse and validate record content type and version */ @@ -4930,11 +4929,11 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) * This would fail, for example, if we received a datagram of * size 13 + n Bytes where n is less than the size of incoming CIDs. */ - ret = mbedtls_ssl_fetch_input( ssl, mbedtls_ssl_in_hdr_len( ssl ) ); - if( ret != 0 ) + if( ssl->in_left < mbedtls_ssl_in_hdr_len( ssl ) ) { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_fetch_input", ret ); - return( ret ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "datagram too short to contain DTLS record header including CID of length %u.", + (unsigned) mbedtls_ssl_conf_get_cid_len( ssl->conf ) ) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } } else From a8814794e9e28047972ce49ee615440af6127b40 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 15:01:45 +0100 Subject: [PATCH 12/40] Don't call ssl_fetch_input for record content fetch in DTLS As explained in the previous commit, if mbedtls_ssl_fetch_input() is called multiple times, all but the first call are equivalent to bounds checks in the incoming datagram. --- library/ssl_tls.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index bae772a20..4c7f9d019 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5980,19 +5980,21 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) } /* - * Read and optionally decrypt the message contents + * Make sure the entire record contents are available. + * + * In TLS, this means fetching them from the underlying transport. + * In DTLS, it means checking that the incoming datagram is large enough. */ - if( ( ret = mbedtls_ssl_fetch_input( ssl, - mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_fetch_input", ret ); - return( ret ); - } - - /* Done reading this record, get ready for the next one */ #if defined(MBEDTLS_SSL_PROTO_DTLS) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) { + if( ssl->in_left < mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Datagram too small to contain record." ) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + } + + /* Remember offset of next record within datagram. */ ssl->next_record_offset = ssl->in_msglen + mbedtls_ssl_in_hdr_len( ssl ); if( ssl->next_record_offset < ssl->in_left ) { @@ -6001,7 +6003,21 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) } else #endif + { + ret = mbedtls_ssl_fetch_input( ssl, + mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_fetch_input", ret ); + return( ret ); + } + ssl->in_left = 0; + } + + /* + * Decrypt record contents. + */ if( ( ret = ssl_prepare_record_content( ssl ) ) != 0 ) { From d5c0f826e62c4bfa30820a81c6f5298833dbf187 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 16:53:30 +0100 Subject: [PATCH 13/40] Don't send an alert when receiving a record of unknown ContentType We don't send alerts on other instances of ill-formed records, so why should we do it here? If we want to keep it, the alerts should rather be sent ssl_get_next_record(). --- library/ssl_tls.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 4c7f9d019..4d6fc9583 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4941,15 +4941,6 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) if( ssl_check_record_type( ssl->in_msgtype ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "unknown record type" ) ); - -#if defined(MBEDTLS_SSL_PROTO_DTLS) - /* Silently ignore invalid DTLS records as recommended by RFC 6347 - * Section 4.1.2.7 */ - if( ssl->conf->transport != MBEDTLS_SSL_TRANSPORT_DATAGRAM ) -#endif /* MBEDTLS_SSL_PROTO_DTLS */ - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } From 955a5c98df8d9067afda00eded8003a012ee295e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 17:12:07 +0100 Subject: [PATCH 14/40] Check for sufficient datagram size in ssl_parse_record_header() Previously, ssl_parse_record_header() did not check whether the current datagram is large enough to hold a record of the advertised size. This could lead to records being silently skipped over or backed up on the basis of an invalid record length. Concretely, the following would happen: 1) In the case of a record from an old epoch, the record would be 'skipped over' by setting next_record_offset according to the advertised but non-validated length, and only in the subsequent mbedtls_ssl_fetch_input() it would be noticed in an assertion failure if the record length is too large for the current incoming datagram. While not critical, this is fragile, and also contrary to the intend that MBEDTLS_ERR_SSL_INTERNAL_ERROR should never be trigger-able by external input. 2) In the case of a future record being buffered, it might be that we backup a record before we have validated its length, hence copying parts of the input buffer that don't belong to the current record. This is a bug, and it's by luck that it doesn't seem to have critical consequences. This commit fixes this by modifying ssl_parse_record_header() to check that the current incoming datagram is large enough to hold a record of the advertised length, returning MBEDTLS_ERR_SSL_INVALID_RECORD otherwise. --- library/ssl_tls.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 4d6fc9583..135caa0ca 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4987,6 +4987,13 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) { unsigned int rec_epoch = ( ssl->in_ctr[0] << 8 ) | ssl->in_ctr[1]; + /* Check that the datagram is large enough to contain a record + * of the advertised length. */ + if( ssl->in_left < mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Datagram too small to contain record." ) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + } if( rec_epoch == (unsigned) ssl->in_epoch + 1 ) { /* Consider buffering the record. */ @@ -5970,21 +5977,9 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) } } - /* - * Make sure the entire record contents are available. - * - * In TLS, this means fetching them from the underlying transport. - * In DTLS, it means checking that the incoming datagram is large enough. - */ #if defined(MBEDTLS_SSL_PROTO_DTLS) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) { - if( ssl->in_left < mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Datagram too small to contain record." ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); - } - /* Remember offset of next record within datagram. */ ssl->next_record_offset = ssl->in_msglen + mbedtls_ssl_in_hdr_len( ssl ); if( ssl->next_record_offset < ssl->in_left ) @@ -5995,6 +5990,9 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) else #endif { + /* + * Fetch record contents from underlying transport. + */ ret = mbedtls_ssl_fetch_input( ssl, mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen ); if( ret != 0 ) From 37cfe73c92ab330c2bbee208567c81a302627ca7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 17:20:01 +0100 Subject: [PATCH 15/40] Minor documentation improvements in ssl_parse_record_header() --- library/ssl_tls.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 135caa0ca..69abb6ab1 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4994,12 +4994,17 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 1, ( "Datagram too small to contain record." ) ); return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } + + /* Records from the next epoch are considered for buffering + * (concretely: early Finished messages). */ if( rec_epoch == (unsigned) ssl->in_epoch + 1 ) { - /* Consider buffering the record. */ MBEDTLS_SSL_DEBUG_MSG( 2, ( "Consider record for buffering" ) ); return( MBEDTLS_ERR_SSL_EARLY_MESSAGE ); } + /* Records from other, non-matching epochs are silently discarded. + * (The case of same-port Client reconnects must be considered in + * the caller). */ else if( rec_epoch != ssl->in_epoch ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "record from another epoch: " @@ -5008,7 +5013,8 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ); } #if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY) - /* Replay detection only works for the current epoch */ + /* For records from the correct epoch, check whether their + * sequence number has been seen before. */ else if( mbedtls_ssl_dtls_replay_check( ssl ) != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "replayed record" ) ); From d840cea4a169080e2d18d93e619f2992e35da2b5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 09:24:36 +0100 Subject: [PATCH 16/40] Expand documentation of internal mbedtls_record structure --- include/mbedtls/ssl_internal.h | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index c2bc3b787..4028abe2b 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -672,18 +672,29 @@ struct mbedtls_ssl_transform typedef struct { - uint8_t ctr[8]; /* Record sequence number */ - uint8_t type; /* Record type */ - uint8_t ver[2]; /* SSL/TLS version */ + uint8_t ctr[8]; /* In TLS: The implicit record sequence number. + * In DTLS: The 2-byte epoch followed by + * the 6-byte sequence number. + * This is stored as a raw big endian byte array + * as opposed to a uint64_t because we rarely + * need to perform arithmetic on this, but do + * need it as a Byte array for the purpose of + * MAC computations. */ + uint8_t type; /* The record content type. */ + uint8_t ver[2]; /* SSL/TLS version as present on the wire. + * Convert to internal presentation of versions + * using mbedtls_ssl_read_version() and + * mbedtls_ssl_write_version(). + * Keep wire-format for MAC computations. */ - unsigned char *buf; /* Memory buffer enclosing the record content */ - size_t buf_len; /* Buffer length */ - size_t data_offset; /* Offset of record content */ - size_t data_len; /* Length of record content */ + unsigned char *buf; /* Memory buffer enclosing the record content */ + size_t buf_len; /* Buffer length */ + size_t data_offset; /* Offset of record content */ + size_t data_len; /* Length of record content */ #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) - uint8_t cid_len; /* Length of the CID (0 if not present) */ - unsigned char cid[ MBEDTLS_SSL_CID_LEN_MAX ]; /* The CID */ + uint8_t cid_len; /* Length of the CID (0 if not present) */ + unsigned char cid[ MBEDTLS_SSL_CID_LEN_MAX ]; /* The CID */ #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ } mbedtls_record; From e5e7e7833cd34516deb3fc5145270e78af0dd769 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 12:29:35 +0100 Subject: [PATCH 17/40] Setup SSL record structure in ssl_parse_record_header() This commit makes a first step towards modularizing the incoming record processing by having it operate on instances of the structure mbedtls_record representing SSL records. So far, only record encryption/decryption operate in terms of record instances, but the rest of the parsing doesn't. In particular, ssl_parse_record_header() operates directly on the fixed input buffer, setting the various ssl->in_xxx pointers and fields, and only directly before/after calling ssl_decrypt_buf() these fields a converted to/from mbedtls_record instances. This commit does not yet remove the ssl->in_xxx fields, but makes a step towards extending the lifetime of mbedtls_record structure representing incoming records, by modifying ssl_parse_record_header() to setup an instance of mbedtls_record, and setting the ssl->in_xxx fields from that instance. The instance so-constructed isn't used further so far, and in particular it is not yet consolidated with the instance set up for use in ssl_decrypt_record(). That's for a later commit. --- library/ssl_tls.c | 164 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 131 insertions(+), 33 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 69abb6ab1..d91b05d37 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4891,20 +4891,73 @@ static int ssl_check_record_type( uint8_t record_type ) * Point 2 is needed when the peer is resending, and we have already received * the first record from a datagram but are still waiting for the others. */ -static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) +static int ssl_parse_record_header( mbedtls_ssl_context *ssl, + unsigned char *buf, + size_t len, + mbedtls_record *rec ) { int major_ver, minor_ver; - /* Parse and validate record content type and version */ + size_t const rec_hdr_type_offset = 0; + size_t const rec_hdr_type_len = 1; - ssl->in_msgtype = ssl->in_hdr[0]; - mbedtls_ssl_read_version( &major_ver, &minor_ver, ssl->conf->transport, ssl->in_hdr + 1 ); + size_t const rec_hdr_version_offset = rec_hdr_type_offset + + rec_hdr_type_len; + size_t const rec_hdr_version_len = 2; + + size_t const rec_hdr_ctr_len = 8; +#if defined(MBEDTLS_SSL_PROTO_DTLS) + uint32_t rec_epoch; + size_t const rec_hdr_ctr_offset = rec_hdr_version_offset + + rec_hdr_version_len; - /* Check record type */ #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) + size_t const rec_hdr_cid_offset = rec_hdr_ctr_offset + + rec_hdr_ctr_len; + size_t rec_hdr_cid_len = 0; +#endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + + size_t rec_hdr_len_offset; /* To be determined */ + size_t const rec_hdr_len_len = 2; + + /* + * Check minimum lengths for record header. + */ + +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + { + rec_hdr_len_offset = rec_hdr_ctr_offset + rec_hdr_ctr_len; + } + else +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + { + rec_hdr_len_offset = rec_hdr_version_offset + rec_hdr_version_len; + } + + if( len < rec_hdr_len_offset + rec_hdr_len_len ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "datagram of length %u too small to hold DTLS record header of length %u", + (unsigned) len, + (unsigned)( rec_hdr_len_len + rec_hdr_len_len ) ) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + } + + /* + * Parse and validate record content type + */ + + rec->type = buf[ rec_hdr_type_offset ]; + ssl->in_msgtype = rec->type; + + /* Check record content type */ +#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) + rec->cid_len = 0; + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && - ssl->in_msgtype == MBEDTLS_SSL_MSG_CID && - ssl->conf->cid_len != 0 ) + ssl->conf->cid_len != 0 && + rec->type == MBEDTLS_SSL_MSG_CID ) { /* Shift pointers to account for record header including CID * struct { @@ -4921,30 +4974,45 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) /* So far, we only support static CID lengths * fixed in the configuration. */ - ssl->in_len = ssl->in_cid + ssl->conf->cid_len; - ssl->in_iv = ssl->in_msg = ssl->in_len + 2; + rec_hdr_cid_len = ssl->conf->cid_len; + rec_hdr_len_offset += rec_hdr_cid_len; - /* Now that the total length of the record header is known, ensure - * that the current datagram is large enough to hold it. - * This would fail, for example, if we received a datagram of - * size 13 + n Bytes where n is less than the size of incoming CIDs. - */ - if( ssl->in_left < mbedtls_ssl_in_hdr_len( ssl ) ) + if( len < rec_hdr_len_offset + rec_hdr_len_len ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "datagram too short to contain DTLS record header including CID of length %u.", - (unsigned) mbedtls_ssl_conf_get_cid_len( ssl->conf ) ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "datagram of length %u too small to hold DTLS record header including CID, length %u", + (unsigned) len, + (unsigned)( rec_hdr_len_offset + rec_hdr_len_len ) ) ); return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } + + rec->cid_len = rec_hdr_cid_len; + memcpy( rec->cid, buf + rec_hdr_cid_offset, rec_hdr_cid_len ); + + ssl->in_len = ssl->in_cid + mbedtls_ssl_conf_get_cid_len( ssl->conf ); + ssl->in_iv = ssl->in_msg = ssl->in_len + 2; } else #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ - if( ssl_check_record_type( ssl->in_msgtype ) ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "unknown record type" ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + if( ssl_check_record_type( rec->type ) ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "unknown record type" ) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + } } - /* Check version */ + /* + * Parse and validate record version + */ + + memcpy( &rec->ver[0], + buf + rec_hdr_version_offset, + rec_hdr_version_len ); + + mbedtls_ssl_read_version( &major_ver, &minor_ver, + ssl->conf->transport, + buf + rec_hdr_version_offset ); + if( major_ver != ssl->major_ver ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "major version mismatch" ) ); @@ -4957,18 +5025,44 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } - MBEDTLS_SSL_DEBUG_BUF( 4, "input record header", ssl->in_hdr, mbedtls_ssl_in_hdr_len( ssl ) ); + /* + * Parse/Copy record sequence number. + */ - /* Parse and validate record length - * This must happen after the CID parsing because - * its position in the record header depends on - * the presence of a CID. */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + { + /* Copy explicit record sequence number from input buffer. */ + memcpy( &rec->ctr[0], buf + rec_hdr_ctr_offset, + rec_hdr_ctr_len ); + } + else +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + { + /* Copy implicit record sequence number from SSL context structure. */ + memcpy( &rec->ctr[0], ssl->in_ctr, rec_hdr_ctr_len ); + } - ssl->in_msglen = ( ssl->in_len[0] << 8 ) | ssl->in_len[1]; + /* + * Parse record length. + */ + +#define READ_UINT16_BE( p ) \ + ( ( *( (unsigned char*)( p ) + 0 ) << 8 ) | \ + ( *( (unsigned char*)( p ) + 1 ) << 0 ) ) + + rec->data_offset = rec_hdr_len_offset + rec_hdr_len_len; + rec->data_len = (size_t) READ_UINT16_BE( buf + rec_hdr_len_offset ); + MBEDTLS_SSL_DEBUG_BUF( 4, "input record header", buf, rec->data_offset ); + + ssl->in_msglen = rec->data_len; MBEDTLS_SSL_DEBUG_MSG( 3, ( "input record: msgtype = %d, " "version = [%d:%d], msglen = %d", - ssl->in_msgtype, - major_ver, minor_ver, ssl->in_msglen ) ); + rec->type, + major_ver, minor_ver, rec->data_len ) ); + + rec->buf = buf; + rec->buf_len = rec->data_offset + rec->data_len; /* * DTLS-related tests. @@ -4985,13 +5079,15 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_PROTO_DTLS) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) { - unsigned int rec_epoch = ( ssl->in_ctr[0] << 8 ) | ssl->in_ctr[1]; + rec_epoch = ( rec->ctr[0] << 8 ) | rec->ctr[1]; /* Check that the datagram is large enough to contain a record * of the advertised length. */ - if( ssl->in_left < mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen ) + if( len < rec->data_offset + rec->data_len ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Datagram too small to contain record." ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Datagram of length %u too small to contain record of advertised length %u.", + (unsigned) len, + (unsigned)( rec->data_offset + rec->data_len ) ) ); return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } @@ -5905,6 +6001,7 @@ static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ) static int ssl_get_next_record( mbedtls_ssl_context *ssl ) { int ret; + mbedtls_record rec; #if defined(MBEDTLS_SSL_PROTO_DTLS) /* We might have buffered a future record; if so, @@ -5933,7 +6030,8 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) return( ret ); } - if( ( ret = ssl_parse_record_header( ssl ) ) != 0 ) + ret = ssl_parse_record_header( ssl, ssl->in_hdr, ssl->in_left, &rec ); + if( ret != 0 ) { #if defined(MBEDTLS_SSL_PROTO_DTLS) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) From 519f15dbba2f942798afa41dd40f4d1704552c97 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 12:43:20 +0100 Subject: [PATCH 18/40] Adapt ssl_buffer_future_record() to work with SSL record structure --- library/ssl_tls.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d91b05d37..d270f80fb 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -230,7 +230,8 @@ static void ssl_free_buffered_record( mbedtls_ssl_context *ssl ); static int ssl_load_buffered_message( mbedtls_ssl_context *ssl ); static int ssl_load_buffered_record( mbedtls_ssl_context *ssl ); static int ssl_buffer_message( mbedtls_ssl_context *ssl ); -static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ); +static int ssl_buffer_future_record( mbedtls_ssl_context *ssl, + mbedtls_record const *rec ); static int ssl_next_record_is_in_datagram( mbedtls_ssl_context *ssl ); static size_t ssl_get_current_mtu( const mbedtls_ssl_context *ssl ); @@ -5941,11 +5942,10 @@ exit: return( 0 ); } -static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ) +static int ssl_buffer_future_record( mbedtls_ssl_context *ssl, + mbedtls_record const *rec ) { mbedtls_ssl_handshake_params * const hs = ssl->handshake; - size_t const rec_hdr_len = 13; - size_t const total_buf_sz = rec_hdr_len + ssl->in_msglen; /* Don't buffer future records outside handshakes. */ if( hs == NULL ) @@ -5953,7 +5953,7 @@ static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ) /* Only buffer handshake records (we are only interested * in Finished messages). */ - if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) + if( rec->type != MBEDTLS_SSL_MSG_HANDSHAKE ) return( 0 ); /* Don't buffer more than one future epoch record. */ @@ -5961,11 +5961,11 @@ static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ) return( 0 ); /* Don't buffer record if there's not enough buffering space remaining. */ - if( total_buf_sz > ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - + if( rec->buf_len > ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - hs->buffering.total_bytes_buffered ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "Buffering of future epoch record of size %u would exceed the compile-time limit %u (already %u bytes buffered) -- ignore\n", - (unsigned) total_buf_sz, MBEDTLS_SSL_DTLS_MAX_BUFFERING, + (unsigned) rec->buf_len, MBEDTLS_SSL_DTLS_MAX_BUFFERING, (unsigned) hs->buffering.total_bytes_buffered ) ); return( 0 ); } @@ -5973,13 +5973,12 @@ static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ) /* Buffer record */ MBEDTLS_SSL_DEBUG_MSG( 2, ( "Buffer record from epoch %u", ssl->in_epoch + 1 ) ); - MBEDTLS_SSL_DEBUG_BUF( 3, "Buffered record", ssl->in_hdr, - rec_hdr_len + ssl->in_msglen ); + MBEDTLS_SSL_DEBUG_BUF( 3, "Buffered record", rec->buf, rec->buf_len ); /* ssl_parse_record_header() only considers records * of the next epoch as candidates for buffering. */ hs->buffering.future_record.epoch = ssl->in_epoch + 1; - hs->buffering.future_record.len = total_buf_sz; + hs->buffering.future_record.len = rec->buf_len; hs->buffering.future_record.data = mbedtls_calloc( 1, hs->buffering.future_record.len ); @@ -5990,9 +5989,9 @@ static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ) return( 0 ); } - memcpy( hs->buffering.future_record.data, ssl->in_hdr, total_buf_sz ); + memcpy( hs->buffering.future_record.data, rec->buf, rec->buf_len ); - hs->buffering.total_bytes_buffered += total_buf_sz; + hs->buffering.total_bytes_buffered += rec->buf_len; return( 0 ); } @@ -6038,7 +6037,7 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) { if( ret == MBEDTLS_ERR_SSL_EARLY_MESSAGE ) { - ret = ssl_buffer_future_record( ssl ); + ret = ssl_buffer_future_record( ssl, &rec ); if( ret != 0 ) return( ret ); From 4acada35f54cbc4b439aed1669f0c101f3989ac9 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 12:48:53 +0100 Subject: [PATCH 19/40] Use SSL record structure when skipping over unexpected record --- library/ssl_tls.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d270f80fb..e16c0282f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6054,8 +6054,7 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) #endif /* Skip unexpected record (but not whole datagram) */ - ssl->next_record_offset = ssl->in_msglen - + mbedtls_ssl_in_hdr_len( ssl ); + ssl->next_record_offset = rec.buf_len; MBEDTLS_SSL_DEBUG_MSG( 1, ( "discarding unexpected record " "(header)" ) ); From f50da50c04988148d5f6e34ee39101aa2db119f0 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 12:50:10 +0100 Subject: [PATCH 20/40] Use record structure when remembering offset of next record in dgram --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e16c0282f..278027a3b 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6083,7 +6083,7 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) { /* Remember offset of next record within datagram. */ - ssl->next_record_offset = ssl->in_msglen + mbedtls_ssl_in_hdr_len( ssl ); + ssl->next_record_offset = rec.buf_len; if( ssl->next_record_offset < ssl->in_left ) { MBEDTLS_SSL_DEBUG_MSG( 3, ( "more than one record within datagram" ) ); From a31756619ca96ed10f38f13988baff39083e41cd Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 12:50:29 +0100 Subject: [PATCH 21/40] Use record length from record structure when fetching content in TLS --- library/ssl_tls.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 278027a3b..3a6efef1b 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6095,8 +6095,7 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) /* * Fetch record contents from underlying transport. */ - ret = mbedtls_ssl_fetch_input( ssl, - mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen ); + ret = mbedtls_ssl_fetch_input( ssl, rec.buf_len ); if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_fetch_input", ret ); From fdf660426d5730239fdbdf220bbc90d50b628d31 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 13:07:45 +0100 Subject: [PATCH 22/40] Adapt ssl_prepare_record_content() to use SSL record structure --- library/ssl_tls.c | 41 +++++++++++++---------------------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 3a6efef1b..46b6df8db 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5155,12 +5155,13 @@ static int ssl_check_client_reconnect( mbedtls_ssl_context *ssl ) /* * If applicable, decrypt (and decompress) record content */ -static int ssl_prepare_record_content( mbedtls_ssl_context *ssl ) +static int ssl_prepare_record_content( mbedtls_ssl_context *ssl, + mbedtls_record *rec ) { int ret, done = 0; MBEDTLS_SSL_DEBUG_BUF( 4, "input record from network", - ssl->in_hdr, mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen ); + rec->buf, rec->buf_len ); #if defined(MBEDTLS_SSL_HW_RECORD_ACCEL) if( mbedtls_ssl_hw_record_read != NULL ) @@ -5180,24 +5181,8 @@ static int ssl_prepare_record_content( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_SSL_HW_RECORD_ACCEL */ if( !done && ssl->transform_in != NULL ) { - mbedtls_record rec; - - rec.buf = ssl->in_iv; - rec.buf_len = MBEDTLS_SSL_IN_BUFFER_LEN - - ( ssl->in_iv - ssl->in_buf ); - rec.data_len = ssl->in_msglen; - rec.data_offset = 0; -#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID ) - rec.cid_len = (uint8_t)( ssl->in_len - ssl->in_cid ); - memcpy( rec.cid, ssl->in_cid, rec.cid_len ); -#endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ - - memcpy( &rec.ctr[0], ssl->in_ctr, 8 ); - mbedtls_ssl_write_version( ssl->major_ver, ssl->minor_ver, - ssl->conf->transport, rec.ver ); - rec.type = ssl->in_msgtype; if( ( ret = mbedtls_ssl_decrypt_buf( ssl, ssl->transform_in, - &rec ) ) != 0 ) + rec ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "ssl_decrypt_buf", ret ); @@ -5214,24 +5199,24 @@ static int ssl_prepare_record_content( mbedtls_ssl_context *ssl ) return( ret ); } - if( ssl->in_msgtype != rec.type ) + if( ssl->in_msgtype != rec->type ) { MBEDTLS_SSL_DEBUG_MSG( 4, ( "record type after decrypt (before %d): %d", - ssl->in_msgtype, rec.type ) ); + ssl->in_msgtype, rec->type ) ); } /* The record content type may change during decryption, * so re-read it. */ - ssl->in_msgtype = rec.type; + ssl->in_msgtype = rec->type; /* Also update the input buffer, because unfortunately * the server-side ssl_parse_client_hello() reparses the * record header when receiving a ClientHello initiating * a renegotiation. */ - ssl->in_hdr[0] = rec.type; - ssl->in_msg = rec.buf + rec.data_offset; - ssl->in_msglen = rec.data_len; - ssl->in_len[0] = (unsigned char)( rec.data_len >> 8 ); - ssl->in_len[1] = (unsigned char)( rec.data_len ); + ssl->in_hdr[0] = rec->type; + ssl->in_msg = rec->buf + rec->data_offset; + ssl->in_msglen = rec->data_len; + ssl->in_len[0] = (unsigned char)( rec->data_len >> 8 ); + ssl->in_len[1] = (unsigned char)( rec->data_len ); MBEDTLS_SSL_DEBUG_BUF( 4, "input payload after decrypt", ssl->in_msg, ssl->in_msglen ); @@ -6109,7 +6094,7 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) * Decrypt record contents. */ - if( ( ret = ssl_prepare_record_content( ssl ) ) != 0 ) + if( ( ret = ssl_prepare_record_content( ssl, &rec ) ) != 0 ) { #if defined(MBEDTLS_SSL_PROTO_DTLS) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) From 605949f84cfc299304c8940240294561299fe294 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 08:23:59 +0100 Subject: [PATCH 23/40] Mark ssl_decrypt_buf() as `const in the input SSL context In fact, the SSL context is only used to access the debug callback. --- include/mbedtls/ssl_internal.h | 2 +- library/ssl_tls.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 4028abe2b..5889972ea 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -1024,7 +1024,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, mbedtls_record *rec, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ); -int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, +int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, mbedtls_ssl_transform *transform, mbedtls_record *rec ); diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 46b6df8db..9696bd64e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2554,7 +2554,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, return( 0 ); } -int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, +int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, mbedtls_ssl_transform *transform, mbedtls_record *rec ) { From 7ae20e0f4c3c886309fed8b6940696526f90bc86 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 08:33:49 +0100 Subject: [PATCH 24/40] Move updating the internal rec ptrs to outside of rec hdr parsing The stack maintains pointers mbedtls_ssl_context::in_xxx pointing to various parts of the [D]TLS record header. Originally, these fields were determined and set in ssl_parse_record_header(). By now, ssl_parse_record_header() has been modularized to setup an instance of the internal SSL record structure mbedtls_record, and to derive the old in_xxx fields from that. This commit takes a further step towards removing the in_xxx fields by deriving them from the established record structure _outside_ of ssl_parse_record_header() after the latter has succeeded. One exception is the handling of possible client reconnects, which happens in the case then ssl_parse_record_header() returns MBEDTLS_ERR_SSL_UNEXPECTED_RECORD; since ssl_check_client_reconnect() so far uses the in_xxx fields, they need to be derived from the record structure beforehand. --- library/ssl_tls.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 9696bd64e..f16b61960 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4950,7 +4950,6 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl, */ rec->type = buf[ rec_hdr_type_offset ]; - ssl->in_msgtype = rec->type; /* Check record content type */ #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) @@ -4988,9 +4987,6 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl, rec->cid_len = rec_hdr_cid_len; memcpy( rec->cid, buf + rec_hdr_cid_offset, rec_hdr_cid_len ); - - ssl->in_len = ssl->in_cid + mbedtls_ssl_conf_get_cid_len( ssl->conf ); - ssl->in_iv = ssl->in_msg = ssl->in_len + 2; } else #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ @@ -5056,7 +5052,6 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl, rec->data_len = (size_t) READ_UINT16_BE( buf + rec_hdr_len_offset ); MBEDTLS_SSL_DEBUG_BUF( 4, "input record header", buf, rec->data_offset ); - ssl->in_msglen = rec->data_len; MBEDTLS_SSL_DEBUG_MSG( 3, ( "input record: msgtype = %d, " "version = [%d:%d], msglen = %d", rec->type, @@ -6033,6 +6028,14 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) if( ret == MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ) { #if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && defined(MBEDTLS_SSL_SRV_C) + /* Setup internal message pointers from record structure. */ + ssl->in_msgtype = rec.type; +#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) + ssl->in_len = ssl->in_cid + rec.cid_len; +#endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ + ssl->in_iv = ssl->in_msg = ssl->in_len + 2; + ssl->in_msglen = rec.data_len; + ret = ssl_check_client_reconnect( ssl ); if( ret != 0 ) return( ret ); @@ -6064,6 +6067,14 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) } } + /* Setup internal message pointers from record structure. */ + ssl->in_msgtype = rec.type; +#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) + ssl->in_len = ssl->in_cid + rec.cid_len; +#endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ + ssl->in_iv = ssl->in_msg = ssl->in_len + 2; + ssl->in_msglen = rec.data_len; + #if defined(MBEDTLS_SSL_PROTO_DTLS) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) { From 0183d699bf06efe5bf92785a2057d57de91f56d2 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 08:50:37 +0100 Subject: [PATCH 25/40] Mark DTLS replay check as `const` on the SSL context --- include/mbedtls/ssl_internal.h | 2 +- library/ssl_tls.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 5889972ea..248689262 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -969,7 +969,7 @@ int mbedtls_ssl_flight_transmit( mbedtls_ssl_context *ssl ); /* Visible for testing purposes only */ #if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY) -int mbedtls_ssl_dtls_replay_check( mbedtls_ssl_context *ssl ); +int mbedtls_ssl_dtls_replay_check( mbedtls_ssl_context const *ssl ); void mbedtls_ssl_dtls_replay_update( mbedtls_ssl_context *ssl ); #endif diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f16b61960..d200304f4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4608,7 +4608,7 @@ static inline uint64_t ssl_load_six_bytes( unsigned char *buf ) /* * Return 0 if sequence number is acceptable, -1 otherwise */ -int mbedtls_ssl_dtls_replay_check( mbedtls_ssl_context *ssl ) +int mbedtls_ssl_dtls_replay_check( mbedtls_ssl_context const *ssl ) { uint64_t rec_seqnum = ssl_load_six_bytes( ssl->in_ctr + 2 ); uint64_t bit; From d8bf8ceeb43d4f79a31f0b95af76a57f985cd6e4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 09:23:47 +0100 Subject: [PATCH 26/40] Move ssl_update_in_pointers() to after record hdr parsing Previously, ssl_update_in_pointers() ensured that the in_xxx pointers in the SSL context are set to their default state so that the record header parsing function ssl_parse_record_header() could make use of them. By now, the latter is independent of these pointers, so they don't need to be setup before calling ssl_parse_record_header() anymore. However, other parts of the messaging stack might still depend on it (to be studied), and hence this commit does not yet reomve ssl_update_in_pointers() entirely. --- library/ssl_tls.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d200304f4..128a40e13 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5994,11 +5994,6 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) return( ret ); #endif /* MBEDTLS_SSL_PROTO_DTLS */ - /* Reset in pointers to default state for TLS/DTLS records, - * assuming no CID and no offset between record content and - * record plaintext. */ - ssl_update_in_pointers( ssl ); - /* Ensure that we have enough space available for the default form * of TLS / DTLS record headers (5 Bytes for TLS, 13 Bytes for DTLS, * with no space for CIDs counted in). */ @@ -6028,6 +6023,11 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) if( ret == MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ) { #if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && defined(MBEDTLS_SSL_SRV_C) + /* Reset in pointers to default state for TLS/DTLS records, + * assuming no CID and no offset between record content and + * record plaintext. */ + ssl_update_in_pointers( ssl ); + /* Setup internal message pointers from record structure. */ ssl->in_msgtype = rec.type; #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) @@ -6067,6 +6067,11 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) } } + /* Reset in pointers to default state for TLS/DTLS records, + * assuming no CID and no offset between record content and + * record plaintext. */ + ssl_update_in_pointers( ssl ); + /* Setup internal message pointers from record structure. */ ssl->in_msgtype = rec.type; #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) From 58ef0bf19fc4602d629bd4db68290fec275ff73c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 09:35:58 +0100 Subject: [PATCH 27/40] Reduce dependency of ssl_prepare_record_content() on in_xxx fields --- library/ssl_tls.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 128a40e13..2fd615320 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5176,6 +5176,8 @@ static int ssl_prepare_record_content( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_HW_RECORD_ACCEL */ if( !done && ssl->transform_in != NULL ) { + unsigned char const old_msg_type = rec->type; + if( ( ret = mbedtls_ssl_decrypt_buf( ssl, ssl->transform_in, rec ) ) != 0 ) { @@ -5194,10 +5196,10 @@ static int ssl_prepare_record_content( mbedtls_ssl_context *ssl, return( ret ); } - if( ssl->in_msgtype != rec->type ) + if( old_msg_type != rec->type ) { MBEDTLS_SSL_DEBUG_MSG( 4, ( "record type after decrypt (before %d): %d", - ssl->in_msgtype, rec->type ) ); + old_msg_type, rec->type ) ); } /* The record content type may change during decryption, @@ -5214,7 +5216,7 @@ static int ssl_prepare_record_content( mbedtls_ssl_context *ssl, ssl->in_len[1] = (unsigned char)( rec->data_len ); MBEDTLS_SSL_DEBUG_BUF( 4, "input payload after decrypt", - ssl->in_msg, ssl->in_msglen ); + rec->buf + rec->data_offset, rec->data_len ); #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) /* We have already checked the record content type @@ -5224,18 +5226,18 @@ static int ssl_prepare_record_content( mbedtls_ssl_context *ssl, * Since with the use of CIDs, the record content type * might change during decryption, re-check the record * content type, but treat a failure as fatal this time. */ - if( ssl_check_record_type( ssl->in_msgtype ) ) + if( ssl_check_record_type( rec->type ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "unknown record type" ) ); return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ - if( ssl->in_msglen == 0 ) + if( rec->data_len == 0 ) { #if defined(MBEDTLS_SSL_PROTO_TLS1_2) if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 - && ssl->in_msgtype != MBEDTLS_SSL_MSG_APPLICATION_DATA ) + && rec->type != MBEDTLS_SSL_MSG_APPLICATION_DATA ) { /* TLS v1.2 explicitly disallows zero-length messages which are not application data */ MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid zero-length message type: %d", ssl->in_msgtype ) ); From 8685c822c10bf6a6d17b56d4a7b3bf7c987b7910 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 09:37:30 +0100 Subject: [PATCH 28/40] Move update of in_xxx fields outside of ssl_prepare_record_content() Multiple record attributes such as content type and payload length may change during record decryption, and the legacy in_xxx fields in the SSL context therefore need to be updated after the record decryption routine ssl_decrypt_buf() has been called. After the previous commit has made ssl_prepare_record_content() independent of the in_xxx fields, setting them can be moved outside of ssl_prepare_record_content(), which is what this commit does. --- library/ssl_tls.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 2fd615320..fec43fe39 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5202,19 +5202,6 @@ static int ssl_prepare_record_content( mbedtls_ssl_context *ssl, old_msg_type, rec->type ) ); } - /* The record content type may change during decryption, - * so re-read it. */ - ssl->in_msgtype = rec->type; - /* Also update the input buffer, because unfortunately - * the server-side ssl_parse_client_hello() reparses the - * record header when receiving a ClientHello initiating - * a renegotiation. */ - ssl->in_hdr[0] = rec->type; - ssl->in_msg = rec->buf + rec->data_offset; - ssl->in_msglen = rec->data_len; - ssl->in_len[0] = (unsigned char)( rec->data_len >> 8 ); - ssl->in_len[1] = (unsigned char)( rec->data_len ); - MBEDTLS_SSL_DEBUG_BUF( 4, "input payload after decrypt", rec->buf + rec->data_offset, rec->data_len ); @@ -6174,6 +6161,19 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) } } + /* The record content type may change during decryption, + * so re-read it. */ + ssl->in_msgtype = rec.type; + /* Also update the input buffer, because unfortunately + * the server-side ssl_parse_client_hello() reparses the + * record header when receiving a ClientHello initiating + * a renegotiation. */ + ssl->in_hdr[0] = rec.type; + ssl->in_msg = rec.buf + rec.data_offset; + ssl->in_msglen = rec.data_len; + ssl->in_len[0] = (unsigned char)( rec.data_len >> 8 ); + ssl->in_len[1] = (unsigned char)( rec.data_len ); + return( 0 ); } From 44d89b2d53a66fd31f770e16ffd6a0599438626d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 09:40:44 +0100 Subject: [PATCH 29/40] Move update of in_xxx fields in ssl_get_next_record() ssl_get_next_record() updates the legacy in_xxx fields in two places, once before record decryption and once after. Now that record decryption doesn't use or affect the in_xxx fields anymore, setting up the these legacy fields can entirely be moved to the end of ssl_get_next_record(), which is what this comit does. This commit solely moves existing code, but doesn't yet simplify the now partially redundant settings of the in_xxx fields. This will be done in a separate commit. --- library/ssl_tls.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index fec43fe39..fb3a4a090 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6056,19 +6056,6 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) } } - /* Reset in pointers to default state for TLS/DTLS records, - * assuming no CID and no offset between record content and - * record plaintext. */ - ssl_update_in_pointers( ssl ); - - /* Setup internal message pointers from record structure. */ - ssl->in_msgtype = rec.type; -#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) - ssl->in_len = ssl->in_cid + rec.cid_len; -#endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ - ssl->in_iv = ssl->in_msg = ssl->in_len + 2; - ssl->in_msglen = rec.data_len; - #if defined(MBEDTLS_SSL_PROTO_DTLS) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) { @@ -6161,6 +6148,20 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) } } + + /* Reset in pointers to default state for TLS/DTLS records, + * assuming no CID and no offset between record content and + * record plaintext. */ + ssl_update_in_pointers( ssl ); + + /* Setup internal message pointers from record structure. */ + ssl->in_msgtype = rec.type; +#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) + ssl->in_len = ssl->in_cid + rec.cid_len; +#endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ + ssl->in_iv = ssl->in_msg = ssl->in_len + 2; + ssl->in_msglen = rec.data_len; + /* The record content type may change during decryption, * so re-read it. */ ssl->in_msgtype = rec.type; From b0fe0eedce748e14c6f702164ea0334cdbf5b8ff Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 09:44:55 +0100 Subject: [PATCH 30/40] Remove duplicate setting of ssl->in_msgtype and ssl->in_msglen --- library/ssl_tls.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index fb3a4a090..bb51575d9 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6153,14 +6153,10 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) * assuming no CID and no offset between record content and * record plaintext. */ ssl_update_in_pointers( ssl ); - - /* Setup internal message pointers from record structure. */ - ssl->in_msgtype = rec.type; #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) ssl->in_len = ssl->in_cid + rec.cid_len; #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ ssl->in_iv = ssl->in_msg = ssl->in_len + 2; - ssl->in_msglen = rec.data_len; /* The record content type may change during decryption, * so re-read it. */ From 47be7686ab5fc67a33c71288e3c93e29e450e122 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 09:55:46 +0100 Subject: [PATCH 31/40] Make mbedtls_ssl_in_hdr_len() CID-unaware The function mbedtls_ssl_in_hdr_len() is supposed to return the length of the record header of the current incoming record. With the advent of the DTLS Connection ID, this length is only known at runtime and hence so far needed to be derived from the internal in_iv pointer pointing to the beginning of the payload of the current incooing record. By now, however, those uses of mbedtls_ssl_in_hdr_len() where the presence of a CID would need to be detected have been removed (specifically, ssl_parse_record_header() doesn't use it anymore when checking that the current datagram is large enough to hold the record header, including the CID), and it's sufficient to statically return the default record header sizes of 5 / 13 Bytes for TLS / DTLS. --- include/mbedtls/ssl_internal.h | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 248689262..5fb201aea 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -941,7 +941,20 @@ void mbedtls_ssl_read_version( int *major, int *minor, int transport, static inline size_t mbedtls_ssl_in_hdr_len( const mbedtls_ssl_context *ssl ) { - return( (size_t) ( ssl->in_iv - ssl->in_hdr ) ); +#if !defined(MBEDTLS_SSL_PROTO_DTLS) + ((void) ssl); +#endif + +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + { + return( 13 ); + } + else +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + { + return( 5 ); + } } static inline size_t mbedtls_ssl_out_hdr_len( const mbedtls_ssl_context *ssl ) From 331de3df9a09463d66e1f19a1fb0231a85a36765 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 11:10:16 +0100 Subject: [PATCH 32/40] Mark ssl_parse_record_header() as `const` in SSL context --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index bb51575d9..7d5f95c20 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4892,7 +4892,7 @@ static int ssl_check_record_type( uint8_t record_type ) * Point 2 is needed when the peer is resending, and we have already received * the first record from a datagram but are still waiting for the others. */ -static int ssl_parse_record_header( mbedtls_ssl_context *ssl, +static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, unsigned char *buf, size_t len, mbedtls_record *rec ) From 5422981052b7a44831dda67a0bc19f3f3d8069db Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 14:40:00 +0100 Subject: [PATCH 33/40] Implement record checking API This commit implements the record checking API mbedtls_ssl_check_record() on top of the restructured incoming record stack. Specifically, it makes use of the fact that the core processing routines ssl_parse_record_header() mbedtls_ssl_decrypt_buf() now operate on instances of the SSL record structure mbedtls_record instead of the previous mbedtls_ssl_context::in_xxx fields. --- library/ssl_tls.c | 64 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 59 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 7d5f95c20..9f35aae6a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -113,14 +113,67 @@ static void ssl_update_out_pointers( mbedtls_ssl_context *ssl, static void ssl_update_in_pointers( mbedtls_ssl_context *ssl ); #if defined(MBEDTLS_SSL_RECORD_CHECKING) +static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, + unsigned char *buf, + size_t len, + mbedtls_record *rec ); + int mbedtls_ssl_check_record( mbedtls_ssl_context const *ssl, unsigned char *buf, size_t buflen ) { - ((void) ssl); - ((void) buf); - ((void) buflen); - return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); + int ret = 0; + mbedtls_record rec; + MBEDTLS_SSL_DEBUG_MSG( 1, ( "=> mbedtls_ssl_check_record" ) ); + MBEDTLS_SSL_DEBUG_BUF( 3, "record buffer", buf, buflen ); + + /* We don't support record checking in TLS because + * (a) there doesn't seem to be a usecase for it, and + * (b) In SSLv3 and TLS 1.0, CBC record decryption has state + * and we'd need to backup the transform here. + */ + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_STREAM ) + { + ret = MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE; + goto exit; + } +#if defined(MBEDTLS_SSL_PROTO_DTLS) + else + { + ret = ssl_parse_record_header( ssl, buf, buflen, &rec ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 3, "ssl_parse_record_header", ret ); + goto exit; + } + + if( ssl->transform_in != NULL ) + { + ret = mbedtls_ssl_decrypt_buf( ssl, ssl->transform_in, &rec ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 3, "mbedtls_ssl_decrypt_buf", ret ); + goto exit; + } + } + } +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + +exit: + /* On success, we have decrypted the buffer in-place, so make + * sure we don't leak any plaintext data. */ + mbedtls_platform_zeroize( buf, buflen ); + + /* For the purpose of this API, treat messages with unexpected CID + * as well as such from future epochs as unexpected. */ + if( ret == MBEDTLS_ERR_SSL_UNEXPECTED_CID || + ret == MBEDTLS_ERR_SSL_EARLY_MESSAGE ) + { + ret = MBEDTLS_ERR_SSL_UNEXPECTED_RECORD; + } + + MBEDTLS_SSL_DEBUG_MSG( 1, ( "<= mbedtls_ssl_check_record" ) ); + return( ret ); } #endif /* MBEDTLS_SSL_RECORD_CHECKING */ @@ -4993,7 +5046,8 @@ static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, { if( ssl_check_record_type( rec->type ) ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "unknown record type" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "unknown record type %u", + (unsigned) rec->type ) ); return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } } From 552f74721695739f0dc460c17319ea877847c77b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 19 Jul 2019 10:59:12 +0100 Subject: [PATCH 34/40] Make sure 'record from another epoch' is displayed for next epoch The test 'DTLS proxy: delay ChangeCipherSpec' from ssl-opt.sh relies on this. --- library/ssl_tls.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 9f35aae6a..27a6c49be 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5141,21 +5141,23 @@ static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } - /* Records from the next epoch are considered for buffering - * (concretely: early Finished messages). */ - if( rec_epoch == (unsigned) ssl->in_epoch + 1 ) - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "Consider record for buffering" ) ); - return( MBEDTLS_ERR_SSL_EARLY_MESSAGE ); - } /* Records from other, non-matching epochs are silently discarded. * (The case of same-port Client reconnects must be considered in * the caller). */ - else if( rec_epoch != ssl->in_epoch ) + if( rec_epoch != ssl->in_epoch ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "record from another epoch: " "expected %d, received %d", ssl->in_epoch, rec_epoch ) ); + + /* Records from the next epoch are considered for buffering + * (concretely: early Finished messages). */ + if( rec_epoch == (unsigned) ssl->in_epoch + 1 ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Consider record for buffering" ) ); + return( MBEDTLS_ERR_SSL_EARLY_MESSAGE ); + } + return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ); } #if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY) From b2a86c3e012b94a20d9407360a730b2b5e3118ee Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 19 Jul 2019 15:43:09 +0100 Subject: [PATCH 35/40] Don't disallow 'record from another epoch' log msg in proxy ref test It happens regularly in test runs that the server example application shuts down a connection, goes into waiting mode for a new connection, and then receives the encrypted ClosureAlert from the client. The only reason why this does currently not trigger the 'record from another epoch' message is that we handle ClientHello parsing outside of the main record stack because we want to be able to detect SSLv2 ClientHellos. However, this is likely to go away, and once it happens, we'll see the log message. Further, when record checking is used, every record, including the mentioned closure alert, is passed to the record checking API before being passed to the rest of the stack, which leads to the log message being printed. In summary, grepping for 'record from another epoch' is a fragile way of checking whether a reordered message has arrived. A more reliable way is to grep for 'Buffer record from epoch' which is printed when a record from a future epoch is actually buffered, and 'ssl_buffer_message' which is the function buffering a future handshake message. --- 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 ed97f233f..44743d4a1 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -8005,8 +8005,10 @@ run_test "DTLS proxy: reference" \ 0 \ -C "replayed record" \ -S "replayed record" \ - -C "record from another epoch" \ - -S "record from another epoch" \ + -C "Buffer record from epoch" \ + -S "Buffer record from epoch" \ + -C "ssl_buffer_message" \ + -S "ssl_buffer_message" \ -C "discarding invalid record" \ -S "discarding invalid record" \ -S "resend" \ From f5466258b4896936796469c9c153c5b375f05bb8 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 25 Jul 2019 10:13:02 +0100 Subject: [PATCH 36/40] Fix alignment in record header parsing routine --- library/ssl_tls.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 27a6c49be..56887f966 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4961,14 +4961,14 @@ static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, size_t const rec_hdr_ctr_len = 8; #if defined(MBEDTLS_SSL_PROTO_DTLS) - uint32_t rec_epoch; + uint32_t rec_epoch; size_t const rec_hdr_ctr_offset = rec_hdr_version_offset + rec_hdr_version_len; #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) size_t const rec_hdr_cid_offset = rec_hdr_ctr_offset + rec_hdr_ctr_len; - size_t rec_hdr_cid_len = 0; + size_t rec_hdr_cid_len = 0; #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ #endif /* MBEDTLS_SSL_PROTO_DTLS */ From 9eca2767688c2982a457e0525f06d89b564ae1f0 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 25 Jul 2019 10:16:37 +0100 Subject: [PATCH 37/40] Remove integer parsing macro If this is introduced, it should be defined in a prominent place and put to use throughout the library, but this is left for another time. --- library/ssl_tls.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 56887f966..102a13f7d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5098,12 +5098,9 @@ static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, * Parse record length. */ -#define READ_UINT16_BE( p ) \ - ( ( *( (unsigned char*)( p ) + 0 ) << 8 ) | \ - ( *( (unsigned char*)( p ) + 1 ) << 0 ) ) - rec->data_offset = rec_hdr_len_offset + rec_hdr_len_len; - rec->data_len = (size_t) READ_UINT16_BE( buf + rec_hdr_len_offset ); + rec->data_len = ( (size_t) buf[ rec_hdr_len_offset + 0 ] << 8 ) | + ( (size_t) buf[ rec_hdr_len_offset + 1 ] << 0 ); MBEDTLS_SSL_DEBUG_BUF( 4, "input record header", buf, rec->data_offset ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "input record: msgtype = %d, " From d0b66d08bb4beb721b2e6ccff4f4aaf172006126 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 26 Jul 2019 08:07:03 +0100 Subject: [PATCH 38/40] Don't use memcpy() for 2-byte copy operation Manual copying is slightly shorter here. --- library/ssl_tls.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 102a13f7d..3e0552c4e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5056,13 +5056,11 @@ static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, * Parse and validate record version */ - memcpy( &rec->ver[0], - buf + rec_hdr_version_offset, - rec_hdr_version_len ); - + rec->ver[0] = buf[ rec_hdr_version_offset + 0 ]; + rec->ver[1] = buf[ rec_hdr_version_offset + 1 ]; mbedtls_ssl_read_version( &major_ver, &minor_ver, ssl->conf->transport, - buf + rec_hdr_version_offset ); + &rec->ver[0] ); if( major_ver != ssl->major_ver ) { From d417cc945cf9dd6aad9e6cfca710187cf1fb537e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 26 Jul 2019 08:20:27 +0100 Subject: [PATCH 39/40] Reintroduce length 0 check for records --- library/ssl_tls.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 3e0552c4e..a9c099c1f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5109,6 +5109,9 @@ static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, rec->buf = buf; rec->buf_len = rec->data_offset + rec->data_len; + if( rec->data_len == 0 ) + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + /* * DTLS-related tests. * Check epoch before checking length constraint because From 7e821b5bcde4d2086b116245b3af6ae2e20e14e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 2 Aug 2019 10:17:15 +0200 Subject: [PATCH 40/40] Fix possibly-lossy conversion warning from MSVC ssl_tls.c(4876): warning C4267: '=': conversion from 'size_t' to 'uint8_t', possible loss of data --- library/ssl_tls.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a9c099c1f..37f2dabb9 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5038,7 +5038,9 @@ static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } - rec->cid_len = rec_hdr_cid_len; + /* configured CID len is guaranteed at most 255, see + * MBEDTLS_SSL_CID_OUT_LEN_MAX in check_config.h */ + rec->cid_len = (uint8_t) rec_hdr_cid_len; memcpy( rec->cid, buf + rec_hdr_cid_offset, rec_hdr_cid_len ); } else