From 60a4299bbf5c1a51f6580941088e3970b6db60ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 24 May 2019 12:06:29 +0200 Subject: [PATCH 1/3] Add new ABI-independent format for serialization --- library/ssl_tls.c | 191 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 173 insertions(+), 18 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 4ad49f19f..876457c4f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8779,11 +8779,23 @@ const mbedtls_ssl_session *mbedtls_ssl_get_session_pointer( const mbedtls_ssl_co * Serialize a session in the following format: * (in the presentation language of TLS, RFC 8446 section 3) * - * opaque session_struct[n]; // n = sizeof(mbedtls_ssl_session) - * opaque peer_cert<0..2^24-1>; // 0 means no peer cert - * opaque ticket<0..2^24-1>; // 0 means no ticket + * uint64 start_time; + * uint8 ciphersuite[2]; // defined by the standard + * uint8 compression; // 0 or 1 + * uint8 session_id_len; // at most 32 + * opaque session_id[32]; + * opaque master[48]; // fixed length in the standard + * uint32 verify_result; + * opaque peer_cert<0..2^24-1>; // length 0 means no peer cert + * opaque ticket<0..2^24-1>; // length 0 means no ticket + * uint32 ticket_lifetime; + * uint8 mfl_code; // up to 255 according to standard + * uint8 trunc_hmac; // 0 or 1 + * uint8 encrypt_then_mac; // 0 or 1 * - * Only the peer's certificate is saved, not the whole chain. + * The order is the same as in the definition of the structure, except + * verify_result is put before peer_cert so that all mandatory fields come + * together in one block. */ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, unsigned char *buf, @@ -8792,23 +8804,66 @@ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, { unsigned char *p = buf; size_t used = 0; +#if defined(MBEDTLS_HAVE_TIME) + uint64_t start; +#endif #if defined(MBEDTLS_X509_CRT_PARSE_C) size_t cert_len; -#endif /* MBEDTLS_X509_CRT_PARSE_C */ +#endif /* - * Shallow copy of the session structure + * Time */ - used += sizeof( mbedtls_ssl_session ); +#if defined(MBEDTLS_HAVE_TIME) + used += 8; if( used <= buf_len ) { - memcpy( p, session, sizeof( mbedtls_ssl_session ) ); - p += sizeof( mbedtls_ssl_session ); + start = (uint64_t) session->start; + + *p++ = (unsigned char)( ( start >> 56 ) & 0xFF ); + *p++ = (unsigned char)( ( start >> 48 ) & 0xFF ); + *p++ = (unsigned char)( ( start >> 40 ) & 0xFF ); + *p++ = (unsigned char)( ( start >> 32 ) & 0xFF ); + *p++ = (unsigned char)( ( start >> 24 ) & 0xFF ); + *p++ = (unsigned char)( ( start >> 16 ) & 0xFF ); + *p++ = (unsigned char)( ( start >> 8 ) & 0xFF ); + *p++ = (unsigned char)( ( start ) & 0xFF ); + } +#endif /* MBEDTLS_HAVE_TIME */ + + /* + * Basic mandatory fields + */ + used += 2 /* ciphersuite */ + + 1 /* compression */ + + 1 /* id_len */ + + sizeof( session->id ) + + sizeof( session->master ) + + 4; /* verify_result */ + + if( used <= buf_len ) + { + *p++ = (unsigned char)( ( session->ciphersuite >> 8 ) & 0xFF ); + *p++ = (unsigned char)( ( session->ciphersuite ) & 0xFF ); + + *p++ = (unsigned char)( session->compression & 0xFF ); + + *p++ = (unsigned char)( session->id_len & 0xFF ); + memcpy( p, session->id, 32 ); + p += 32; + + memcpy( p, session->master, 48 ); + p += 48; + + *p++ = (unsigned char)( ( session->verify_result >> 24 ) & 0xFF ); + *p++ = (unsigned char)( ( session->verify_result >> 16 ) & 0xFF ); + *p++ = (unsigned char)( ( session->verify_result >> 8 ) & 0xFF ); + *p++ = (unsigned char)( ( session->verify_result ) & 0xFF ); } /* - * Copy of the peer's end-entity certificate + * Peer's end-entity certificate */ #if defined(MBEDTLS_X509_CRT_PARSE_C) if( session->peer_cert == NULL ) @@ -8833,10 +8888,10 @@ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, #endif /* MBEDTLS_X509_CRT_PARSE_C */ /* - * Copy of the session ticket if any + * Session ticket if any, plus associated data */ #if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) - used += 3 + session->ticket_len; + used += 3 + session->ticket_len + 4; /* len + ticket + lifetime */ if( used <= buf_len ) { @@ -8849,9 +8904,38 @@ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, memcpy( p, session->ticket, session->ticket_len ); p += session->ticket_len; } + + *p++ = (unsigned char)( ( session->ticket_lifetime >> 24 ) & 0xFF ); + *p++ = (unsigned char)( ( session->ticket_lifetime >> 16 ) & 0xFF ); + *p++ = (unsigned char)( ( session->ticket_lifetime >> 8 ) & 0xFF ); + *p++ = (unsigned char)( ( session->ticket_lifetime ) & 0xFF ); } #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ + /* + * Misc extension-related info + */ +#if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) + used += 1; + + if( used <= buf_len ) + *p++ = session->mfl_code; +#endif + +#if defined(MBEDTLS_SSL_TRUNCATED_HMAC) + used += 1; + + if( used <= buf_len ) + *p++ = (unsigned char)( ( session->trunc_hmac ) & 0xFF ); +#endif + +#if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) + used += 1; + + if( used <= buf_len ) + *p++ = (unsigned char)( ( session->encrypt_then_mac ) & 0xFF ); +#endif + /* Done */ *olen = used; @@ -8862,7 +8946,7 @@ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, } /* - * Unserialize session, see mbedtls_ssl_session_save(). + * Unserialize session, see mbedtls_ssl_session_save() for format. * * This internal version is wrapped by a public function that cleans up in * case of error. @@ -8873,18 +8957,56 @@ static int ssl_session_load( mbedtls_ssl_session *session, { const unsigned char *p = buf; const unsigned char * const end = buf + len; +#if defined(MBEDTLS_HAVE_TIME) + uint64_t start; +#endif #if defined(MBEDTLS_X509_CRT_PARSE_C) size_t cert_len; #endif /* MBEDTLS_X509_CRT_PARSE_C */ /* - * Shallow session structure + * Time */ - if( sizeof( mbedtls_ssl_session ) > (size_t)( end - p ) ) +#if defined(MBEDTLS_HAVE_TIME) + if( 8 > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - memcpy( session, p, sizeof( mbedtls_ssl_session ) ); - p += sizeof( mbedtls_ssl_session ); + start = ( (uint64_t) p[0] << 56 ) | + ( (uint64_t) p[1] << 48 ) | + ( (uint64_t) p[2] << 40 ) | + ( (uint64_t) p[3] << 32 ) | + ( (uint64_t) p[4] << 24 ) | + ( (uint64_t) p[5] << 16 ) | + ( (uint64_t) p[6] << 8 ) | + ( (uint64_t) p[7] ); + p += 8; + + session->start = (time_t) start; +#endif /* MBEDTLS_HAVE_TIME */ + + /* + * Basic mandatory fields + */ + if( 2 + 1 + 1 + 32 + 48 + 4 > (size_t)( end - p ) ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + + session->ciphersuite = ( p[0] << 8 ) | p[1]; + p += 2; + + session->compression = *p++; + + session->id_len = *p++; + memcpy( session->id, p, 32 ); + p += 32; + + memcpy( session->master, p, 48 ); + p += 48; + + session->verify_result = ( (uint32_t) p[0] << 24 ) | + ( (uint32_t) p[1] << 16 ) | + ( (uint32_t) p[2] << 8 ) | + ( (uint32_t) p[3] ); + p += 4; /* Immediately clear invalid pointer values that have been read, in case * we exit early before we replaced them with valid ones. */ @@ -8937,7 +9059,7 @@ static int ssl_session_load( mbedtls_ssl_session *session, #endif /* MBEDTLS_X509_CRT_PARSE_C */ /* - * Session ticket + * Session ticket and associated data */ #if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) if( 3 > (size_t)( end - p ) ) @@ -8958,8 +9080,41 @@ static int ssl_session_load( mbedtls_ssl_session *session, memcpy( session->ticket, p, session->ticket_len ); p += session->ticket_len; } + + if( 4 > (size_t)( end - p ) ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + + session->ticket_lifetime = ( (uint32_t) p[0] << 24 ) | + ( (uint32_t) p[1] << 16 ) | + ( (uint32_t) p[2] << 8 ) | + ( (uint32_t) p[3] ); + p += 4; #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ + /* + * Misc extension-related info + */ +#if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) + if( 1 > (size_t)( end - p ) ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + + session->mfl_code = *p++; +#endif + +#if defined(MBEDTLS_SSL_TRUNCATED_HMAC) + if( 1 > (size_t)( end - p ) ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + + session->trunc_hmac = *p++; +#endif + +#if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) + if( 1 > (size_t)( end - p ) ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + + session->encrypt_then_mac = *p++; +#endif + /* Done, should have consumed entire buffer */ if( p != end ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); From f8c355a012cd7f5f899bf14ffc2efadacce4e921 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 May 2019 10:21:30 +0200 Subject: [PATCH 2/3] Adapt buffering test to new ticket size The size of the ticket used in this test dropped from 192 to 143 bytes, so move all sizes used in this test down 50 bytes. Also, we now need to adapt the server response size as the default size would otherwise collide with the new mtu value. --- tests/scripts/all.sh | 2 +- tests/ssl-opt.sh | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 72e7c3e88..95d733683 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -702,7 +702,7 @@ component_test_small_ssl_dtls_max_buffering () { component_test_small_mbedtls_ssl_dtls_max_buffering () { msg "build: small MBEDTLS_SSL_DTLS_MAX_BUFFERING #1" - scripts/config.pl set MBEDTLS_SSL_DTLS_MAX_BUFFERING 240 + scripts/config.pl set MBEDTLS_SSL_DTLS_MAX_BUFFERING 190 CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . make diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index d6683b41e..da89642e3 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -7711,11 +7711,11 @@ run_test "DTLS reordering: Buffer encrypted Finished message" \ # without fragmentation or be reassembled within the bounds of # MBEDTLS_SSL_DTLS_MAX_BUFFERING. Achieve this by testing with a PSK-based # handshake, omitting CRTs. -requires_config_value_at_least "MBEDTLS_SSL_DTLS_MAX_BUFFERING" 240 -requires_config_value_at_most "MBEDTLS_SSL_DTLS_MAX_BUFFERING" 280 +requires_config_value_at_least "MBEDTLS_SSL_DTLS_MAX_BUFFERING" 190 +requires_config_value_at_most "MBEDTLS_SSL_DTLS_MAX_BUFFERING" 230 run_test "DTLS reordering: Buffer encrypted Finished message, drop for fragmented NewSessionTicket" \ -p "$P_PXY delay_srv=NewSessionTicket delay_srv=NewSessionTicket delay_ccs=1" \ - "$P_SRV mtu=190 dgram_packing=0 psk=abc123 psk_identity=foo cookies=0 dtls=1 debug_level=2" \ + "$P_SRV mtu=140 response_size=90 dgram_packing=0 psk=abc123 psk_identity=foo cookies=0 dtls=1 debug_level=2" \ "$P_CLI dgram_packing=0 dtls=1 debug_level=2 force_ciphersuite=TLS-PSK-WITH-AES-128-CCM-8 psk=abc123 psk_identity=foo" \ 0 \ -s "Buffer record from epoch 1" \ From c4f5080b3475247fd867adbcc0f9eefcc2873e12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 3 Jun 2019 10:53:47 +0200 Subject: [PATCH 3/3] Re-enable test that now works with new format Previously the test didn't work because of embedded pointer values that are not predictable. Now it works as we no longer serialize such values. --- 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 b13f882c3..1b5501848 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -724,7 +724,7 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_SEE_FUTURE_PR */ +/* BEGIN_CASE */ void ssl_serialize_session_load_save( int ticket_len, char *crt_file ) { mbedtls_ssl_session session;