From ed79483aca30bff301cb420dcf5828530eea6603 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 f1d988aa7..cdc87e53e 100644 --- a/library/x509.c +++ b/library/x509.c @@ -468,6 +468,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 ) @@ -475,6 +480,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 ) @@ -484,14 +491,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; @@ -502,7 +512,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; } @@ -516,10 +529,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 05bb2c5d0e61b2804edfafc080e584c3be6ab5e4 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 000000000..358d1afa7 --- /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 db73d3b149becb510f3515b7e7e6f1b8dc3111ee Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 4 Oct 2022 16:49:16 +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 6263fba2c..1db09826e 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -415,6 +415,12 @@ mbedtls_x509_dn_get_next:"C=NL, O=PolarSSL, CN=PolarSSL Server 1":0x03:"C":1:"C= X509 Get Next DN #4 Consecutive Multivalue RDNs mbedtls_x509_dn_get_next:"C=NL, O=PolarSSL, title=Example, CN=PolarSSL Server 1":0x05:"C title":2:"C=NL + O=PolarSSL, title=Example + CN=PolarSSL Server 1" +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_HAS_ALG_SHA_1_VIA_MD_OR_PSA_BASED_ON_USE_PSA 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 60e703a94..e8a2bb971 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -818,6 +818,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_X509_CREATE_C:MBEDTLS_X509_USE_C:MBEDTLS_X509_CRT_PARSE_C:!MBEDTLS_X509_REMOVE_INFO */ void mbedtls_x509_dn_get_next( char * name_str, int next_merged, char * expected_oids, int exp_count, char * exp_dn_gets ) { From d0e3d45e96f83bc058dc5f4c94bc6c5b979a13de 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 1db09826e..8dd337995 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -415,9 +415,43 @@ mbedtls_x509_dn_get_next:"C=NL, O=PolarSSL, CN=PolarSSL Server 1":0x03:"C":1:"C= X509 Get Next DN #4 Consecutive Multivalue RDNs mbedtls_x509_dn_get_next:"C=NL, O=PolarSSL, title=Example, CN=PolarSSL Server 1":0x05:"C title":2:"C=NL + O=PolarSSL, title=Example + CN=PolarSSL Server 1" +# 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 3cd67584bc8f1c65937da74bdca74dbc758876bc 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 e8a2bb971..6947f3c19 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -819,21 +819,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; @@ -848,7 +848,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 11307a1933cbcb36ad9a066c6c345e4bf9f90127 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 cdc87e53e..403507413 100644 --- a/library/x509.c +++ b/library/x509.c @@ -469,7 +469,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 178ec96c89c30b271b43c0fe77615a059fafc3ca 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 403507413..812eba8af 100644 --- a/library/x509.c +++ b/library/x509.c @@ -538,14 +538,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 078250eb56869145af669cfd951a317feaddb746 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 812eba8af..199f82bc2 100644 --- a/library/x509.c +++ b/library/x509.c @@ -552,7 +552,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 01dd5480255f3555a090dd7bff856d9d767a62d9 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 6947f3c19..eb5f1aa78 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -824,7 +824,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; @@ -832,12 +832,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 ) { @@ -848,7 +845,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 2bb9c8a884c55bd2c63c84b3e7a33967a07598f6 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 eb5f1aa78..a3606f29b 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -824,10 +824,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;