commit 8c176b487adebb2d3c21b8708e35cb0bec13265c
author David Horstmann <david.horstmann@arm.com> Tue Oct 04 18:12:06 2022 +0100
committer David Horstmann <david.horstmann@arm.com> Wed Oct 05 12:57:32 2022 +0100
tree cef50e86254ad445d2c95c7e26d89c800f392a99
parent 30e0623c7c9c8019f87e807f59e43259e45c1f11

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 <david.horstmann@arm.com>

library/x509.c

diff --git a/library/x509.c b/library/x509.c
index 3997ebd..5d83384 100644
--- a/library/x509.c
+++ b/library/x509.c
@@ -424,6 +424,11 @@
  * 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 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 @@
          */
         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 @@
             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 @@
         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 )