From 8c176b487adebb2d3c21b8708e35cb0bec13265c Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 4 Oct 2022 18:12:06 +0100 Subject: [PATCH 01/10] Free structs in mbedtls_x509_get_name() on error mbedtls_x509_get_name() allocates a linked list of mbedtls_x509_name structs but does not free these when there is an error, leaving the caller to free them itself. Change this to cleanup these objects within the function in case of an error. Signed-off-by: David Horstmann --- library/x509.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/library/x509.c b/library/x509.c index 3997ebd1f3..5d8338418c 100644 --- a/library/x509.c +++ b/library/x509.c @@ -424,6 +424,11 @@ static int x509_get_attr_type_value( unsigned char **p, * For the general case we still use a flat list, but we mark elements of the * same set so that they are "merged" together in the functions that consume * this list, eg mbedtls_x509_dn_gets(). + * + * On success, this function allocates a linked list starting at cur->next + * that must later be free'd by the caller using mbedtls_free(). In error + * cases, this function frees all allocated memory internally and the caller + * has no freeing responsibilities. */ int mbedtls_x509_get_name( unsigned char **p, const unsigned char *end, mbedtls_x509_name *cur ) @@ -431,6 +436,8 @@ int mbedtls_x509_get_name( unsigned char **p, const unsigned char *end, int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; size_t set_len; const unsigned char *end_set; + mbedtls_x509_name *head = cur; + mbedtls_x509_name *prev, *allocated; /* don't use recursion, we'd risk stack overflow if not optimized */ while( 1 ) @@ -440,14 +447,17 @@ int mbedtls_x509_get_name( unsigned char **p, const unsigned char *end, */ if( ( ret = mbedtls_asn1_get_tag( p, end, &set_len, MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SET ) ) != 0 ) - return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_X509_INVALID_NAME, ret ) ); + { + ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_X509_INVALID_NAME, ret ); + goto error; + } end_set = *p + set_len; while( 1 ) { if( ( ret = x509_get_attr_type_value( p, end_set, cur ) ) != 0 ) - return( ret ); + goto error; if( *p == end_set ) break; @@ -458,7 +468,10 @@ int mbedtls_x509_get_name( unsigned char **p, const unsigned char *end, cur->next = mbedtls_calloc( 1, sizeof( mbedtls_x509_name ) ); if( cur->next == NULL ) - return( MBEDTLS_ERR_X509_ALLOC_FAILED ); + { + ret = MBEDTLS_ERR_X509_ALLOC_FAILED; + goto error; + } cur = cur->next; } @@ -472,10 +485,35 @@ int mbedtls_x509_get_name( unsigned char **p, const unsigned char *end, cur->next = mbedtls_calloc( 1, sizeof( mbedtls_x509_name ) ); if( cur->next == NULL ) - return( MBEDTLS_ERR_X509_ALLOC_FAILED ); + { + ret = MBEDTLS_ERR_X509_ALLOC_FAILED; + goto error; + } cur = cur->next; } + +error: + prev = NULL; + + /* Skip the first element as we did not allocate it */ + allocated = head->next; + + /* Make sure we cannot be followed along this list */ + head->next = NULL; + + while( allocated != NULL ) + { + prev = allocated; + allocated = allocated->next; + + mbedtls_platform_zeroize( prev, sizeof( *prev ) ); + mbedtls_free( prev ); + } + + mbedtls_platform_zeroize( head, sizeof( *head ) ); + + return ret; } static int x509_parse_int( unsigned char **p, size_t n, int *res ) From 854be059497363130db970a65b2146121e329c7b Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 5 Oct 2022 12:06:23 +0100 Subject: [PATCH 02/10] Add ChangeLog entry for memory leak fix Signed-off-by: David Horstmann --- ChangeLog.d/fix_x509_get_name_mem_leak.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ChangeLog.d/fix_x509_get_name_mem_leak.txt diff --git a/ChangeLog.d/fix_x509_get_name_mem_leak.txt b/ChangeLog.d/fix_x509_get_name_mem_leak.txt new file mode 100644 index 0000000000..358d1afa72 --- /dev/null +++ b/ChangeLog.d/fix_x509_get_name_mem_leak.txt @@ -0,0 +1,4 @@ +Bugfix + * Fix memory leak in ssl_parse_certificate_request() caused by + mbedtls_x509_get_name() not freeing allocated objects in case of error. + Change mbedtls_x509_get_name() to clean up allocated objects on error. From 77ecc6e4e9cdb85622eca3b91d410893834fa363 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 5 Oct 2022 13:08:45 +0100 Subject: [PATCH 03/10] Add mbedtls_x509_get_name memory leak unit test Introduce a unit test to test mbedtls_x509_get_name() and add a testcase with a corrupt DER-encoded name that causes mbedtls_x509_get_name() to have to cleanup things it is allocated. If it fails to do this, a memory leak is detected under Asan builds. Signed-off-by: David Horstmann --- tests/suites/test_suite_x509parse.data | 6 ++++ tests/suites/test_suite_x509parse.function | 36 ++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 066d6e49f5..ed242a4768 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -427,6 +427,12 @@ X509 Get Modified DN #5 Name exactly 255 bytes, ending with comma requiring esca depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_SHA1_C mbedtls_x509_dn_gets_subject_replace:"data_files/server1.crt":"12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234,":"":MBEDTLS_ERR_X509_BUFFER_TOO_SMALL +X509 Get Name Valid DN +mbedtls_x509_get_name:"310B3009060355040613024E4C3111300F060355040A0C08506F6C617253534C3119301706035504030C10506F6C617253534C2054657374204341":0 + +X509 Get Name Corrupted DN Mem Leak +mbedtls_x509_get_name:"310B3009060355040613024E4C3111300F060355040A0C08506F6C617253534C3019301706035504030C10506F6C617253534C2054657374204341":MBEDTLS_ERR_X509_INVALID_NAME + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG + X509 Time Expired #1 depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_HAVE_TIME_DATE:MBEDTLS_SHA1_C mbedtls_x509_time_is_past:"data_files/server1.crt":"valid_from":1 diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index 77f3d2338f..0af56d215e 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -799,6 +799,42 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_X509_CRT_PARSE_C */ +void mbedtls_x509_get_name( char * hex_name, int exp_ret ) +{ + unsigned char *name; + unsigned char *p; + size_t name_len; + mbedtls_x509_name head; + mbedtls_x509_name *allocated, *prev; + int res; + + name = mbedtls_test_unhexify_alloc( hex_name, &name_len ); + p = name; + + res = mbedtls_x509_get_name( &p, ( name + name_len ), &head ); + + if( res == 0 ) + { + allocated = head.next; + head.next = NULL; + prev = NULL; + + while( allocated != NULL ) + { + prev = allocated; + allocated = allocated->next; + + mbedtls_free( prev ); + } + } + + TEST_ASSERT( res == exp_ret ); + + mbedtls_free( name ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_FS_IO:MBEDTLS_X509_CRT_PARSE_C */ void mbedtls_x509_time_is_past( char * crt_file, char * entity, int result ) { From 94cbd30a247a20b87592a3f729108d0de9ff5414 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 17 Oct 2022 17:42:19 +0100 Subject: [PATCH 04/10] Add explanatory comments to raw DER test data Break down the DER-encoded ASN.1 test data into its structure in a comment and explain it, to make it easier to understand where the data came from and how it is corrupted. Signed-off-by: David Horstmann --- tests/suites/test_suite_x509parse.data | 34 ++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index ed242a4768..6ae39ff6bc 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -427,9 +427,43 @@ X509 Get Modified DN #5 Name exactly 255 bytes, ending with comma requiring esca depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_SHA1_C mbedtls_x509_dn_gets_subject_replace:"data_files/server1.crt":"12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234,":"":MBEDTLS_ERR_X509_BUFFER_TOO_SMALL +# Parse the following valid DN: +# +# 31 0B <- Set of +# 30 09 <- Sequence of +# 06 03 55 04 06 <- OID 2.5.4.6 countryName (C) +# 13 02 4E 4C <- PrintableString "NL" +# 31 11 <- Set of +# 30 0F <- Sequence of +# 06 03 55 04 0A <- OID 2.5.4.10 organizationName (O) +# 0C 08 50 6F 6C 61 72 53 53 4C <- UTF8String "PolarSSL" +# 31 19 <- Set of +# 30 17 <- Sequence of +# 06 03 55 04 03 <- OID 2.5.4.3 commonName (CN) +# 0C 10 50 6F 6C 61 72 53 53 4C 20 54 65 73 74 20 43 41 <- UTF8String "PolarSSL Test CA" +# X509 Get Name Valid DN mbedtls_x509_get_name:"310B3009060355040613024E4C3111300F060355040A0C08506F6C617253534C3119301706035504030C10506F6C617253534C2054657374204341":0 +# Parse the following corrupted DN: +# +# 31 0B <- Set of +# 30 09 <- Sequence of +# 06 03 55 04 06 <- OID 2.5.4.6 countryName (C) +# 13 02 4E 4C <- PrintableString "NL" +# 31 11 <- Set of +# 30 0F <- Sequence of +# 06 03 55 04 0A <- OID 2.5.4.10 organizationName (O) +# 0C 08 50 6F 6C 61 72 53 53 4C <- UTF8String "PolarSSL" +# 30 19 <- Sequence of (corrupted) +# 30 17 <- Sequence of +# 06 03 55 04 03 <- OID 2.5.4.3 commonName (CN) +# 0C 10 50 6F 6C 61 72 53 53 4C 20 54 65 73 74 20 43 41 <- UTF8String "PolarSSL Test CA" +# +# The third 'Set of' is corrupted to instead be a 'Sequence of', causing an +# error and forcing mbedtls_x509_get_name() to clean up the names it has +# already allocated. +# X509 Get Name Corrupted DN Mem Leak mbedtls_x509_get_name:"310B3009060355040613024E4C3111300F060355040A0C08506F6C617253534C3019301706035504030C10506F6C617253534C2054657374204341":MBEDTLS_ERR_X509_INVALID_NAME + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG From 4a67c351a8bdaae2bbaa29eeccf88c758fb2b41d Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 17 Oct 2022 17:59:10 +0100 Subject: [PATCH 05/10] Improve X509 DN test naming Signed-off-by: David Horstmann --- tests/suites/test_suite_x509parse.function | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index 0af56d215e..b7d01ae357 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -800,21 +800,21 @@ exit: /* END_CASE */ /* BEGIN_CASE depends_on:MBEDTLS_X509_CRT_PARSE_C */ -void mbedtls_x509_get_name( char * hex_name, int exp_ret ) +void mbedtls_x509_get_name( char * rdn_sequence, int exp_ret ) { unsigned char *name; unsigned char *p; size_t name_len; mbedtls_x509_name head; mbedtls_x509_name *allocated, *prev; - int res; + int ret; - name = mbedtls_test_unhexify_alloc( hex_name, &name_len ); + name = mbedtls_test_unhexify_alloc( rdn_sequence, &name_len ); p = name; - res = mbedtls_x509_get_name( &p, ( name + name_len ), &head ); + ret = mbedtls_x509_get_name( &p, ( name + name_len ), &head ); - if( res == 0 ) + if( ret == 0 ) { allocated = head.next; head.next = NULL; @@ -829,7 +829,7 @@ void mbedtls_x509_get_name( char * hex_name, int exp_ret ) } } - TEST_ASSERT( res == exp_ret ); + TEST_ASSERT( ret == exp_ret ); mbedtls_free( name ); } From 5ad5e1657db134209189185475e7aeef9144919c Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 17 Oct 2022 18:10:23 +0100 Subject: [PATCH 06/10] Clarify wording on allocation Signed-off-by: David Horstmann --- library/x509.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/x509.c b/library/x509.c index 5d8338418c..6f3fbb623e 100644 --- a/library/x509.c +++ b/library/x509.c @@ -425,7 +425,7 @@ static int x509_get_attr_type_value( unsigned char **p, * same set so that they are "merged" together in the functions that consume * this list, eg mbedtls_x509_dn_gets(). * - * On success, this function allocates a linked list starting at cur->next + * On success, this function may allocate a linked list starting at cur->next * that must later be free'd by the caller using mbedtls_free(). In error * cases, this function frees all allocated memory internally and the caller * has no freeing responsibilities. From e6917d05d3e420c3d2a459b8ce6297846b76a4d6 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 18 Oct 2022 17:42:22 +0100 Subject: [PATCH 07/10] Remove unnecessary NULL assignments Signed-off-by: David Horstmann --- library/x509.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/library/x509.c b/library/x509.c index 6f3fbb623e..586c506284 100644 --- a/library/x509.c +++ b/library/x509.c @@ -494,14 +494,9 @@ int mbedtls_x509_get_name( unsigned char **p, const unsigned char *end, } error: - prev = NULL; - /* Skip the first element as we did not allocate it */ allocated = head->next; - /* Make sure we cannot be followed along this list */ - head->next = NULL; - while( allocated != NULL ) { prev = allocated; From 670a993dcdf872bd22dabf427e85fb83f2d4421e Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 18 Oct 2022 18:11:13 +0100 Subject: [PATCH 08/10] Fix incorrect return style Signed-off-by: David Horstmann --- library/x509.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/x509.c b/library/x509.c index 586c506284..7a501bca8d 100644 --- a/library/x509.c +++ b/library/x509.c @@ -508,7 +508,7 @@ error: mbedtls_platform_zeroize( head, sizeof( *head ) ); - return ret; + return( ret ); } static int x509_parse_int( unsigned char **p, size_t n, int *res ) From 8eb3ed56f8d6ddfd96dcc15456843af78aa223b0 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 19 Oct 2022 17:13:57 +0100 Subject: [PATCH 09/10] Minor fixes to x509_get_name() test function Signed-off-by: David Horstmann --- tests/suites/test_suite_x509parse.function | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index b7d01ae357..c03becc4e1 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -805,7 +805,7 @@ void mbedtls_x509_get_name( char * rdn_sequence, int exp_ret ) unsigned char *name; unsigned char *p; size_t name_len; - mbedtls_x509_name head; + mbedtls_x509_name head = { 0 }; mbedtls_x509_name *allocated, *prev; int ret; @@ -813,12 +813,9 @@ void mbedtls_x509_get_name( char * rdn_sequence, int exp_ret ) p = name; ret = mbedtls_x509_get_name( &p, ( name + name_len ), &head ); - if( ret == 0 ) { allocated = head.next; - head.next = NULL; - prev = NULL; while( allocated != NULL ) { @@ -829,7 +826,7 @@ void mbedtls_x509_get_name( char * rdn_sequence, int exp_ret ) } } - TEST_ASSERT( ret == exp_ret ); + TEST_EQUAL( ret, exp_ret ); mbedtls_free( name ); } From 6c4226ce9506d68ebc4b0e13b665a20d52ac3a95 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 20 Oct 2022 10:18:37 +0100 Subject: [PATCH 10/10] Change brace initialization to memset Signed-off-by: David Horstmann --- tests/suites/test_suite_x509parse.function | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index c03becc4e1..5828c3bc73 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -805,10 +805,12 @@ void mbedtls_x509_get_name( char * rdn_sequence, int exp_ret ) unsigned char *name; unsigned char *p; size_t name_len; - mbedtls_x509_name head = { 0 }; + mbedtls_x509_name head; mbedtls_x509_name *allocated, *prev; int ret; + memset( &head, 0, sizeof( head ) ); + name = mbedtls_test_unhexify_alloc( rdn_sequence, &name_len ); p = name;