From 1152fa83f99489cd22149594e6e2f5a4e9be0234 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Fri, 13 Apr 2018 05:15:17 -0400 Subject: [PATCH 1/7] Add platform setup and teardown calls to test suites Add a global platform context variable available for tests --- tests/suites/helpers.function | 1 + tests/suites/main_test.function | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index eef41c79a..9295bfaa5 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -109,6 +109,7 @@ static struct } test_info; +mbedtls_platform_context platform_ctx; /*----------------------------------------------------------------------------*/ /* Helper flags for complex dependencies */ diff --git a/tests/suites/main_test.function b/tests/suites/main_test.function index 042085f0b..9dd792d36 100644 --- a/tests/suites/main_test.function +++ b/tests/suites/main_test.function @@ -281,6 +281,14 @@ int main(int argc, const char *argv[]) #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) && \ !defined(TEST_SUITE_MEMORY_BUFFER_ALLOC) unsigned char alloc_buf[1000000]; +#endif + if( mbedtls_platform_setup( &platform_ctx ) ) + { + mbedtls_fprintf( stderr, "FATAL: Failed to initialize platform" ); + return -1; + } +#if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) && \ + !defined(TEST_SUITE_MEMORY_BUFFER_ALLOC) mbedtls_memory_buffer_alloc_init( alloc_buf, sizeof(alloc_buf) ); #endif @@ -293,6 +301,7 @@ int main(int argc, const char *argv[]) if( pointer != NULL ) { mbedtls_fprintf( stderr, "all-bits-zero is not a NULL pointer\n" ); + mbedtls_platform_teardown( &platform_ctx ); return( 1 ); } @@ -302,6 +311,7 @@ int main(int argc, const char *argv[]) if( run_test_snprintf() != 0 ) { mbedtls_fprintf( stderr, "the snprintf implementation is broken\n" ); + mbedtls_platform_teardown( &platform_ctx ); return( 0 ); } @@ -318,6 +328,7 @@ int main(int argc, const char *argv[]) strcmp(next_arg, "-h" ) == 0 ) { mbedtls_fprintf( stdout, USAGE ); + mbedtls_platform_teardown( &platform_ctx ); mbedtls_exit( EXIT_SUCCESS ); } else @@ -357,6 +368,7 @@ int main(int argc, const char *argv[]) { mbedtls_fprintf( stderr, "Failed to open test file: %s\n", test_filename ); + mbedtls_platform_teardown( &platform_ctx ); return( 1 ); } @@ -366,6 +378,7 @@ int main(int argc, const char *argv[]) { mbedtls_fprintf( stderr, "FATAL: Dep count larger than zero at start of loop\n" ); + mbedtls_platform_teardown( &platform_ctx ); mbedtls_exit( MBEDTLS_EXIT_FAILURE ); } unmet_dep_count = 0; @@ -402,6 +415,7 @@ int main(int argc, const char *argv[]) if( unmet_dependencies[ unmet_dep_count ] == NULL ) { mbedtls_fprintf( stderr, "FATAL: Out of memory\n" ); + mbedtls_platform_teardown( &platform_ctx ); mbedtls_exit( MBEDTLS_EXIT_FAILURE ); } unmet_dep_count++; @@ -428,6 +442,7 @@ int main(int argc, const char *argv[]) if( stdout_fd == -1 ) { /* Redirection has failed with no stdout so exit */ + mbedtls_platform_teardown( &platform_ctx ); exit( 1 ); } } @@ -439,6 +454,7 @@ int main(int argc, const char *argv[]) if( !option_verbose && restore_output( &stdout, stdout_fd ) ) { /* Redirection has failed with no stdout so exit */ + mbedtls_platform_teardown( &platform_ctx ); exit( 1 ); } #endif /* __unix__ || __APPLE__ __MACH__ */ @@ -490,6 +506,7 @@ int main(int argc, const char *argv[]) { mbedtls_fprintf( stderr, "FAILED: FATAL PARSE ERROR\n" ); fclose( file ); + mbedtls_platform_teardown( &platform_ctx ); mbedtls_exit( 2 ); } else @@ -501,6 +518,7 @@ int main(int argc, const char *argv[]) { mbedtls_fprintf( stderr, "Should be empty %d\n", (int) strlen( buf ) ); + mbedtls_platform_teardown( &platform_ctx ); return( 1 ); } } @@ -533,5 +551,6 @@ int main(int argc, const char *argv[]) close_output( stdout ); #endif /* __unix__ || __APPLE__ __MACH__ */ + mbedtls_platform_teardown( &platform_ctx ); return( total_errors != 0 ); } From aca09c70263c0abe3bc99d6d2f73e4fe7c4e0729 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Fri, 13 Apr 2018 05:18:08 -0400 Subject: [PATCH 2/7] Changelog entry Describing platform teardown and setup calls in test suites --- ChangeLog | 1 + 1 file changed, 1 insertion(+) diff --git a/ChangeLog b/ChangeLog index 9ee82c685..d8f742527 100644 --- a/ChangeLog +++ b/ChangeLog @@ -93,6 +93,7 @@ Changes * Improve robustness of mbedtls_ssl_derive_keys against the use of HMAC functions with non-HMAC ciphersuites. Independently contributed by Jiayuan Chen in #1377. Fixes #1437. + * Add platform setup and teardown calls in test suites. = mbed TLS 2.8.0 branch released 2018-03-16 From 32a675f032fdc9ef14cb8c171fb187d42b51c998 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Fri, 13 Apr 2018 06:16:04 -0400 Subject: [PATCH 3/7] Add conditional platform context creation & usage Add another layer of abstraction before calling platform setup and teardown. --- tests/suites/helpers.function | 19 +++++++++++++++++++ tests/suites/main_test.function | 24 ++++++++++++------------ 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index 9295bfaa5..e716318b1 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -109,7 +109,9 @@ static struct } test_info; +#if defined(MBEDTLS_PLATFORM_C) mbedtls_platform_context platform_ctx; +#endif /*----------------------------------------------------------------------------*/ /* Helper flags for complex dependencies */ @@ -128,6 +130,23 @@ mbedtls_platform_context platform_ctx; /*----------------------------------------------------------------------------*/ /* Helper Functions */ +static int platform_setup() +{ +#if defined(MBEDTLS_PLATFORM_C) + if( mbedtls_platform_setup( &platform_ctx ) ) + { + return -1; + } +#endif /* MBEDTLS_PLATFORM_C */ + return 0; +} + +static void platform_teardown() +{ +#if defined(MBEDTLS_PLATFORM_C) + mbedtls_platform_teardown( &platform_ctx ); +#endif /* MBEDTLS_PLATFORM_C */ +} #if defined(__unix__) || (defined(__APPLE__) && defined(__MACH__)) static int redirect_output( FILE** out_stream, const char* path ) diff --git a/tests/suites/main_test.function b/tests/suites/main_test.function index 9dd792d36..e5b404358 100644 --- a/tests/suites/main_test.function +++ b/tests/suites/main_test.function @@ -282,7 +282,7 @@ int main(int argc, const char *argv[]) !defined(TEST_SUITE_MEMORY_BUFFER_ALLOC) unsigned char alloc_buf[1000000]; #endif - if( mbedtls_platform_setup( &platform_ctx ) ) + if( platform_setup() ) { mbedtls_fprintf( stderr, "FATAL: Failed to initialize platform" ); return -1; @@ -301,7 +301,7 @@ int main(int argc, const char *argv[]) if( pointer != NULL ) { mbedtls_fprintf( stderr, "all-bits-zero is not a NULL pointer\n" ); - mbedtls_platform_teardown( &platform_ctx ); + platform_teardown(); return( 1 ); } @@ -311,7 +311,7 @@ int main(int argc, const char *argv[]) if( run_test_snprintf() != 0 ) { mbedtls_fprintf( stderr, "the snprintf implementation is broken\n" ); - mbedtls_platform_teardown( &platform_ctx ); + platform_teardown(); return( 0 ); } @@ -328,7 +328,7 @@ int main(int argc, const char *argv[]) strcmp(next_arg, "-h" ) == 0 ) { mbedtls_fprintf( stdout, USAGE ); - mbedtls_platform_teardown( &platform_ctx ); + platform_teardown(); mbedtls_exit( EXIT_SUCCESS ); } else @@ -368,7 +368,7 @@ int main(int argc, const char *argv[]) { mbedtls_fprintf( stderr, "Failed to open test file: %s\n", test_filename ); - mbedtls_platform_teardown( &platform_ctx ); + platform_teardown(); return( 1 ); } @@ -378,7 +378,7 @@ int main(int argc, const char *argv[]) { mbedtls_fprintf( stderr, "FATAL: Dep count larger than zero at start of loop\n" ); - mbedtls_platform_teardown( &platform_ctx ); + platform_teardown(); mbedtls_exit( MBEDTLS_EXIT_FAILURE ); } unmet_dep_count = 0; @@ -415,7 +415,7 @@ int main(int argc, const char *argv[]) if( unmet_dependencies[ unmet_dep_count ] == NULL ) { mbedtls_fprintf( stderr, "FATAL: Out of memory\n" ); - mbedtls_platform_teardown( &platform_ctx ); + platform_teardown(); mbedtls_exit( MBEDTLS_EXIT_FAILURE ); } unmet_dep_count++; @@ -441,8 +441,8 @@ int main(int argc, const char *argv[]) stdout_fd = redirect_output( &stdout, "/dev/null" ); if( stdout_fd == -1 ) { + platform_teardown(); /* Redirection has failed with no stdout so exit */ - mbedtls_platform_teardown( &platform_ctx ); exit( 1 ); } } @@ -454,7 +454,7 @@ int main(int argc, const char *argv[]) if( !option_verbose && restore_output( &stdout, stdout_fd ) ) { /* Redirection has failed with no stdout so exit */ - mbedtls_platform_teardown( &platform_ctx ); + platform_teardown(); exit( 1 ); } #endif /* __unix__ || __APPLE__ __MACH__ */ @@ -506,7 +506,7 @@ int main(int argc, const char *argv[]) { mbedtls_fprintf( stderr, "FAILED: FATAL PARSE ERROR\n" ); fclose( file ); - mbedtls_platform_teardown( &platform_ctx ); + platform_teardown(); mbedtls_exit( 2 ); } else @@ -518,7 +518,7 @@ int main(int argc, const char *argv[]) { mbedtls_fprintf( stderr, "Should be empty %d\n", (int) strlen( buf ) ); - mbedtls_platform_teardown( &platform_ctx ); + platform_teardown(); return( 1 ); } } @@ -551,6 +551,6 @@ int main(int argc, const char *argv[]) close_output( stdout ); #endif /* __unix__ || __APPLE__ __MACH__ */ - mbedtls_platform_teardown( &platform_ctx ); + platform_teardown(); return( total_errors != 0 ); } From a282270a10b42fb0d4d109c3e96ccc745f2095cf Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Mon, 16 Apr 2018 06:33:28 -0400 Subject: [PATCH 4/7] Add explicit checks for non-zero result of platform setup in test suites --- tests/suites/helpers.function | 2 +- tests/suites/main_test.function | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index e716318b1..c436fbb87 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -133,7 +133,7 @@ mbedtls_platform_context platform_ctx; static int platform_setup() { #if defined(MBEDTLS_PLATFORM_C) - if( mbedtls_platform_setup( &platform_ctx ) ) + if( mbedtls_platform_setup( &platform_ctx ) != 0 ) { return -1; } diff --git a/tests/suites/main_test.function b/tests/suites/main_test.function index e5b404358..8d7e47769 100644 --- a/tests/suites/main_test.function +++ b/tests/suites/main_test.function @@ -282,7 +282,7 @@ int main(int argc, const char *argv[]) !defined(TEST_SUITE_MEMORY_BUFFER_ALLOC) unsigned char alloc_buf[1000000]; #endif - if( platform_setup() ) + if( platform_setup() != 0 ) { mbedtls_fprintf( stderr, "FATAL: Failed to initialize platform" ); return -1; From f13ca9536c80c145e4b96721bb73a21ca8e9f41a Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 18 Apr 2018 04:14:31 -0400 Subject: [PATCH 5/7] Test suites: print error on failed platform_setup Return encountered errors instead of covering them Fix return value on the broken snprintf implementation --- tests/suites/helpers.function | 8 +++----- tests/suites/main_test.function | 12 ++++++++---- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index c436fbb87..f82694ada 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -132,13 +132,11 @@ mbedtls_platform_context platform_ctx; /* Helper Functions */ static int platform_setup() { + int ret = 0; #if defined(MBEDTLS_PLATFORM_C) - if( mbedtls_platform_setup( &platform_ctx ) != 0 ) - { - return -1; - } + ret = mbedtls_platform_setup( &platform_ctx ); #endif /* MBEDTLS_PLATFORM_C */ - return 0; + return( ret ); } static void platform_teardown() diff --git a/tests/suites/main_test.function b/tests/suites/main_test.function index 8d7e47769..1390f9fbb 100644 --- a/tests/suites/main_test.function +++ b/tests/suites/main_test.function @@ -282,10 +282,14 @@ int main(int argc, const char *argv[]) !defined(TEST_SUITE_MEMORY_BUFFER_ALLOC) unsigned char alloc_buf[1000000]; #endif - if( platform_setup() != 0 ) + /* Platform setup should be called in the beginning */ + ret = platform_setup(); + if( ret != 0 ) { - mbedtls_fprintf( stderr, "FATAL: Failed to initialize platform" ); - return -1; + mbedtls_fprintf( stderr, + "FATAL: Failed to initialize platform - error %d\n", + ret ); + return( -1 ); } #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) && \ !defined(TEST_SUITE_MEMORY_BUFFER_ALLOC) @@ -312,7 +316,7 @@ int main(int argc, const char *argv[]) { mbedtls_fprintf( stderr, "the snprintf implementation is broken\n" ); platform_teardown(); - return( 0 ); + return( 1 ); } while( arg_index < argc) From 5462e028743938c120e35c0a410955bb055f1a4f Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Fri, 20 Apr 2018 07:58:53 -0400 Subject: [PATCH 6/7] ssl_tls: Fix invalid buffer sizes during compression / decompression Adjust information passed to zlib to include already written data. --- ChangeLog | 2 ++ library/ssl_tls.c | 13 ++++++++----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index ae8d86f20..e15a53b6b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -55,6 +55,8 @@ Bugfix in the internal buffers; these cases lead to deadlocks in case event-driven I/O was used. Found and reported by Hubert Mis in #772. + * Fix invalid buffer sizes passed to zlib during record compression and + decompression. Changes * Remove some redundant code in bignum.c. Contributed by Alexey Skalozub. diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e8063d2c1..9374961bd 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2108,6 +2108,7 @@ static int ssl_compress_buf( mbedtls_ssl_context *ssl ) { int ret; unsigned char *msg_post = ssl->out_msg; + ptrdiff_t bytes_written = ssl->out_msg - ssl->out_buf; size_t len_pre = ssl->out_msglen; unsigned char *msg_pre = ssl->compress_buf; @@ -2127,7 +2128,7 @@ static int ssl_compress_buf( mbedtls_ssl_context *ssl ) ssl->transform_out->ctx_deflate.next_in = msg_pre; ssl->transform_out->ctx_deflate.avail_in = len_pre; ssl->transform_out->ctx_deflate.next_out = msg_post; - ssl->transform_out->ctx_deflate.avail_out = MBEDTLS_SSL_BUFFER_LEN; + ssl->transform_out->ctx_deflate.avail_out = MBEDTLS_SSL_BUFFER_LEN - bytes_written; ret = deflate( &ssl->transform_out->ctx_deflate, Z_SYNC_FLUSH ); if( ret != Z_OK ) @@ -2137,7 +2138,7 @@ static int ssl_compress_buf( mbedtls_ssl_context *ssl ) } ssl->out_msglen = MBEDTLS_SSL_BUFFER_LEN - - ssl->transform_out->ctx_deflate.avail_out; + ssl->transform_out->ctx_deflate.avail_out - bytes_written; MBEDTLS_SSL_DEBUG_MSG( 3, ( "after compression: msglen = %d, ", ssl->out_msglen ) ); @@ -2154,6 +2155,7 @@ static int ssl_decompress_buf( mbedtls_ssl_context *ssl ) { int ret; unsigned char *msg_post = ssl->in_msg; + ptrdiff_t bytes_written = ssl->in_msg - ssl->in_buf; size_t len_pre = ssl->in_msglen; unsigned char *msg_pre = ssl->compress_buf; @@ -2173,7 +2175,8 @@ static int ssl_decompress_buf( mbedtls_ssl_context *ssl ) ssl->transform_in->ctx_inflate.next_in = msg_pre; ssl->transform_in->ctx_inflate.avail_in = len_pre; ssl->transform_in->ctx_inflate.next_out = msg_post; - ssl->transform_in->ctx_inflate.avail_out = MBEDTLS_SSL_MAX_CONTENT_LEN; + ssl->transform_in->ctx_inflate.avail_out = MBEDTLS_SSL_BUFFER_LEN - + bytes_written; ret = inflate( &ssl->transform_in->ctx_inflate, Z_SYNC_FLUSH ); if( ret != Z_OK ) @@ -2182,8 +2185,8 @@ static int ssl_decompress_buf( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_COMPRESSION_FAILED ); } - ssl->in_msglen = MBEDTLS_SSL_MAX_CONTENT_LEN - - ssl->transform_in->ctx_inflate.avail_out; + ssl->in_msglen = MBEDTLS_SSL_BUFFER_LEN - + ssl->transform_in->ctx_inflate.avail_out - bytes_written; MBEDTLS_SSL_DEBUG_MSG( 3, ( "after decompression: msglen = %d, ", ssl->in_msglen ) ); From a9ceef8e032e488e9b3821bfdac4b4dfba2f3a20 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Tue, 24 Apr 2018 06:32:44 -0400 Subject: [PATCH 7/7] Change variable bytes_written to header_bytes in record decompression The name is changed to better reflect the input, decompression case --- library/ssl_tls.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 9374961bd..8a903c563 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2155,7 +2155,7 @@ static int ssl_decompress_buf( mbedtls_ssl_context *ssl ) { int ret; unsigned char *msg_post = ssl->in_msg; - ptrdiff_t bytes_written = ssl->in_msg - ssl->in_buf; + ptrdiff_t header_bytes = ssl->in_msg - ssl->in_buf; size_t len_pre = ssl->in_msglen; unsigned char *msg_pre = ssl->compress_buf; @@ -2176,7 +2176,7 @@ static int ssl_decompress_buf( mbedtls_ssl_context *ssl ) ssl->transform_in->ctx_inflate.avail_in = len_pre; ssl->transform_in->ctx_inflate.next_out = msg_post; ssl->transform_in->ctx_inflate.avail_out = MBEDTLS_SSL_BUFFER_LEN - - bytes_written; + header_bytes; ret = inflate( &ssl->transform_in->ctx_inflate, Z_SYNC_FLUSH ); if( ret != Z_OK ) @@ -2186,7 +2186,7 @@ static int ssl_decompress_buf( mbedtls_ssl_context *ssl ) } ssl->in_msglen = MBEDTLS_SSL_BUFFER_LEN - - ssl->transform_in->ctx_inflate.avail_out - bytes_written; + ssl->transform_in->ctx_inflate.avail_out - header_bytes; MBEDTLS_SSL_DEBUG_MSG( 3, ( "after decompression: msglen = %d, ", ssl->in_msglen ) );