From 1c2684577720c0bfb1bbecc432d4ae39028a00bd Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 11:37:19 +0100 Subject: [PATCH 01/42] 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 d04602027..a0e992d32 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4776,13 +4776,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 408a2742b3da1e593448232c7897c67f063615ab Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 9 Jul 2019 17:27:32 +0100 Subject: [PATCH 02/42] 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 a0e992d32..fe6112600 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4854,8 +4854,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 f024285034abe8e079eb38b5a7c67903912d5e10 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 9 Jul 2019 17:30:02 +0100 Subject: [PATCH 03/42] 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 fe6112600..ab33d8bf7 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4852,43 +4852,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( mbedtls_ssl_get_minor_ver( ssl ) == 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( mbedtls_ssl_get_minor_ver( ssl ) >= 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 ); @@ -4994,12 +4964,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( mbedtls_ssl_get_minor_ver( ssl ) == MBEDTLS_SSL_MINOR_VERSION_3 @@ -5069,6 +5034,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 b603bd34bc370d9a32c6762cfb067e7918bd0146 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 09:45:44 +0100 Subject: [PATCH 04/42] 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 ab33d8bf7..5f27e2da6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2368,11 +2368,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 9520b31860cec6dadebb9f5049f5479cde5342a9 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 09:49:56 +0100 Subject: [PATCH 05/42] 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 5f27e2da6..8977fecb7 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2423,9 +2423,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 6d3db0fa2516b09d990af1b99455092bbb5f9658 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 13:55:25 +0100 Subject: [PATCH 06/42] 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 8977fecb7..5eed60bc4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2423,6 +2423,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) " @@ -2431,17 +2438,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; @@ -2452,12 +2462,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; @@ -2466,6 +2479,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 ); @@ -2473,7 +2492,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 */ @@ -2494,6 +2512,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" ) ); @@ -2561,11 +2580,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, @@ -2580,6 +2608,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 ) { @@ -2593,6 +2622,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", @@ -2607,9 +2640,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, if( mbedtls_ssl_transform_get_minor_ver( transform ) >= 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; @@ -2618,6 +2649,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 ) @@ -2626,6 +2659,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" ) ); @@ -2637,7 +2671,10 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context *ssl, 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 ); @@ -2646,7 +2683,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 ) @@ -2776,7 +2814,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) @@ -2831,7 +2868,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 8244cfa8bc84c298c56e4f41bd2f920797f798c0 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 13:55:17 +0100 Subject: [PATCH 07/42] 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 5eed60bc4..1aeadbdf2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4879,17 +4879,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 07d420d6adaaadb63992854236b860b2b068bfb9 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 11:44:13 +0100 Subject: [PATCH 08/42] 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 1aeadbdf2..5f602b0b6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2482,12 +2482,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 87b5626d736c1780be73d379dc4e72ac456af533 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 14:37:41 +0100 Subject: [PATCH 09/42] 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 | 103 ++++++++++++++++++++++++++-------------------- 1 file changed, 58 insertions(+), 45 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5f602b0b6..26dadb83f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4522,9 +4522,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 @@ -4650,6 +4647,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, @@ -4666,8 +4671,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) mbedtls_ssl_get_send( ssl )( ssl->p_bio, ssl->out_buf, len ); - - return( MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED ); + ret = 0; } if( ret == 0 ) @@ -4824,50 +4828,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( mbedtls_ssl_conf_get_endpoint( ssl->conf ) == - 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 ); @@ -4879,6 +4855,35 @@ 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 && + mbedtls_ssl_conf_get_endpoint( ssl->conf ) == + 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 */ @@ -5759,8 +5764,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( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) && - ret != MBEDTLS_ERR_SSL_CLIENT_RECONNECT ) + if( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) { if( ret == MBEDTLS_ERR_SSL_EARLY_MESSAGE ) { @@ -5774,6 +5778,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 ); @@ -5794,8 +5804,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 de7d6d33e5804644adac9ffcb8baefdca6d73a31 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 14:50:10 +0100 Subject: [PATCH 10/42] 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 26dadb83f..801426843 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4755,6 +4755,18 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) * fixed in the configuration. */ ssl->in_len = ssl->in_cid + mbedtls_ssl_conf_get_cid_len( ssl->conf ); 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 */ @@ -4788,16 +4800,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 29823466a1690f1398a0c58b76841cad38eda78f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 14:53:43 +0100 Subject: [PATCH 11/42] 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 801426843..2b5775339 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4725,7 +4725,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 */ @@ -4761,11 +4760,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 dc4d62748cc6878ba56cfa45a6c7ac55faac74c2 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 15:01:45 +0100 Subject: [PATCH 12/42] 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 | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 2b5775339..c7f8e4858 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5813,19 +5813,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( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) { + 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 ) { @@ -5833,12 +5835,24 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) } } MBEDTLS_SSL_TRANSPORT_ELSE -#endif +#endif /* MBEDTLS_SSL_PROTO_DTLS */ #if defined(MBEDTLS_SSL_PROTO_TLS) { + 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; } -#endif +#endif /* MBEDTLS_SSL_PROTO_TLS */ + + /* + * Decrypt record contents. + */ if( ( ret = ssl_prepare_record_content( ssl ) ) != 0 ) { From a61925fa513267564ccffc35c9e5a7282b4b5e21 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 16:53:30 +0100 Subject: [PATCH 13/42] 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 | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index c7f8e4858..343590206 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4772,17 +4772,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_TLS) - /* Silently ignore invalid DTLS records as recommended by RFC 6347 - * Section 4.1.2.7, that is, send alert only with TLS */ - if( MBEDTLS_SSL_TRANSPORT_IS_TLS( ssl->conf->transport ) ) - { - mbedtls_ssl_pend_fatal_alert( ssl, - MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE ); - } -#endif /* MBEDTLS_SSL_PROTO_TLS */ - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } From e04527755b613730bfd9abf8618bfa88a5e53798 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 17:12:07 +0100 Subject: [PATCH 14/42] 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 343590206..647e49d62 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4818,6 +4818,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. */ @@ -5801,21 +5808,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( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) { - 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 ) @@ -5827,6 +5822,9 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_SSL_PROTO_DTLS */ #if defined(MBEDTLS_SSL_PROTO_TLS) { + /* + * 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 6c0e53ce6f92d506c741bc24da07a88ea88a6c12 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 17:20:01 +0100 Subject: [PATCH 15/42] 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 647e49d62..036fba781 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4825,12 +4825,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: " @@ -4839,7 +4844,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 e84b28cb9d82cf6c80d2bdb0e6cabbc1bdbe67e7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 09:24:36 +0100 Subject: [PATCH 16/42] 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 9b8e21fd0..1553200ce 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -789,18 +789,29 @@ static inline int mbedtls_ssl_transform_uses_aead( 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 c6e7c573d96b4ce5de01027f227459968345b39f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 12:29:35 +0100 Subject: [PATCH 17/42] 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 | 168 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 135 insertions(+), 33 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 036fba781..0ae2f1b76 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4722,20 +4722,75 @@ 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( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) + { + rec_hdr_len_offset = rec_hdr_ctr_offset + rec_hdr_ctr_len; + } + MBEDTLS_SSL_TRANSPORT_ELSE +#endif /* MBEDTLS_SSL_PROTO_DTLS */ +#if defined(MBEDTLS_SSL_PROTO_TLS) + { + rec_hdr_len_offset = rec_hdr_version_offset + rec_hdr_version_len; + } +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + + 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( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) && - ssl->in_msgtype == MBEDTLS_SSL_MSG_CID && - mbedtls_ssl_conf_get_cid_len( ssl->conf ) != 0 ) + mbedtls_ssl_conf_get_cid_len( ssl->conf ) != 0 && + rec->type == MBEDTLS_SSL_MSG_CID ) { /* Shift pointers to account for record header including CID * struct { @@ -4752,30 +4807,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 + mbedtls_ssl_conf_get_cid_len( ssl->conf ); - ssl->in_iv = ssl->in_msg = ssl->in_len + 2; + rec_hdr_cid_len = mbedtls_ssl_conf_get_cid_len( ssl->conf ); + 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 != mbedtls_ssl_get_major_ver( ssl ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "major version mismatch" ) ); @@ -4788,18 +4858,46 @@ 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( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) + { + /* Copy explicit record sequence number from input buffer. */ + memcpy( &rec->ctr[0], buf + rec_hdr_ctr_offset, + rec_hdr_ctr_len ); + } + MBEDTLS_SSL_TRANSPORT_ELSE +#endif /* MBEDTLS_SSL_PROTO_DTLS */ +#if defined(MBEDTLS_SSL_PROTO_TLS) + { + /* Copy implicit record sequence number from SSL context structure. */ + memcpy( &rec->ctr[0], ssl->in_ctr, rec_hdr_ctr_len ); + } +#endif /* MBEDTLS_SSL_PROTO_TLS */ - 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. @@ -4816,13 +4914,15 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_PROTO_DTLS) if( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) { - 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 ); } @@ -5736,6 +5836,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, @@ -5764,7 +5865,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( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) From af5bcfc7654902baf8f4dc78967ac883514878cd Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 12:43:20 +0100 Subject: [PATCH 18/42] 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 0ae2f1b76..127e2f414 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -258,7 +258,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 ); @@ -5776,11 +5777,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 ) @@ -5788,7 +5788,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. */ @@ -5796,11 +5796,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 ); } @@ -5808,13 +5808,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 ); @@ -5825,9 +5824,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 ); } @@ -5873,7 +5872,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 2528ee09ac4871ca605cb308136b6ccf284110e4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 12:48:53 +0100 Subject: [PATCH 19/42] 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 127e2f414..0a5f08cc2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5889,8 +5889,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 2720f4c33cab54da1f0b1bdc88a58c58415f78fe Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 12:50:10 +0100 Subject: [PATCH 20/42] 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 0a5f08cc2..8785baaaa 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5918,7 +5918,7 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) if( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) { /* 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 9babbf7e752ae1ce921936a545b2b21c5e46fa9a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 12:50:29 +0100 Subject: [PATCH 21/42] 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 8785baaaa..2bd621f4d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5931,8 +5931,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 a89610aaf2dd2d78c87f2114b61200b4beb27977 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 Jul 2019 13:07:45 +0100 Subject: [PATCH 22/42] Adapt ssl_prepare_record_content() to use SSL record structure --- library/ssl_tls.c | 42 +++++++++++++----------------------------- 1 file changed, 13 insertions(+), 29 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 2bd621f4d..69ba0dba5 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4991,12 +4991,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 ) @@ -5016,25 +5017,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( mbedtls_ssl_get_major_ver( ssl ), - mbedtls_ssl_get_minor_ver( ssl ), - 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 ); @@ -5051,24 +5035,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 ); @@ -5946,7 +5930,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( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) From 40478be987f899c323e8d6861848055bfd281e0c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 08:23:59 +0100 Subject: [PATCH 23/42] 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 1553200ce..82f91412b 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -1223,7 +1223,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 69ba0dba5..39a32c8de 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2349,7 +2349,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 69412458526aabab2a230ff54d5a8cace295d78c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 08:33:49 +0100 Subject: [PATCH 24/42] 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 39a32c8de..96e374e1e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4783,7 +4783,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) @@ -4821,9 +4820,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 */ @@ -4891,7 +4887,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, @@ -5867,6 +5862,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 ); @@ -5898,6 +5901,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( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) { From fc55172c41268732ff759cada1501a568af87018 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 08:50:37 +0100 Subject: [PATCH 25/42] 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 82f91412b..422df3fd3 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -1106,7 +1106,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 96e374e1e..bfa944697 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4433,7 +4433,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 68379720b6a65c02c6da18212c9282a325254acb Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 09:23:47 +0100 Subject: [PATCH 26/42] 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 bfa944697..33eec39ee 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5828,11 +5828,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). */ @@ -5862,6 +5857,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) @@ -5901,6 +5901,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 106f3dab57ed7efeb704c40bfce46408fd1a1445 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 09:35:58 +0100 Subject: [PATCH 27/42] 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 33eec39ee..00182f0a2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5012,6 +5012,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 ) { @@ -5030,10 +5032,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, @@ -5050,7 +5052,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 @@ -5060,18 +5062,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( mbedtls_ssl_get_minor_ver( ssl ) == 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 bf256cdb0b88c374f335c128c3c922d4d5d00856 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 09:37:30 +0100 Subject: [PATCH 28/42] 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 00182f0a2..49a009dca 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5038,19 +5038,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 ); @@ -6010,6 +5997,19 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_SSL_PROTO_TLS */ } + /* 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 bd70c8e77190f8f0498f60830c8ddac1d3958328 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 09:40:44 +0100 Subject: [PATCH 29/42] 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 49a009dca..7e3b9ebae 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5890,19 +5890,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( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) { @@ -5997,6 +5984,20 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_SSL_PROTO_TLS */ } + + /* 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 05413d9041c10f1bff102df36bcd68309e3b12cc Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 09:44:55 +0100 Subject: [PATCH 30/42] 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 7e3b9ebae..9083156fc 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5989,14 +5989,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 f903dc8354379f191b71118e5fd21cf83cc3b56a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 09:55:46 +0100 Subject: [PATCH 31/42] 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 | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 422df3fd3..ee0fa297b 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -1073,7 +1073,22 @@ int mbedtls_ssl_check_cert_usage( const mbedtls_x509_crt *cert, 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__BOTH) + ((void) ssl); +#endif + +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) + { + return( 13 ); + } + MBEDTLS_SSL_TRANSPORT_ELSE +#endif /* MBEDTLS_SSL_PROTO_DTLS */ +#if defined(MBEDTLS_SSL_PROTO_TLS) + { + return( 5 ); + } +#endif /* MBEDTLS_SSL_PROTO_TLS */ } static inline size_t mbedtls_ssl_out_hdr_len( const mbedtls_ssl_context *ssl ) From c360dcc679d3c84a4f7137f1a500266de95709e4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 10:00:45 +0100 Subject: [PATCH 32/42] [API break] Remove mbedtls_ssl_context::in_iv field After the rewrite of incoming record processing to use the internal SSL record structure mbedtls_record (which contains the data_offset field to indicate where the IV resides), this field is no longer necessary. Note: This is an API break. --- include/mbedtls/ssl.h | 1 - library/ssl_tls.c | 17 ++++++----------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index edb0a4699..ec2cf4574 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1317,7 +1317,6 @@ struct mbedtls_ssl_context * (the end is marked by in_len). */ #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ unsigned char *in_len; /*!< two-bytes message length field */ - unsigned char *in_iv; /*!< ivlen-byte IV */ unsigned char *in_msg; /*!< message contents (in_iv+ivlen) */ unsigned char *in_offt; /*!< read offset in application data */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 9083156fc..40fe19fb7 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5856,7 +5856,7 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) #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_msg = ssl->in_len + 2; ssl->in_msglen = rec.data_len; ret = ssl_check_client_reconnect( ssl ); @@ -5992,7 +5992,7 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) #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_msg = ssl->in_len + 2; /* The record content type may change during decryption, * so re-read it. */ @@ -7991,9 +7991,8 @@ static void ssl_update_out_pointers( mbedtls_ssl_context *ssl, static void ssl_update_in_pointers( mbedtls_ssl_context *ssl ) { /* This function sets the pointers to match the case - * of unprotected TLS/DTLS records, with both ssl->in_iv - * and ssl->in_msg pointing to the beginning of the record - * content. + * of unprotected TLS/DTLS records, with ssl->in_msg + * pointing to the beginning of the record content. * * When decrypting a protected record, ssl->in_msg * will be shifted to point to the beginning of the @@ -8014,7 +8013,7 @@ static void ssl_update_in_pointers( mbedtls_ssl_context *ssl ) #else /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ ssl->in_len = ssl->in_ctr + 8; #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ - ssl->in_iv = ssl->in_len + 2; + ssl->in_msg = ssl->in_len + 2; } MBEDTLS_SSL_TRANSPORT_ELSE #endif /* MBEDTLS_SSL_PROTO_DTLS */ @@ -8025,12 +8024,9 @@ static void ssl_update_in_pointers( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) ssl->in_cid = ssl->in_len; #endif - ssl->in_iv = ssl->in_hdr + 5; + ssl->in_msg = ssl->in_hdr + 5; } #endif /* MBEDTLS_SSL_PROTO_TLS */ - - /* This will be adjusted at record decryption time. */ - ssl->in_msg = ssl->in_iv; } /* @@ -8119,7 +8115,6 @@ error: ssl->in_hdr = NULL; ssl->in_ctr = NULL; ssl->in_len = NULL; - ssl->in_iv = NULL; ssl->in_msg = NULL; ssl->out_hdr = NULL; From 21fc61c7a71ebb7afe9b58697cdd29deab18f9d0 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 11:10:16 +0100 Subject: [PATCH 33/42] 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 40fe19fb7..37a3f74d8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4723,7 +4723,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 03e2db6f358a13b899db975368aba0dcbafa86b6 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Jul 2019 14:40:00 +0100 Subject: [PATCH 34/42] 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 | 66 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 61 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 37a3f74d8..3502f051a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -123,14 +123,69 @@ 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 defined(MBEDTLS_SSL_PROTO_TLS) + if( MBEDTLS_SSL_TRANSPORT_IS_TLS( ssl->conf->transport ) ) + { + ret = MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE; + goto exit; + } + MBEDTLS_SSL_TRANSPORT_ELSE +#endif /* MBEDTLS_SSL_PROTO_TLS */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + { + 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 */ @@ -4826,7 +4881,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 5579c5399b8414a9a7a6521b51b4f03676524f3a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 19 Jul 2019 10:27:58 +0100 Subject: [PATCH 35/42] Add x509_internal.h to cpp_dummy_build.cpp --- programs/test/cpp_dummy_build.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/programs/test/cpp_dummy_build.cpp b/programs/test/cpp_dummy_build.cpp index c65288404..3c9c2786f 100644 --- a/programs/test/cpp_dummy_build.cpp +++ b/programs/test/cpp_dummy_build.cpp @@ -97,6 +97,7 @@ #include "mbedtls/timing.h" #include "mbedtls/version.h" #include "mbedtls/x509.h" +#include "mbedtls/x509_internal.h" #include "mbedtls/x509_crl.h" #include "mbedtls/x509_crt.h" #include "mbedtls/x509_csr.h" From c1c173cadffccac5766bd69b8e2c557a988b5670 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 19 Jul 2019 10:59:12 +0100 Subject: [PATCH 36/42] 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 3502f051a..876941fc6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4978,21 +4978,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 e03eb7bb640ff4f33cec839e4b593489fc9e6ade Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 19 Jul 2019 15:43:09 +0100 Subject: [PATCH 37/42] 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 1ea58dfc9..f117a695a 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -7872,8 +7872,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 618176126cd073f5f0b9b22213875d486c99b3f6 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 25 Jul 2019 10:13:02 +0100 Subject: [PATCH 38/42] 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 876941fc6..33823380f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4794,14 +4794,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 7b5ba84624e57bfca0aaa8df63d0fee8b0ccb1a0 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 25 Jul 2019 10:16:37 +0100 Subject: [PATCH 39/42] 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 33823380f..47b1538c2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4935,12 +4935,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 8061c6e894293bb09d6926470392144c1f383612 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 26 Jul 2019 08:07:03 +0100 Subject: [PATCH 40/42] 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 47b1538c2..22ee6fc70 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4891,13 +4891,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 != mbedtls_ssl_get_major_ver( ssl ) ) { From ec014083890a70ce3bb93e797d4970e3ab98b970 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 26 Jul 2019 08:20:27 +0100 Subject: [PATCH 41/42] 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 22ee6fc70..6dd509ec6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4946,6 +4946,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 f3a15b3de078bceafdf0d46c5b0d47f8eee6dd46 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 42/42] 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 6dd509ec6..6187eb01a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4873,7 +4873,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