From e36fdd676cae36ffebf68d74fcf3e34269bec74f Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Wed, 17 Aug 2022 21:31:36 +0800 Subject: [PATCH 1/4] Change signature of tls13_session_save Signed-off-by: Jerry Yu --- library/ssl_tls.c | 48 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 19b8a4135..66bbf122a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1898,7 +1898,7 @@ mbedtls_ssl_mode_t mbedtls_ssl_get_mode_from_ciphersuite( * struct { * uint64 ticket_received; * uint32 ticket_lifetime; - * opaque ticket<0..2^16>; + * opaque ticket<0..2^16-1>; * } ClientOnlyData; * * struct { @@ -1915,9 +1915,11 @@ mbedtls_ssl_mode_t mbedtls_ssl_get_mode_from_ciphersuite( * */ #if defined(MBEDTLS_SSL_SESSION_TICKETS) -static size_t ssl_tls13_session_save( const mbedtls_ssl_session *session, - unsigned char *buf, - size_t buf_len ) +MBEDTLS_CHECK_RETURN_CRITICAL +static int ssl_tls13_session_save( const mbedtls_ssl_session *session, + unsigned char *buf, + size_t buf_len, + size_t *olen ) { unsigned char *p = buf; size_t needed = 1 /* endpoint */ @@ -1925,7 +1927,7 @@ static size_t ssl_tls13_session_save( const mbedtls_ssl_session *session, + 4 /* ticket_age_add */ + 2 /* resumption_key length */ + session->resumption_key_len; /* resumption_key */ - + *olen = 0; #if defined(MBEDTLS_HAVE_TIME) needed += 8; /* start_time or ticket_received */ #endif @@ -1934,13 +1936,18 @@ static size_t ssl_tls13_session_save( const mbedtls_ssl_session *session, if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { needed += 4 /* ticket_lifetime */ - + 2 /* ticket_len */ - + session->ticket_len; /* ticket */ + + 2; /* ticket_len */ + if( session->ticket_len > SIZE_MAX - needed ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + needed += session->ticket_len; /* ticket */ } #endif /* MBEDTLS_SSL_CLI_C */ + *olen = needed; if( needed > buf_len ) - return( needed ); + { + return( MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL ); + } p[0] = session->endpoint; MBEDTLS_PUT_UINT16_BE( session->ciphersuite, p, 1 ); @@ -1980,7 +1987,7 @@ static size_t ssl_tls13_session_save( const mbedtls_ssl_session *session, } } #endif /* MBEDTLS_SSL_CLI_C */ - return( needed ); + return( 0 ); } MBEDTLS_CHECK_RETURN_CRITICAL @@ -2056,14 +2063,17 @@ static int ssl_tls13_session_load( mbedtls_ssl_session *session, } #else /* MBEDTLS_SSL_SESSION_TICKETS */ -static size_t ssl_tls13_session_save( const mbedtls_ssl_session *session, - unsigned char *buf, - size_t buf_len ) +MBEDTLS_CHECK_RETURN_CRITICAL +static int ssl_tls13_session_save( const mbedtls_ssl_session *session, + unsigned char *buf, + size_t buf_len, + size_t *olen ) { ((void) session); ((void) buf); ((void) buf_len); - return( 0 ); + *olen = 0; + return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); } static int ssl_tls13_session_load( const mbedtls_ssl_session *session, @@ -3007,7 +3017,10 @@ static int ssl_session_save( const mbedtls_ssl_session *session, unsigned char *p = buf; size_t used = 0; size_t remaining_len; - +#if defined(MBEDTLS_SSL_PROTO_TLS1_3) + size_t out_len; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; +#endif if( session == NULL ) return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); @@ -3047,7 +3060,12 @@ static int ssl_session_save( const mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_PROTO_TLS1_3) case MBEDTLS_SSL_VERSION_TLS1_3: - used += ssl_tls13_session_save( session, p, remaining_len ); + ret = ssl_tls13_session_save( session, p, remaining_len, &out_len ); + if( ret != 0 && ret != MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL ) + { + return( ret ); + } + used += out_len; break; #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ From 3419107e8d347dc2d5de4aed0b3a6109e8eef5cf Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Thu, 18 Aug 2022 10:32:09 +0800 Subject: [PATCH 2/4] Add checks for ticket and resumption_key fields From RFC 8446 and the definition of session, we should check the length. Signed-off-by: Jerry Yu --- library/ssl_tls.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 66bbf122a..266cf77b1 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1898,7 +1898,7 @@ mbedtls_ssl_mode_t mbedtls_ssl_get_mode_from_ciphersuite( * struct { * uint64 ticket_received; * uint32 ticket_lifetime; - * opaque ticket<0..2^16-1>; + * opaque ticket<1..2^16-1>; * } ClientOnlyData; * * struct { @@ -1925,9 +1925,14 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, size_t needed = 1 /* endpoint */ + 2 /* ciphersuite */ + 4 /* ticket_age_add */ - + 2 /* resumption_key length */ - + session->resumption_key_len; /* resumption_key */ + + 1 /* ticket_flags */ + + 1; /* resumption_key length */ *olen = 0; + + if( session->resumption_key_len > MBEDTLS_SSL_TLS1_3_TICKET_RESUMPTION_KEY_LEN ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + needed += session->resumption_key_len; /* resumption_key */ + #if defined(MBEDTLS_HAVE_TIME) needed += 8; /* start_time or ticket_received */ #endif @@ -1937,8 +1942,13 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, { needed += 4 /* ticket_lifetime */ + 2; /* ticket_len */ + + /* Check size_t overflow */ if( session->ticket_len > SIZE_MAX - needed ) + { return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + } + needed += session->ticket_len; /* ticket */ } #endif /* MBEDTLS_SSL_CLI_C */ @@ -1980,7 +1990,8 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, MBEDTLS_PUT_UINT16_BE( session->ticket_len, p, 0 ); p += 2; - if( session->ticket_len > 0 ) + + if( session->ticket != NULL && session->ticket_len > 0 ) { memcpy( p, session->ticket, session->ticket_len ); p += session->ticket_len; From 5b7c7caee6488082919d4b047fa77eec89381562 Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Sat, 23 Jul 2022 10:45:12 +0800 Subject: [PATCH 3/4] fix wrong condition issues Signed-off-by: Jerry Yu --- tests/suites/test_suite_ssl.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 33e0bdb0e..7f78e4d43 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -4809,7 +4809,7 @@ void ssl_serialize_session_save_load( int ticket_len, char *crt_file, original.resumption_key_len ) == 0 ); } #if defined(MBEDTLS_HAVE_TIME) && defined(MBEDTLS_SSL_SRV_C) - if( endpoint_type == MBEDTLS_SSL_IS_CLIENT) + if( endpoint_type == MBEDTLS_SSL_IS_SERVER) { TEST_ASSERT( original.start == restored.start ); } From e28d9745a12b32243fb0c292f98000e95303029f Mon Sep 17 00:00:00 2001 From: Jerry Yu Date: Thu, 18 Aug 2022 15:44:03 +0800 Subject: [PATCH 4/4] fix coding style issues Signed-off-by: Jerry Yu --- library/ssl_tls.c | 8 +------- tests/suites/test_suite_ssl.function | 2 +- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 266cf77b1..c45a1b84c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1945,19 +1945,15 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, /* Check size_t overflow */ if( session->ticket_len > SIZE_MAX - needed ) - { return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - } - needed += session->ticket_len; /* ticket */ + needed += session->ticket_len; /* ticket */ } #endif /* MBEDTLS_SSL_CLI_C */ *olen = needed; if( needed > buf_len ) - { return( MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL ); - } p[0] = session->endpoint; MBEDTLS_PUT_UINT16_BE( session->ciphersuite, p, 1 ); @@ -3073,9 +3069,7 @@ static int ssl_session_save( const mbedtls_ssl_session *session, case MBEDTLS_SSL_VERSION_TLS1_3: ret = ssl_tls13_session_save( session, p, remaining_len, &out_len ); if( ret != 0 && ret != MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL ) - { return( ret ); - } used += out_len; break; #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 7f78e4d43..145591dfa 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -4809,7 +4809,7 @@ void ssl_serialize_session_save_load( int ticket_len, char *crt_file, original.resumption_key_len ) == 0 ); } #if defined(MBEDTLS_HAVE_TIME) && defined(MBEDTLS_SSL_SRV_C) - if( endpoint_type == MBEDTLS_SSL_IS_SERVER) + if( endpoint_type == MBEDTLS_SSL_IS_SERVER ) { TEST_ASSERT( original.start == restored.start ); }