From 4a0637981badccd9cf4a2e47bf46e37c1520d8ff Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 21 Oct 2020 15:08:44 +0200 Subject: [PATCH 1/4] Refactor the variable I/O buffer size feature Reduce code duplication to simplify the feature and reduce code size. Signed-off-by: Andrzej Kurek --- library/ssl_tls.c | 185 +++++++++++++++++----------------------------- 1 file changed, 68 insertions(+), 117 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a1a5859f0..4a57e769a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -260,6 +260,70 @@ static int resize_buffer( unsigned char **buffer, size_t len_new, size_t *len_ol return 0; } + +static void handle_buffer_resizing( mbedtls_ssl_context *ssl, int downsizing, + uint32_t in_buf_new_len, + uint32_t out_buf_new_len ) +{ + int modified = 0; + size_t written_in = 0, iv_offset_in = 0, len_offset_in = 0; + size_t written_out = 0, iv_offset_out = 0, len_offset_out = 0; + if( ssl->in_buf != NULL ) + { + written_in = ssl->in_msg - ssl->in_buf; + iv_offset_in = ssl->in_iv - ssl->in_buf; + len_offset_in = ssl->in_len - ssl->in_buf; + if( downsizing ? + ssl->in_buf_len > in_buf_new_len && ssl->in_left < in_buf_new_len : + ssl->in_buf_len < in_buf_new_len ) + { + if( resize_buffer( &ssl->in_buf, in_buf_new_len, &ssl->in_buf_len ) != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "input buffer resizing failed - out of memory" ) ); + } + else + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Reallocating in_buf to %d", in_buf_new_len ) ); + modified = 1; + } + } + } + + if( ssl->out_buf != NULL ) + { + written_out = ssl->out_msg - ssl->out_buf; + iv_offset_out = ssl->out_iv - ssl->out_buf; + len_offset_out = ssl->out_len - ssl->out_buf; + if( downsizing ? + ssl->out_buf_len > out_buf_new_len && ssl->out_left < out_buf_new_len : + ssl->out_buf_len < out_buf_new_len ) + { + if( resize_buffer( &ssl->out_buf, out_buf_new_len, &ssl->out_buf_len ) != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "output buffer resizing failed - out of memory" ) ); + } + else + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Reallocating out_buf to %d", out_buf_new_len ) ); + modified = 1; + } + } + } + if( modified ) + { + /* Update pointers here to avoid doing it twice. */ + mbedtls_ssl_reset_in_out_pointers( ssl ); + /* Fields below might not be properly updated with record + * splitting or with CID, so they are manually updated here. */ + ssl->out_msg = ssl->out_buf + written_out; + ssl->out_len = ssl->out_buf + len_offset_out; + ssl->out_iv = ssl->out_buf + iv_offset_out; + + ssl->in_msg = ssl->in_buf + written_in; + ssl->in_len = ssl->in_buf + len_offset_in; + ssl->in_iv = ssl->in_buf + iv_offset_in; + } +} #endif /* MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH */ /* @@ -3686,64 +3750,9 @@ static int ssl_handshake_init( mbedtls_ssl_context *ssl ) } #if defined(MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH) /* If the buffers are too small - reallocate */ - { - int modified = 0; - size_t written_in = 0, iv_offset_in = 0, len_offset_in = 0; - size_t written_out = 0, iv_offset_out = 0, len_offset_out = 0; - if( ssl->in_buf != NULL ) - { - written_in = ssl->in_msg - ssl->in_buf; - iv_offset_in = ssl->in_iv - ssl->in_buf; - len_offset_in = ssl->in_len - ssl->in_buf; - if( ssl->in_buf_len < MBEDTLS_SSL_IN_BUFFER_LEN ) - { - if( resize_buffer( &ssl->in_buf, MBEDTLS_SSL_IN_BUFFER_LEN, - &ssl->in_buf_len ) != 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "input buffer resizing failed - out of memory" ) ); - } - else - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "Reallocating in_buf to %d", MBEDTLS_SSL_IN_BUFFER_LEN ) ); - modified = 1; - } - } - } - if( ssl->out_buf != NULL ) - { - written_out = ssl->out_msg - ssl->out_buf; - iv_offset_out = ssl->out_iv - ssl->out_buf; - len_offset_out = ssl->out_len - ssl->out_buf; - if( ssl->out_buf_len < MBEDTLS_SSL_OUT_BUFFER_LEN ) - { - if( resize_buffer( &ssl->out_buf, MBEDTLS_SSL_OUT_BUFFER_LEN, - &ssl->out_buf_len ) != 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "output buffer resizing failed - out of memory" ) ); - } - else - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "Reallocating out_buf to %d", MBEDTLS_SSL_OUT_BUFFER_LEN ) ); - modified = 1; - } - } - } - if( modified ) - { - /* Update pointers here to avoid doing it twice. */ - mbedtls_ssl_reset_in_out_pointers( ssl ); - /* Fields below might not be properly updated with record - * splitting or with CID, so they are manually updated here. */ - ssl->out_msg = ssl->out_buf + written_out; - ssl->out_len = ssl->out_buf + len_offset_out; - ssl->out_iv = ssl->out_buf + iv_offset_out; - - ssl->in_msg = ssl->in_buf + written_in; - ssl->in_len = ssl->in_buf + len_offset_in; - ssl->in_iv = ssl->in_buf + iv_offset_in; - } - } + handle_buffer_resizing( ssl, 0, MBEDTLS_SSL_IN_BUFFER_LEN, + MBEDTLS_SSL_OUT_BUFFER_LEN ); #endif /* All pointers should exist and can be directly freed without issue */ @@ -6068,66 +6077,8 @@ void mbedtls_ssl_handshake_free( mbedtls_ssl_context *ssl ) * processes datagrams and the fact that a datagram is allowed to have * several records in it, it is possible that the I/O buffers are not * empty at this stage */ - { - int modified = 0; - uint32_t buf_len = mbedtls_ssl_get_input_buflen( ssl ); - size_t written_in = 0, iv_offset_in = 0, len_offset_in = 0; - size_t written_out = 0, iv_offset_out = 0, len_offset_out = 0; - if( ssl->in_buf != NULL ) - { - written_in = ssl->in_msg - ssl->in_buf; - iv_offset_in = ssl->in_iv - ssl->in_buf; - len_offset_in = ssl->in_len - ssl->in_buf; - if( ssl->in_buf_len > buf_len && ssl->in_left < buf_len ) - { - if( resize_buffer( &ssl->in_buf, buf_len, &ssl->in_buf_len ) != 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "input buffer resizing failed - out of memory" ) ); - } - else - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "Reallocating in_buf to %d", buf_len ) ); - modified = 1; - } - } - } - - - buf_len = mbedtls_ssl_get_output_buflen( ssl ); - if(ssl->out_buf != NULL ) - { - written_out = ssl->out_msg - ssl->out_buf; - iv_offset_out = ssl->out_iv - ssl->out_buf; - len_offset_out = ssl->out_len - ssl->out_buf; - if( ssl->out_buf_len > mbedtls_ssl_get_output_buflen( ssl ) && - ssl->out_left < buf_len ) - { - if( resize_buffer( &ssl->out_buf, buf_len, &ssl->out_buf_len ) != 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "output buffer resizing failed - out of memory" ) ); - } - else - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "Reallocating out_buf to %d", buf_len ) ); - modified = 1; - } - } - } - if( modified ) - { - /* Update pointers here to avoid doing it twice. */ - mbedtls_ssl_reset_in_out_pointers( ssl ); - /* Fields below might not be properly updated with record - * splitting or with CID, so they are manually updated here. */ - ssl->out_msg = ssl->out_buf + written_out; - ssl->out_len = ssl->out_buf + len_offset_out; - ssl->out_iv = ssl->out_buf + iv_offset_out; - - ssl->in_msg = ssl->in_buf + written_in; - ssl->in_len = ssl->in_buf + len_offset_in; - ssl->in_iv = ssl->in_buf + iv_offset_in; - } - } + handle_buffer_resizing( ssl, 1, mbedtls_ssl_get_input_buflen( ssl ), + mbedtls_ssl_get_output_buflen( ssl ) ); #endif } From 069fa96cd7455eef9dd135f255a019838d7413a2 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Thu, 7 Jan 2021 08:02:15 -0500 Subject: [PATCH 2/4] Use size_t instead of uint32_t for ssl I/O buffer lengths Signed-off-by: Andrzej Kurek --- include/mbedtls/ssl_internal.h | 12 ++++++------ library/ssl_tls.c | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 577c959b6..2097a6dd9 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -275,26 +275,26 @@ #endif #if defined(MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH) -static inline uint32_t mbedtls_ssl_get_output_buflen( const mbedtls_ssl_context *ctx ) +static inline size_t mbedtls_ssl_get_output_buflen( const mbedtls_ssl_context *ctx ) { #if defined (MBEDTLS_SSL_DTLS_CONNECTION_ID) - return (uint32_t) mbedtls_ssl_get_output_max_frag_len( ctx ) + return mbedtls_ssl_get_output_max_frag_len( ctx ) + MBEDTLS_SSL_HEADER_LEN + MBEDTLS_SSL_PAYLOAD_OVERHEAD + MBEDTLS_SSL_CID_OUT_LEN_MAX; #else - return (uint32_t) mbedtls_ssl_get_output_max_frag_len( ctx ) + return mbedtls_ssl_get_output_max_frag_len( ctx ) + MBEDTLS_SSL_HEADER_LEN + MBEDTLS_SSL_PAYLOAD_OVERHEAD; #endif } -static inline uint32_t mbedtls_ssl_get_input_buflen( const mbedtls_ssl_context *ctx ) +static inline size_t mbedtls_ssl_get_input_buflen( const mbedtls_ssl_context *ctx ) { #if defined (MBEDTLS_SSL_DTLS_CONNECTION_ID) - return (uint32_t) mbedtls_ssl_get_input_max_frag_len( ctx ) + return mbedtls_ssl_get_input_max_frag_len( ctx ) + MBEDTLS_SSL_HEADER_LEN + MBEDTLS_SSL_PAYLOAD_OVERHEAD + MBEDTLS_SSL_CID_IN_LEN_MAX; #else - return (uint32_t) mbedtls_ssl_get_input_max_frag_len( ctx ) + return mbedtls_ssl_get_input_max_frag_len( ctx ) + MBEDTLS_SSL_HEADER_LEN + MBEDTLS_SSL_PAYLOAD_OVERHEAD; #endif } diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 4a57e769a..336cbea37 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -262,8 +262,8 @@ static int resize_buffer( unsigned char **buffer, size_t len_new, size_t *len_ol } static void handle_buffer_resizing( mbedtls_ssl_context *ssl, int downsizing, - uint32_t in_buf_new_len, - uint32_t out_buf_new_len ) + size_t in_buf_new_len, + size_t out_buf_new_len ) { int modified = 0; size_t written_in = 0, iv_offset_in = 0, len_offset_in = 0; From 557289babc9f95f82329c69fad58d5e32ee9e377 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 21 Oct 2020 15:12:39 +0200 Subject: [PATCH 3/4] Add a missing dependency to config.h Variable buffer lengths depend on the maximum fragment length extension. Signed-off-by: Andrzej Kurek --- include/mbedtls/check_config.h | 4 ++++ include/mbedtls/config.h | 2 ++ 2 files changed, 6 insertions(+) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index accf51e32..7f403c1e4 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -886,6 +886,10 @@ #error "MBEDTLS_SSL_DTLS_SRTP defined, but not all prerequisites" #endif +#if defined(MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH) && ( !defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) ) +#error "MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH defined, but not all prerequisites" +#endif + /* * Avoid warning from -pedantic. This is a convenient place for this * workaround since this is included by every single file before the diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index c5f65e178..818dc0cb0 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1980,6 +1980,8 @@ * \def MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH * * Enable modifying the maximum I/O buffer size. + * + * Requires: MBEDTLS_SSL_MAX_FRAGMENT_LENGTH */ //#define MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH From 2a54a6fe1ce401bd7181c8557550b53cd842b431 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Thu, 7 Jan 2021 08:13:49 -0500 Subject: [PATCH 4/4] Refactor the variable buffer length config description Signed-off-by: Andrzej Kurek --- include/mbedtls/config.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 818dc0cb0..b563a96b7 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1979,7 +1979,8 @@ /** * \def MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH * - * Enable modifying the maximum I/O buffer size. + * When this option is enabled, the SSL buffer will be resized automatically + * based on the negotiated maximum fragment length in each direction. * * Requires: MBEDTLS_SSL_MAX_FRAGMENT_LENGTH */