Merge pull request #6483 from gilles-peskine-arm/psa-pkparse-pkwrite-2.28
Backport 2.28: PSA with RSA requires PK_WRITE and PK_PARSE
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 0000000..358d1af
--- /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 5a42911..a7ebd25 100644
--- a/library/x509.c
+++ b/library/x509.c
@@ -415,6 +415,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 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 )
@@ -422,6 +427,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 )
@@ -431,14 +438,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;
@@ -449,7 +459,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;
}
@@ -463,10 +476,30 @@
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 066d6e4..6ae39ff 100644
--- a/tests/suites/test_suite_x509parse.data
+++ b/tests/suites/test_suite_x509parse.data
@@ -427,6 +427,46 @@
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
+
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 77f3d23..5828c3b 100644
--- a/tests/suites/test_suite_x509parse.function
+++ b/tests/suites/test_suite_x509parse.function
@@ -799,6 +799,41 @@
}
/* 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_FS_IO:MBEDTLS_X509_CRT_PARSE_C */
void mbedtls_x509_time_is_past( char * crt_file, char * entity, int result )
{