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. diff --git a/library/x509.c b/library/x509.c index ca2e907ef..c5b0161e7 100644 --- a/library/x509.c +++ b/library/x509.c @@ -459,6 +459,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 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. */ int mbedtls_x509_get_name( unsigned char **p, const unsigned char *end, mbedtls_x509_name *cur ) @@ -466,6 +471,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 ) @@ -475,14 +482,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; @@ -493,7 +503,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; } @@ -507,10 +520,30 @@ 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: + /* Skip the first element as we did not allocate it */ + allocated = head->next; + + 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 ) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 6263fba2c..8dd337995 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -415,6 +415,46 @@ 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 + 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..a3606f29b 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -818,6 +818,41 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_X509_CRT_PARSE_C */ +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 ret; + + memset( &head, 0, sizeof( head ) ); + + name = mbedtls_test_unhexify_alloc( rdn_sequence, &name_len ); + p = name; + + ret = mbedtls_x509_get_name( &p, ( name + name_len ), &head ); + if( ret == 0 ) + { + allocated = head.next; + + while( allocated != NULL ) + { + prev = allocated; + allocated = allocated->next; + + mbedtls_free( prev ); + } + } + + TEST_EQUAL( ret, 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 ) {