From 2d5c72be0bd1e32bcf0fa06d23220002da554614 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Mon, 13 Sep 2021 07:30:09 +0000 Subject: [PATCH 1/9] TLS1.3: Add Encrypted Extensions Signed-off-by: XiaokangQian --- include/mbedtls/ssl.h | 1 + library/ssl_tls13_client.c | 124 +++++++++++++++++++++++++++++++++++- library/ssl_tls13_generic.c | 33 ++++++++++ 3 files changed, 155 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index fa2429d07c..6bdb7acd18 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -486,6 +486,7 @@ #define MBEDTLS_SSL_HS_SERVER_HELLO 2 #define MBEDTLS_SSL_HS_HELLO_VERIFY_REQUEST 3 #define MBEDTLS_SSL_HS_NEW_SESSION_TICKET 4 +#define MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION 8 // NEW IN TLS 1.3 #define MBEDTLS_SSL_HS_CERTIFICATE 11 #define MBEDTLS_SSL_HS_SERVER_KEY_EXCHANGE 12 #define MBEDTLS_SSL_HS_CERTIFICATE_REQUEST 13 diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 979db31449..686edfe3b9 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1395,11 +1395,125 @@ cleanup: } /* - * Handler for MBEDTLS_SSL_ENCRYPTED_EXTENSIONS + * + * EncryptedExtensions message + * + * The EncryptedExtensions message contains any extensions which + * should be protected, i.e., any which are not needed to establish + * the cryptographic context. */ -static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ) + +/* + * Overview + */ +static int ssl_tls1_3_read_encrypted_extensions( mbedtls_ssl_context *ssl ); + +/* Main entry point; orchestrates the other functions */ +static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ); + +static int ssl_tls13_encrypted_extensions_parse( mbedtls_ssl_context *ssl, + const unsigned char *buf, + size_t buf_len ); +static int ssl_tls13_encrypted_extensions_postprocess( mbedtls_ssl_context *ssl ); + +/* + * Handler for MBEDTLS_SSL_ENCRYPTED_ENTENSIONS + */ +static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ) +{ + int ret; + unsigned char *buf; + size_t buf_len; + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse encrypted extensions" ) ); + + MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( ssl, + MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, + &buf, &buf_len ) ); + + /* Process the message contents */ + MBEDTLS_SSL_PROC_CHK( ssl_tls13_encrypted_extensions_parse( ssl, buf, buf_len ) ); + + mbedtls_ssl_tls13_add_hs_msg_to_checksum( + ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, buf, buf_len ); + + MBEDTLS_SSL_PROC_CHK( ssl_tls13_encrypted_extensions_postprocess( ssl ) ); + +cleanup: + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse encrypted extensions" ) ); + return( ret ); + +} + +static int ssl_tls13_encrypted_extensions_parse( mbedtls_ssl_context *ssl, + const unsigned char *buf, + size_t buf_len ) +{ + int ret = 0; + size_t ext_len; + const unsigned char *ext; + + if( buf_len < 2 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension message too short" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + ext_len = MBEDTLS_GET_UINT16_BE(buf, 0); + + buf += 2; /* skip extension length */ + ext = buf; + + /* Checking for an extension length that is too short */ + if( ext_len > 0 && ext_len < 4 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension message too short" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + /* Checking for an extension length that isn't aligned with the rest + * of the message */ + if( buf_len != 2 + ext_len ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", ext, ext_len ); + + while( ext_len ) + { + unsigned int ext_id = MBEDTLS_GET_UINT16_BE(ext, 0); + size_t ext_size = MBEDTLS_GET_UINT16_BE(ext, 2); + + if( ext_size + 4 > ext_len ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + /* TBD: The client MUST check EncryptedExtensions for the + * presence of any forbidden extensions and if any are found MUST abort + * the handshake with an "illegal_parameter" alert. + */ + ((void) ext_id); + + ext_len -= 4 + ext_size; + ext += 4 + ext_size; + + if( ext_len > 0 && ext_len < 4 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + } + + return( ret ); +} + +static int ssl_tls13_encrypted_extensions_postprocess( mbedtls_ssl_context *ssl ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "%s hasn't been implemented", __func__ ) ); mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CERTIFICATE_REQUEST ); return( 0 ); } @@ -1555,6 +1669,10 @@ int mbedtls_ssl_tls13_handshake_client_step( mbedtls_ssl_context *ssl ) ret = ssl_tls1_3_handshake_wrapup( ssl ); break; + case MBEDTLS_SSL_ENCRYPTED_EXTENSIONS: + ret = ssl_tls13_encrypted_extensions_process( ssl ); + break; + default: MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid state %d", ssl->state ) ); return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index b3a4a09ddc..949fa74741 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -27,6 +27,39 @@ #include "mbedtls/debug.h" #include "ssl_misc.h" +#include + +int mbedtls_ssl_tls13_fetch_handshake_msg( mbedtls_ssl_context *ssl, + unsigned hs_type, + unsigned char **buf, + size_t *buflen ) +{ + int ret; + + if( ( ret = mbedtls_ssl_read_record( ssl, 0 ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); + goto cleanup; + } + + if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE || + ssl->in_msg[0] != hs_type ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate verify message" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE, + MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); + ret = MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE; + goto cleanup; + } + + *buf = ssl->in_msg + 4; + *buflen = ssl->in_hslen - 4; + + +cleanup: + + return( ret ); +} int mbedtls_ssl_tls1_3_fetch_handshake_msg( mbedtls_ssl_context *ssl, unsigned hs_type, From c1fe000cfd8cca1d9b6994519bee84bdfa0bb608 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Thu, 16 Sep 2021 03:02:14 +0000 Subject: [PATCH 2/9] TLS1.3: Solve check name issue-macro definition Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 686edfe3b9..a27f7239dd 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1417,7 +1417,7 @@ static int ssl_tls13_encrypted_extensions_parse( mbedtls_ssl_context *ssl, static int ssl_tls13_encrypted_extensions_postprocess( mbedtls_ssl_context *ssl ); /* - * Handler for MBEDTLS_SSL_ENCRYPTED_ENTENSIONS + * Handler for MBEDTLS_SSL_ENCRYPTED_EXTENSIONS */ static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ) { From e87e5924c9e8bf4bd6c08bb4ed375afd0d954acb Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Fri, 24 Sep 2021 07:35:32 +0000 Subject: [PATCH 3/9] Fix some issues such as naming mismatch based on comments. Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 25 ++++++++++--------------- library/ssl_tls13_generic.c | 32 -------------------------------- 2 files changed, 10 insertions(+), 47 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index a27f7239dd..00c1835dd3 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1406,20 +1406,19 @@ cleanup: /* * Overview */ -static int ssl_tls1_3_read_encrypted_extensions( mbedtls_ssl_context *ssl ); /* Main entry point; orchestrates the other functions */ -static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ); +static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ); -static int ssl_tls13_encrypted_extensions_parse( mbedtls_ssl_context *ssl, +static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, const unsigned char *buf, size_t buf_len ); -static int ssl_tls13_encrypted_extensions_postprocess( mbedtls_ssl_context *ssl ); +static int ssl_tls1_3_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ); /* * Handler for MBEDTLS_SSL_ENCRYPTED_EXTENSIONS */ -static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ) +static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ) { int ret; unsigned char *buf; @@ -1427,17 +1426,17 @@ static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse encrypted extensions" ) ); - MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( ssl, + MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls1_3_fetch_handshake_msg( ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, &buf, &buf_len ) ); /* Process the message contents */ - MBEDTLS_SSL_PROC_CHK( ssl_tls13_encrypted_extensions_parse( ssl, buf, buf_len ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls1_3_parse_encrypted_extensions( ssl, buf, buf_len ) ); - mbedtls_ssl_tls13_add_hs_msg_to_checksum( + mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, buf, buf_len ); - MBEDTLS_SSL_PROC_CHK( ssl_tls13_encrypted_extensions_postprocess( ssl ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls1_3_postprocess_encrypted_extensions( ssl ) ); cleanup: @@ -1446,7 +1445,7 @@ cleanup: } -static int ssl_tls13_encrypted_extensions_parse( mbedtls_ssl_context *ssl, +static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, const unsigned char *buf, size_t buf_len ) { @@ -1512,7 +1511,7 @@ static int ssl_tls13_encrypted_extensions_parse( mbedtls_ssl_context *ssl, return( ret ); } -static int ssl_tls13_encrypted_extensions_postprocess( mbedtls_ssl_context *ssl ) +static int ssl_tls1_3_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ) { mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CERTIFICATE_REQUEST ); return( 0 ); @@ -1669,10 +1668,6 @@ int mbedtls_ssl_tls13_handshake_client_step( mbedtls_ssl_context *ssl ) ret = ssl_tls1_3_handshake_wrapup( ssl ); break; - case MBEDTLS_SSL_ENCRYPTED_EXTENSIONS: - ret = ssl_tls13_encrypted_extensions_process( ssl ); - break; - default: MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid state %d", ssl->state ) ); return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 949fa74741..70c2b02103 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -29,38 +29,6 @@ #include "ssl_misc.h" #include -int mbedtls_ssl_tls13_fetch_handshake_msg( mbedtls_ssl_context *ssl, - unsigned hs_type, - unsigned char **buf, - size_t *buflen ) -{ - int ret; - - if( ( ret = mbedtls_ssl_read_record( ssl, 0 ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); - goto cleanup; - } - - if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE || - ssl->in_msg[0] != hs_type ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate verify message" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE, - MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); - ret = MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE; - goto cleanup; - } - - *buf = ssl->in_msg + 4; - *buflen = ssl->in_hslen - 4; - - -cleanup: - - return( ret ); -} - int mbedtls_ssl_tls1_3_fetch_handshake_msg( mbedtls_ssl_context *ssl, unsigned hs_type, unsigned char **buf, From 140f0459ed6f99d88b96d5451dc46c9abf0e1aa0 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Fri, 8 Oct 2021 08:05:53 +0000 Subject: [PATCH 4/9] Encrypted Extension: Align the code style of buffer pointer Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 45 ++++++++++++++++++++----------------- library/ssl_tls13_generic.c | 1 - 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 00c1835dd3..acdfa0542e 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1450,45 +1450,46 @@ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, size_t buf_len ) { int ret = 0; - size_t ext_len; - const unsigned char *ext; + size_t p_ext_len; + const unsigned char *end = buf + buf_len; + const unsigned char *p = buf; - if( buf_len < 2 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension message too short" ) ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2); + p_ext_len = MBEDTLS_GET_UINT16_BE(buf, 0); - ext_len = MBEDTLS_GET_UINT16_BE(buf, 0); - - buf += 2; /* skip extension length */ - ext = buf; + p += 2; /* skip extension length */ /* Checking for an extension length that is too short */ - if( ext_len > 0 && ext_len < 4 ) + if( p_ext_len > 0 && p_ext_len < 4 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension message too short" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ + MBEDTLS_ERR_SSL_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } /* Checking for an extension length that isn't aligned with the rest * of the message */ - if( buf_len != 2 + ext_len ) + if( buf_len != 2 + p_ext_len ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ + MBEDTLS_ERR_SSL_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } - MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", ext, ext_len ); + MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", p, p_ext_len ); - while( ext_len ) + while( p_ext_len ) { - unsigned int ext_id = MBEDTLS_GET_UINT16_BE(ext, 0); - size_t ext_size = MBEDTLS_GET_UINT16_BE(ext, 2); + unsigned int ext_id = MBEDTLS_GET_UINT16_BE(p, 0); + size_t ext_size = MBEDTLS_GET_UINT16_BE(p, 2); - if( ext_size + 4 > ext_len ) + if( ext_size + 4 > p_ext_len ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ + MBEDTLS_ERR_SSL_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } @@ -1498,12 +1499,14 @@ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, */ ((void) ext_id); - ext_len -= 4 + ext_size; - ext += 4 + ext_size; + p_ext_len -= 4 + ext_size; + p += 4 + ext_size; - if( ext_len > 0 && ext_len < 4 ) + if( p_ext_len > 0 && p_ext_len < 4 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ + MBEDTLS_ERR_SSL_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } } diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 70c2b02103..b3a4a09ddc 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -27,7 +27,6 @@ #include "mbedtls/debug.h" #include "ssl_misc.h" -#include int mbedtls_ssl_tls1_3_fetch_handshake_msg( mbedtls_ssl_context *ssl, unsigned hs_type, From 08da26c58f959ba4e93498d3ed626717b5ee1cdf Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Sat, 9 Oct 2021 10:12:11 +0000 Subject: [PATCH 5/9] Refine encrypted extensions parse function Change arguments of API. Send different messages base on extensions types. Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 83 +++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 36 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index acdfa0542e..13fb47079c 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1411,8 +1411,8 @@ cleanup: static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ); static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, - const unsigned char *buf, - size_t buf_len ); + const unsigned char *buf, + const unsigned char *end ); static int ssl_tls1_3_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ); /* @@ -1431,7 +1431,7 @@ static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ) &buf, &buf_len ) ); /* Process the message contents */ - MBEDTLS_SSL_PROC_CHK( ssl_tls1_3_parse_encrypted_extensions( ssl, buf, buf_len ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls1_3_parse_encrypted_extensions( ssl, buf, ( buf + buf_len ) ) ); mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, buf, buf_len ); @@ -1445,32 +1445,27 @@ cleanup: } +/* Parse EncryptedExtensions message + * struct { + * Extension extensions<0..2^16-1>; + * } EncryptedExtensions; + */ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, - const unsigned char *buf, - size_t buf_len ) + const unsigned char *buf, + const unsigned char *end ) { int ret = 0; - size_t p_ext_len; - const unsigned char *end = buf + buf_len; + size_t extensions_len; const unsigned char *p = buf; - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2); - p_ext_len = MBEDTLS_GET_UINT16_BE(buf, 0); + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); + extensions_len = MBEDTLS_GET_UINT16_BE(p, 0); - p += 2; /* skip extension length */ - - /* Checking for an extension length that is too short */ - if( p_ext_len > 0 && p_ext_len < 4 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension message too short" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ - MBEDTLS_ERR_SSL_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } + p += 2; /* Checking for an extension length that isn't aligned with the rest * of the message */ - if( buf_len != 2 + p_ext_len ) + if( p + extensions_len != end ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ @@ -1478,31 +1473,47 @@ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } - MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", p, p_ext_len ); + MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", p, extensions_len ); - while( p_ext_len ) + while( p < end ) { - unsigned int ext_id = MBEDTLS_GET_UINT16_BE(p, 0); - size_t ext_size = MBEDTLS_GET_UINT16_BE(p, 2); + unsigned int extension_type; + size_t extension_data_len; - if( ext_size + 4 > p_ext_len ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ - MBEDTLS_ERR_SSL_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } + /* + * struct { + * ExtensionType extension_type; (2 bytes) + * opaque extension_data<0..2^16-1>; + * } Extension; + */ + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 4 ); + extension_type = MBEDTLS_GET_UINT16_BE( p, 0 ); + extension_data_len = MBEDTLS_GET_UINT16_BE( p, 2 ); + p += 4; + + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, extension_data_len ); /* TBD: The client MUST check EncryptedExtensions for the * presence of any forbidden extensions and if any are found MUST abort - * the handshake with an "illegal_parameter" alert. + * the handshake with an "unsupported_extension" alert. */ - ((void) ext_id); + switch( extension_type ) + { - p_ext_len -= 4 + ext_size; - p += 4 + ext_size; + case MBEDTLS_TLS_EXT_SUPPORTED_GROUPS: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found extensions supported groups" ) ); + break; - if( p_ext_len > 0 && p_ext_len < 4 ) + default: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "unsupported extension found: %d ( ignoring )", extension_type) ); + break; + } + + extensions_len -= 4 + extension_data_len; + p += extension_data_len; + + /* Checking for an extension length that is too short */ + if( extensions_len > 0 && extensions_len < 4 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ From 97799ac27bb095d1966aa0abce294cb076fa7706 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Mon, 11 Oct 2021 10:05:54 +0000 Subject: [PATCH 6/9] Encrypted Extensions: Align code style and some check logic Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 74 ++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 40 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 13fb47079c..f7f7eaabac 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1408,17 +1408,17 @@ cleanup: */ /* Main entry point; orchestrates the other functions */ -static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ); +static int ssl_tls13_process_encrypted_extensions( mbedtls_ssl_context *ssl ); -static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, - const unsigned char *buf, - const unsigned char *end ); -static int ssl_tls1_3_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ); +static int ssl_tls13_parse_encrypted_extensions( mbedtls_ssl_context *ssl, + const unsigned char *buf, + const unsigned char *end ); +static int ssl_tls13_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ); /* * Handler for MBEDTLS_SSL_ENCRYPTED_EXTENSIONS */ -static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ) +static int ssl_tls13_process_encrypted_extensions( mbedtls_ssl_context *ssl ) { int ret; unsigned char *buf; @@ -1431,12 +1431,13 @@ static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ) &buf, &buf_len ) ); /* Process the message contents */ - MBEDTLS_SSL_PROC_CHK( ssl_tls1_3_parse_encrypted_extensions( ssl, buf, ( buf + buf_len ) ) ); + MBEDTLS_SSL_PROC_CHK( + ssl_tls13_parse_encrypted_extensions( ssl, buf, ( buf + buf_len ) ) ); mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, buf, buf_len ); - MBEDTLS_SSL_PROC_CHK( ssl_tls1_3_postprocess_encrypted_extensions( ssl ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls13_postprocess_encrypted_extensions( ssl ) ); cleanup: @@ -1447,33 +1448,22 @@ cleanup: /* Parse EncryptedExtensions message * struct { - * Extension extensions<0..2^16-1>; + * Extension extensions<0..2^16-1>; * } EncryptedExtensions; */ -static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, - const unsigned char *buf, - const unsigned char *end ) +static int ssl_tls13_parse_encrypted_extensions( mbedtls_ssl_context *ssl, + const unsigned char *buf, + const unsigned char *end ) { int ret = 0; size_t extensions_len; const unsigned char *p = buf; MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); - extensions_len = MBEDTLS_GET_UINT16_BE(p, 0); - + extensions_len = MBEDTLS_GET_UINT16_BE( p, 0 ); p += 2; - /* Checking for an extension length that isn't aligned with the rest - * of the message */ - if( p + extensions_len != end ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ - MBEDTLS_ERR_SSL_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } - - MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", p, extensions_len ); + MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions", p, extensions_len ); while( p < end ) { @@ -1482,8 +1472,8 @@ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, /* * struct { - * ExtensionType extension_type; (2 bytes) - * opaque extension_data<0..2^16-1>; + * ExtensionType extension_type; (2 bytes) + * opaque extension_data<0..2^16-1>; * } Extension; */ MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 4 ); @@ -1493,7 +1483,7 @@ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, extension_data_len ); - /* TBD: The client MUST check EncryptedExtensions for the + /* The client MUST check EncryptedExtensions for the * presence of any forbidden extensions and if any are found MUST abort * the handshake with an "unsupported_extension" alert. */ @@ -1505,27 +1495,31 @@ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, break; default: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "unsupported extension found: %d ( ignoring )", extension_type) ); + MBEDTLS_SSL_DEBUG_MSG( + 3, ( "unsupported extension found: %u ", extension_type) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT, \ + MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); + return ( MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); break; } - extensions_len -= 4 + extension_data_len; p += extension_data_len; + } - /* Checking for an extension length that is too short */ - if( extensions_len > 0 && extensions_len < 4 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ - MBEDTLS_ERR_SSL_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } + /* Check that we consumed all the message. */ + if( p != end ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ + MBEDTLS_ERR_SSL_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } return( ret ); } -static int ssl_tls1_3_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ) +static int ssl_tls13_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ) { mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CERTIFICATE_REQUEST ); return( 0 ); @@ -1643,7 +1637,7 @@ int mbedtls_ssl_tls13_handshake_client_step( mbedtls_ssl_context *ssl ) break; case MBEDTLS_SSL_ENCRYPTED_EXTENSIONS: - ret = ssl_tls1_3_process_encrypted_extensions( ssl ); + ret = ssl_tls13_process_encrypted_extensions( ssl ); break; case MBEDTLS_SSL_CERTIFICATE_REQUEST: From 8db25fffb48ca5c7c5a67d288451428e1652da8a Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Wed, 13 Oct 2021 05:56:18 +0000 Subject: [PATCH 7/9] Encrypted Extensions: Change extensions length check Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index f7f7eaabac..68f5ae568a 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1432,7 +1432,7 @@ static int ssl_tls13_process_encrypted_extensions( mbedtls_ssl_context *ssl ) /* Process the message contents */ MBEDTLS_SSL_PROC_CHK( - ssl_tls13_parse_encrypted_extensions( ssl, buf, ( buf + buf_len ) ) ); + ssl_tls13_parse_encrypted_extensions( ssl, buf, buf + buf_len ) ); mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, buf, buf_len ); @@ -1458,14 +1458,17 @@ static int ssl_tls13_parse_encrypted_extensions( mbedtls_ssl_context *ssl, int ret = 0; size_t extensions_len; const unsigned char *p = buf; + const unsigned char *extensions_end; MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); extensions_len = MBEDTLS_GET_UINT16_BE( p, 0 ); p += 2; MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions", p, extensions_len ); + extensions_end = p + extensions_len; + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, extensions_len ); - while( p < end ) + while( p < extensions_end ) { unsigned int extension_type; size_t extension_data_len; @@ -1476,12 +1479,12 @@ static int ssl_tls13_parse_encrypted_extensions( mbedtls_ssl_context *ssl, * opaque extension_data<0..2^16-1>; * } Extension; */ - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 4 ); + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, 4 ); extension_type = MBEDTLS_GET_UINT16_BE( p, 0 ); extension_data_len = MBEDTLS_GET_UINT16_BE( p, 2 ); p += 4; - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, extension_data_len ); + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, extension_data_len ); /* The client MUST check EncryptedExtensions for the * presence of any forbidden extensions and if any are found MUST abort @@ -1501,18 +1504,17 @@ static int ssl_tls13_parse_encrypted_extensions( mbedtls_ssl_context *ssl, MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT, \ MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); return ( MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); - break; } p += extension_data_len; } /* Check that we consumed all the message. */ - if( p != end ) + if( p != extensions_end ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ - MBEDTLS_ERR_SSL_DECODE_ERROR ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, \ + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } From 7b2d4efee8b8d35b783ddf3a3bb7c263f2c04da0 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Wed, 13 Oct 2021 10:19:02 +0000 Subject: [PATCH 8/9] Change the buffer boundary check and alert type Signed-off-by: XiaokangQian --- include/mbedtls/ssl.h | 2 +- library/ssl_tls13_client.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 6bdb7acd18..288d9b3c5c 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -486,7 +486,7 @@ #define MBEDTLS_SSL_HS_SERVER_HELLO 2 #define MBEDTLS_SSL_HS_HELLO_VERIFY_REQUEST 3 #define MBEDTLS_SSL_HS_NEW_SESSION_TICKET 4 -#define MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION 8 // NEW IN TLS 1.3 +#define MBEDTLS_SSL_HS_ENCRYPTED_EXTENSIONS 8 // NEW IN TLS 1.3 #define MBEDTLS_SSL_HS_CERTIFICATE 11 #define MBEDTLS_SSL_HS_SERVER_KEY_EXCHANGE 12 #define MBEDTLS_SSL_HS_CERTIFICATE_REQUEST 13 diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 68f5ae568a..2c2d0f3afd 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1510,11 +1510,11 @@ static int ssl_tls13_parse_encrypted_extensions( mbedtls_ssl_context *ssl, } /* Check that we consumed all the message. */ - if( p != extensions_end ) + if( p != end ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, \ - MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ + MBEDTLS_ERR_SSL_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } From ab7f50d6389b9f6a6e2d60881d68c28f67126bf5 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Thu, 21 Oct 2021 06:23:29 +0000 Subject: [PATCH 9/9] Change macro names and add test script for extensions Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 4 ++-- tests/ssl-opt.sh | 11 ++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 2c2d0f3afd..5ed01aade2 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1427,7 +1427,7 @@ static int ssl_tls13_process_encrypted_extensions( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse encrypted extensions" ) ); MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls1_3_fetch_handshake_msg( ssl, - MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, + MBEDTLS_SSL_HS_ENCRYPTED_EXTENSIONS, &buf, &buf_len ) ); /* Process the message contents */ @@ -1435,7 +1435,7 @@ static int ssl_tls13_process_encrypted_extensions( mbedtls_ssl_context *ssl ) ssl_tls13_parse_encrypted_extensions( ssl, buf, buf + buf_len ) ); mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( - ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, buf, buf_len ); + ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSIONS, buf, buf_len ); MBEDTLS_SSL_PROC_CHK( ssl_tls13_postprocess_encrypted_extensions( ssl ) ); diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 8fbe67739b..f9bfec2e1e 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -8808,7 +8808,7 @@ requires_openssl_tls1_3 requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL requires_config_disabled MBEDTLS_USE_PSA_CRYPTO run_test "TLS1.3: Test client hello msg work - openssl" \ - "$O_NEXT_SRV -tls1_3 -msg" \ + "$O_NEXT_SRV -tls1_3 -msg -no_middlebox" \ "$P_CLI debug_level=3 min_version=tls1_3 max_version=tls1_3" \ 1 \ -c "SSL - The requested feature is not available" \ @@ -8828,13 +8828,14 @@ run_test "TLS1.3: Test client hello msg work - openssl" \ -c "<= ssl_tls1_3_process_server_hello" \ -c "server hello, chosen ciphersuite: ( 1301 ) - TLS1-3-AES-128-GCM-SHA256" \ -c "ECDH curve: x25519" \ - -c "=> ssl_tls1_3_process_server_hello" + -c "=> ssl_tls1_3_process_server_hello" \ + -c "<= parse encrypted extensions" requires_gnutls_tls1_3 requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL requires_config_disabled MBEDTLS_USE_PSA_CRYPTO run_test "TLS1.3: Test client hello msg work - gnutls" \ - "$G_NEXT_SRV --priority=NORMAL:-VERS-ALL:+VERS-TLS1.3 --debug=4" \ + "$G_NEXT_SRV --priority=NORMAL:-VERS-ALL:+VERS-TLS1.3:%DISABLE_TLS13_COMPAT_MODE --debug=4" \ "$P_CLI debug_level=3 min_version=tls1_3 max_version=tls1_3" \ 1 \ -c "SSL - The requested feature is not available" \ @@ -8854,8 +8855,8 @@ run_test "TLS1.3: Test client hello msg work - gnutls" \ -c "<= ssl_tls1_3_process_server_hello" \ -c "server hello, chosen ciphersuite: ( 1301 ) - TLS1-3-AES-128-GCM-SHA256" \ -c "ECDH curve: x25519" \ - -c "=> ssl_tls1_3_process_server_hello" - + -c "=> ssl_tls1_3_process_server_hello" \ + -c "<= parse encrypted extensions" # Test heap memory usage after handshake requires_config_enabled MBEDTLS_MEMORY_DEBUG