From 9cf1f96a7b4d6dc5e9bb38cb41b407aa65cfeca2 Mon Sep 17 00:00:00 2001 From: Andres AG Date: Mon, 30 Jan 2017 14:34:25 +0000 Subject: [PATCH 1/6] Fix corner case uses of memory_buffer_alloc.c The corner cases fixed include: * Allocating a buffer of size 0. With this change, the allocator now returns a NULL pointer in this case. Note that changes in pem.c and x509_crl.c were required to fix tests that did not work under this assumption. * Initialising the allocator with less memory than required for headers. * Fix header chain checks for uninitialised allocator. --- ChangeLog | 2 ++ library/memory_buffer_alloc.c | 29 ++++++++++++++++++----------- library/pem.c | 4 ++-- library/x509_crl.c | 4 ++-- 4 files changed, 24 insertions(+), 15 deletions(-) diff --git a/ChangeLog b/ChangeLog index a200d51fb..4aa66fd82 100644 --- a/ChangeLog +++ b/ChangeLog @@ -104,6 +104,8 @@ Bugfix * Fix error message in programs/pkey/gen_key.c. Found and fixed by Chris Xue. * Fix programs/pkey/dh_server.c so that it actually works with dh_client.c. Found and fixed by Martijn de Milliano. + * Fix memory allocation corner cases in memory_buffer_alloc.c module. Found + by Guido Vranken. #639 Changes * Extend cert_write example program by options to set the CRT version diff --git a/library/memory_buffer_alloc.c b/library/memory_buffer_alloc.c index 545d5a2c3..0d3342dea 100644 --- a/library/memory_buffer_alloc.c +++ b/library/memory_buffer_alloc.c @@ -182,9 +182,9 @@ static int verify_header( memory_header *hdr ) static int verify_chain() { - memory_header *prv = heap.first, *cur = heap.first->next; + memory_header *prv = heap.first, *cur; - if( verify_header( heap.first ) != 0 ) + if( heap.first == NULL || verify_header( heap.first ) != 0 ) { #if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_fprintf( stderr, "FATAL: verification of first header " @@ -202,6 +202,8 @@ static int verify_chain() return( 1 ); } + cur = heap.first->next; + while( cur != NULL ) { if( verify_header( cur ) != 0 ) @@ -245,7 +247,9 @@ static void *buffer_alloc_calloc( size_t n, size_t size ) original_len = len = n * size; - if( n != 0 && len / n != size ) + if( n == 0 || size == 0 || len / n != size ) + return( NULL ); + else if( len > (size_t)-MBEDTLS_MEMORY_ALIGN_MULTIPLE ) return( NULL ); if( len % MBEDTLS_MEMORY_ALIGN_MULTIPLE ) @@ -386,7 +390,7 @@ static void buffer_alloc_free( void *ptr ) if( ptr == NULL || heap.buf == NULL || heap.first == NULL ) return; - if( p < heap.buf || p > heap.buf + heap.len ) + if( p < heap.buf || p >= heap.buf + heap.len ) { #if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_fprintf( stderr, "FATAL: mbedtls_free() outside of managed " @@ -570,8 +574,7 @@ static void buffer_alloc_free_mutexed( void *ptr ) void mbedtls_memory_buffer_alloc_init( unsigned char *buf, size_t len ) { - memset( &heap, 0, sizeof(buffer_alloc_ctx) ); - memset( buf, 0, len ); + memset( &heap, 0, sizeof( buffer_alloc_ctx ) ); #if defined(MBEDTLS_THREADING_C) mbedtls_mutex_init( &heap.mutex ); @@ -581,20 +584,24 @@ void mbedtls_memory_buffer_alloc_init( unsigned char *buf, size_t len ) mbedtls_platform_set_calloc_free( buffer_alloc_calloc, buffer_alloc_free ); #endif - if( (size_t) buf % MBEDTLS_MEMORY_ALIGN_MULTIPLE ) + if( len < sizeof( memory_header ) + MBEDTLS_MEMORY_ALIGN_MULTIPLE ) + return; + else if( (size_t)buf % MBEDTLS_MEMORY_ALIGN_MULTIPLE ) { /* Adjust len first since buf is used in the computation */ len -= MBEDTLS_MEMORY_ALIGN_MULTIPLE - - (size_t) buf % MBEDTLS_MEMORY_ALIGN_MULTIPLE; + - (size_t)buf % MBEDTLS_MEMORY_ALIGN_MULTIPLE; buf += MBEDTLS_MEMORY_ALIGN_MULTIPLE - - (size_t) buf % MBEDTLS_MEMORY_ALIGN_MULTIPLE; + - (size_t)buf % MBEDTLS_MEMORY_ALIGN_MULTIPLE; } + memset( buf, 0, len ); + heap.buf = buf; heap.len = len; - heap.first = (memory_header *) buf; - heap.first->size = len - sizeof(memory_header); + heap.first = (memory_header *)buf; + heap.first->size = len - sizeof( memory_header ); heap.first->magic1 = MAGIC1; heap.first->magic2 = MAGIC2; heap.first_free = heap.first; diff --git a/library/pem.c b/library/pem.c index 87401ba55..d726bd61b 100644 --- a/library/pem.c +++ b/library/pem.c @@ -423,7 +423,7 @@ int mbedtls_pem_write_buffer( const char *header, const char *footer, unsigned char *buf, size_t buf_len, size_t *olen ) { int ret; - unsigned char *encode_buf, *c, *p = buf; + unsigned char *encode_buf = NULL, *c, *p = buf; size_t len = 0, use_len, add_len = 0; mbedtls_base64_encode( NULL, 0, &use_len, der_data, der_len ); @@ -435,7 +435,7 @@ int mbedtls_pem_write_buffer( const char *header, const char *footer, return( MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL ); } - if( ( encode_buf = mbedtls_calloc( 1, use_len ) ) == NULL ) + if( use_len != 0 && ( encode_buf = mbedtls_calloc( 1, use_len ) ) == NULL ) return( MBEDTLS_ERR_PEM_ALLOC_FAILED ); if( ( ret = mbedtls_base64_encode( encode_buf, use_len, &use_len, der_data, diff --git a/library/x509_crl.c b/library/x509_crl.c index 55d12acd0..c302bb2fb 100644 --- a/library/x509_crl.c +++ b/library/x509_crl.c @@ -257,7 +257,7 @@ int mbedtls_x509_crl_parse_der( mbedtls_x509_crl *chain, { int ret; size_t len; - unsigned char *p, *end; + unsigned char *p = NULL, *end; mbedtls_x509_buf sig_params1, sig_params2, sig_oid2; mbedtls_x509_crl *crl = chain; @@ -294,7 +294,7 @@ int mbedtls_x509_crl_parse_der( mbedtls_x509_crl *chain, /* * Copy raw DER-encoded CRL */ - if( ( p = mbedtls_calloc( 1, buflen ) ) == NULL ) + if( buflen != 0 && ( p = mbedtls_calloc( 1, buflen ) ) == NULL ) return( MBEDTLS_ERR_X509_ALLOC_FAILED ); memcpy( p, buf, buflen ); From 8ec3bfe1800d7b18e24d9439b641e91c4dd408ed Mon Sep 17 00:00:00 2001 From: Andres AG Date: Mon, 30 Jan 2017 14:35:08 +0000 Subject: [PATCH 2/6] Test corner case uses of memory_buffer_alloc.c --- .../test_suite_memory_buffer_alloc.data | 5 ++++ .../test_suite_memory_buffer_alloc.function | 28 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/tests/suites/test_suite_memory_buffer_alloc.data b/tests/suites/test_suite_memory_buffer_alloc.data index 8d3813a7b..d59f1135a 100644 --- a/tests/suites/test_suite_memory_buffer_alloc.data +++ b/tests/suites/test_suite_memory_buffer_alloc.data @@ -16,3 +16,8 @@ memory_buffer_alloc_free_alloc:100:64:100:100:0:0:0:1:200:0 Memory buffer alloc - Out of Memory test memory_buffer_alloc_oom_test: +Memory buffer small buffer +memory_buffer_small_buffer: + +Memory buffer underalloc +memory_buffer_underalloc: diff --git a/tests/suites/test_suite_memory_buffer_alloc.function b/tests/suites/test_suite_memory_buffer_alloc.function index a0c70d8a2..09684c1d4 100644 --- a/tests/suites/test_suite_memory_buffer_alloc.function +++ b/tests/suites/test_suite_memory_buffer_alloc.function @@ -232,3 +232,31 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_MEMORY_DEBUG */ +void memory_buffer_small_buffer( ) +{ + unsigned char buf[1]; + + mbedtls_memory_buffer_alloc_init( buf, sizeof( buf ) ); + TEST_ASSERT( mbedtls_memory_buffer_alloc_verify() != 0 ); +} +/* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_MEMORY_DEBUG */ +void memory_buffer_underalloc( ) +{ + unsigned char buf[100]; + size_t i; + + mbedtls_memory_buffer_alloc_init( buf, sizeof( buf ) ); + for( i = 1; i < MBEDTLS_MEMORY_ALIGN_MULTIPLE; i++ ) + { + TEST_ASSERT( mbedtls_calloc( 1, + (size_t)-( MBEDTLS_MEMORY_ALIGN_MULTIPLE - i ) ) == NULL ); + TEST_ASSERT( mbedtls_memory_buffer_alloc_verify() == 0 ); + } + +exit: + mbedtls_memory_buffer_alloc_free(); +} +/* END_CASE */ From f1ee63562aa6bd42603adc61cda158b9fe109360 Mon Sep 17 00:00:00 2001 From: Andres Amaya Garcia Date: Thu, 6 Jul 2017 10:06:58 +0100 Subject: [PATCH 3/6] Style fixes in pem, x509_crl and buf_alloc --- library/memory_buffer_alloc.c | 2 +- library/pem.c | 3 ++- library/x509_crl.c | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/library/memory_buffer_alloc.c b/library/memory_buffer_alloc.c index 0d3342dea..1cfc27ca6 100644 --- a/library/memory_buffer_alloc.c +++ b/library/memory_buffer_alloc.c @@ -184,7 +184,7 @@ static int verify_chain() { memory_header *prv = heap.first, *cur; - if( heap.first == NULL || verify_header( heap.first ) != 0 ) + if( prv == NULL || verify_header( prv ) != 0 ) { #if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_fprintf( stderr, "FATAL: verification of first header " diff --git a/library/pem.c b/library/pem.c index d726bd61b..7b3ae8d3d 100644 --- a/library/pem.c +++ b/library/pem.c @@ -435,7 +435,8 @@ int mbedtls_pem_write_buffer( const char *header, const char *footer, return( MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL ); } - if( use_len != 0 && ( encode_buf = mbedtls_calloc( 1, use_len ) ) == NULL ) + if( use_len != 0 && + ( ( encode_buf = mbedtls_calloc( 1, use_len ) ) == NULL ) ) return( MBEDTLS_ERR_PEM_ALLOC_FAILED ); if( ( ret = mbedtls_base64_encode( encode_buf, use_len, &use_len, der_data, diff --git a/library/x509_crl.c b/library/x509_crl.c index c302bb2fb..8f98d8c92 100644 --- a/library/x509_crl.c +++ b/library/x509_crl.c @@ -257,7 +257,7 @@ int mbedtls_x509_crl_parse_der( mbedtls_x509_crl *chain, { int ret; size_t len; - unsigned char *p = NULL, *end; + unsigned char *p = NULL, *end = NULL; mbedtls_x509_buf sig_params1, sig_params2, sig_oid2; mbedtls_x509_crl *crl = chain; @@ -294,7 +294,7 @@ int mbedtls_x509_crl_parse_der( mbedtls_x509_crl *chain, /* * Copy raw DER-encoded CRL */ - if( buflen != 0 && ( p = mbedtls_calloc( 1, buflen ) ) == NULL ) + if( buflen != 0 && ( ( p = mbedtls_calloc( 1, buflen ) ) == NULL ) ) return( MBEDTLS_ERR_X509_ALLOC_FAILED ); memcpy( p, buf, buflen ); From cb5123fa86982c75f2c8061b58ac51c9e9938fdb Mon Sep 17 00:00:00 2001 From: Andres Amaya Garcia Date: Wed, 6 Dec 2017 09:39:23 +0000 Subject: [PATCH 4/6] Ensure memcpy is not called with NULL and 0 args in x509 module --- library/x509_crl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/x509_crl.c b/library/x509_crl.c index 8f98d8c92..9422457b5 100644 --- a/library/x509_crl.c +++ b/library/x509_crl.c @@ -294,7 +294,9 @@ int mbedtls_x509_crl_parse_der( mbedtls_x509_crl *chain, /* * Copy raw DER-encoded CRL */ - if( buflen != 0 && ( ( p = mbedtls_calloc( 1, buflen ) ) == NULL ) ) + if( buflen == 0 ) + return( MBEDTLS_ERR_X509_INVALID_FORMAT ); + else if( ( p = mbedtls_calloc( 1, buflen ) ) == NULL ) return( MBEDTLS_ERR_X509_ALLOC_FAILED ); memcpy( p, buf, buflen ); From c9d6226d2c7b7a99be6694c6014e4e4be1cc69d7 Mon Sep 17 00:00:00 2001 From: Andres Amaya Garcia Date: Tue, 12 Dec 2017 20:15:03 +0000 Subject: [PATCH 5/6] Change formatting of allocation check in x509_crl --- library/x509_crl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/x509_crl.c b/library/x509_crl.c index 9422457b5..0bb7236bd 100644 --- a/library/x509_crl.c +++ b/library/x509_crl.c @@ -296,7 +296,9 @@ int mbedtls_x509_crl_parse_der( mbedtls_x509_crl *chain, */ if( buflen == 0 ) return( MBEDTLS_ERR_X509_INVALID_FORMAT ); - else if( ( p = mbedtls_calloc( 1, buflen ) ) == NULL ) + + p = mbedtls_calloc( 1, buflen ); + if( p == NULL ) return( MBEDTLS_ERR_X509_ALLOC_FAILED ); memcpy( p, buf, buflen ); From e9124b943da5c30899cc75294f390d46ea23c995 Mon Sep 17 00:00:00 2001 From: Andres Amaya Garcia Date: Tue, 23 Jan 2018 20:03:52 +0000 Subject: [PATCH 6/6] Ensure that mbedtls_pk_parse_key() does not allocate 0 bytes --- library/pkparse.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/pkparse.c b/library/pkparse.c index f97d89ea1..491cecf50 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -1274,6 +1274,9 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk, { unsigned char *key_copy; + if( keylen == 0 ) + return( MBEDTLS_ERR_PK_KEY_INVALID_FORMAT ); + if( ( key_copy = mbedtls_calloc( 1, keylen ) ) == NULL ) return( MBEDTLS_ERR_PK_ALLOC_FAILED );