From a2bdcb9e3ae86ce01f6e33067a1f5d16860f0008 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 15:02:14 +0100 Subject: [PATCH 1/5] Remove redundant block_size validity check Check the value only once, as soon as we've obtained it. --- library/cipher.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index b62f1d593..409c3fe67 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -527,6 +527,10 @@ int mbedtls_cipher_update( mbedtls_cipher_context_t *ctx, const unsigned char *i *olen = 0; block_size = mbedtls_cipher_get_block_size( ctx ); + if ( 0 == block_size ) + { + return( MBEDTLS_ERR_CIPHER_INVALID_CONTEXT ); + } if( ctx->cipher_info->mode == MBEDTLS_MODE_ECB ) { @@ -562,11 +566,6 @@ int mbedtls_cipher_update( mbedtls_cipher_context_t *ctx, const unsigned char *i } #endif - if ( 0 == block_size ) - { - return( MBEDTLS_ERR_CIPHER_INVALID_CONTEXT ); - } - if( input == output && ( ctx->unprocessed_len != 0 || ilen % block_size ) ) { @@ -625,11 +624,6 @@ int mbedtls_cipher_update( mbedtls_cipher_context_t *ctx, const unsigned char *i */ if( 0 != ilen ) { - if( 0 == block_size ) - { - return( MBEDTLS_ERR_CIPHER_INVALID_CONTEXT ); - } - /* Encryption: only cache partial blocks * Decryption w/ padding: always keep at least one whole block * Decryption w/o padding: only cache partial blocks From 42a1acfd0e100035f71e7112935a115136d6b90c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 16:12:07 +0100 Subject: [PATCH 2/5] get_len_step: Fix end-of-buffer calculation when buffer_size==0 Fix get_len_step when buffer_size==0. The intent of this test is to ensure (via static or runtime buffer overflow analysis) that mbedtls_asn1_get_len does not attempt to access beyond the end of the buffer. When buffer_size is 0 (reached from get_len when parsing a 1-byte buffer), the buffer is buf[1..1] because allocating a 0-byte buffer might yield a null pointer rather than a valid pointer. In this case the end of the buffer is p==buf+1, not buf+buffer_size which is buf+0. The test passed because calling mbedtls_asn1_get_len(&p,end,...) with end < p happens to work, but this is not guaranteed. --- tests/suites/test_suite_asn1parse.function | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_asn1parse.function b/tests/suites/test_suite_asn1parse.function index d747cc254..f07fd409d 100644 --- a/tests/suites/test_suite_asn1parse.function +++ b/tests/suites/test_suite_asn1parse.function @@ -121,6 +121,7 @@ int get_len_step( const data_t *input, size_t buffer_size, { unsigned char *buf = NULL; unsigned char *p = NULL; + unsigned char *end; size_t parsed_length; int ret; @@ -130,7 +131,8 @@ int get_len_step( const data_t *input, size_t buffer_size, if( buffer_size == 0 ) { ASSERT_ALLOC( buf, 1 ); - p = buf + 1; + end = buf + 1; + p = end; } else { @@ -145,9 +147,10 @@ int get_len_step( const data_t *input, size_t buffer_size, memcpy( buf, input->x, buffer_size ); } p = buf; + end = buf + buffer_size; } - ret = mbedtls_asn1_get_len( &p, buf + buffer_size, &parsed_length ); + ret = mbedtls_asn1_get_len( &p, end, &parsed_length ); if( buffer_size >= input->len + actual_length ) { From 292672eb1246f3d0a93b456b2ed6090ff13d4020 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 16:20:04 +0100 Subject: [PATCH 3/5] If ASSERT_ALLOC_WEAK fails, mark the test as skipped, not passed This was the intended behavior of ASSERT_ALLOC_WEAK all along, but skipping was not implemented yet when ASSERT_ALLOC_WEAK was introduced. --- tests/suites/helpers.function | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index 0cce463c0..3e68c0657 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -158,11 +158,10 @@ typedef enum } \ while( 0 ) -/** Allocate memory dynamically. Exit the test if this fails, but do - * not mark the test as failed. +/** Allocate memory dynamically. If the allocation fails, skip the test case. * * This macro behaves like #ASSERT_ALLOC, except that if the allocation - * fails, it jumps to the \c exit label without calling test_fail(). + * fails, it marks the test as skipped rather than failed. */ #define ASSERT_ALLOC_WEAK( pointer, length ) \ do \ @@ -172,8 +171,7 @@ typedef enum { \ ( pointer ) = mbedtls_calloc( sizeof( *( pointer ) ), \ ( length ) ); \ - if( ( pointer ) == NULL ) \ - goto exit; \ + TEST_ASSUME( ( pointer ) != NULL ); \ } \ } \ while( 0 ) From 9018b11302ecde4e00f586aaeff3388a971ada73 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 16:30:53 +0100 Subject: [PATCH 4/5] Check that mbedtls_mpi_grow succeeds --- tests/suites/test_suite_mpi.function | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index 63a2509e1..0f35a2a28 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -600,8 +600,8 @@ void mbedtls_mpi_lt_mpi_ct( int size_X, char * input_X, TEST_ASSERT( mbedtls_mpi_read_string( &X, 16, input_X ) == 0 ); TEST_ASSERT( mbedtls_mpi_read_string( &Y, 16, input_Y ) == 0 ); - mbedtls_mpi_grow( &X, size_X ); - mbedtls_mpi_grow( &Y, size_Y ); + TEST_ASSERT( mbedtls_mpi_grow( &X, size_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_grow( &Y, size_Y ) == 0 ); TEST_ASSERT( mbedtls_mpi_lt_mpi_ct( &X, &Y, &ret ) == input_err ); if( input_err == 0 ) From 84984ae22077538eb97ea0f85d12706b224c367e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 16:52:08 +0100 Subject: [PATCH 5/5] Add missing return code check on calls to mbedtls_md() --- tests/suites/test_suite_ecdsa.function | 4 +++- tests/suites/test_suite_pk.function | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_ecdsa.function b/tests/suites/test_suite_ecdsa.function index ab3db3adf..59c1c4907 100644 --- a/tests/suites/test_suite_ecdsa.function +++ b/tests/suites/test_suite_ecdsa.function @@ -500,7 +500,9 @@ void ecdsa_write_restart( int id, char *d_str, int md_alg, TEST_ASSERT( md_info != NULL ); hlen = mbedtls_md_get_size( md_info ); - mbedtls_md( md_info, (const unsigned char *) msg, strlen( msg ), hash ); + TEST_ASSERT( mbedtls_md( md_info, + (const unsigned char *) msg, strlen( msg ), + hash ) == 0 ); mbedtls_ecp_set_max_ops( max_ops ); diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 926cec425..47427252c 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -844,7 +844,9 @@ void pk_sign_verify_restart( int pk_type, int grp_id, char *d_str, TEST_ASSERT( md_info != NULL ); hlen = mbedtls_md_get_size( md_info ); - mbedtls_md( md_info, (const unsigned char *) msg, strlen( msg ), hash ); + TEST_ASSERT( mbedtls_md( md_info, + (const unsigned char *) msg, strlen( msg ), + hash ) == 0 ); mbedtls_ecp_set_max_ops( max_ops );