From ed79483aca30bff301cb420dcf5828530eea6603 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 4 Oct 2022 18:12:06 +0100 Subject: [PATCH] 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 f1d988aa75..cdc87e53eb 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 )