Merge pull request #6670 from gilles-peskine-arm/pkcs7-use-after-free-20221127

PKCS7: Fix some memory management errors

library/pkcs7.c

@@ -103,15 +103,13 @@ static int pkcs7_get_content_info_type( unsigned char **p, unsigned char *end,
                                             | MBEDTLS_ASN1_SEQUENCE );
     if( ret != 0 ) {
         *p = start;
-        ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, ret );
+        return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, ret ) );
-        goto out;
     }
 
     ret = mbedtls_asn1_get_tag( p, end, &len, MBEDTLS_ASN1_OID );
     if( ret != 0 ) {
         *p = start;
-        ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, ret );
+        return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, ret ) );
-        goto out;
     }
 
     pkcs7->tag = MBEDTLS_ASN1_OID;
@@ -119,7 +117,6 @@ static int pkcs7_get_content_info_type( unsigned char **p, unsigned char *end,
     pkcs7->p = *p;
     *p += len;
 
-out:
     return( ret );
 }
 
@@ -153,8 +150,7 @@ static int pkcs7_get_digest_algorithm_set( unsigned char **p,
                                             | MBEDTLS_ASN1_SET );
     if( ret != 0 )
     {
-        ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_ALG, ret );
+        return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_ALG, ret ) );
-        goto out;
     }
 
     end = *p + len;
@@ -162,16 +158,14 @@ static int pkcs7_get_digest_algorithm_set( unsigned char **p,
     ret = mbedtls_asn1_get_alg_null( p, end, alg );
     if( ret != 0 )
     {
-        ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_ALG, ret );
+        return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_ALG, ret ) );
-        goto out;
     }
 
     /** For now, it assumes there is only one digest algorithm specified **/
     if ( *p != end )
-        ret = MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE;
+        return( MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE );
 
-out:
+    return( 0 );
-    return( ret );
 }
 
 /**
@@ -195,10 +189,9 @@ static int pkcs7_get_certificates( unsigned char **p, unsigned char *end,
                     | MBEDTLS_ASN1_CONTEXT_SPECIFIC ) ) != 0 )
     {
         if( ret == MBEDTLS_ERR_ASN1_UNEXPECTED_TAG )
-            ret = 0;
+            return( 0 );
         else
-            ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_FORMAT, ret );
+            return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_FORMAT, ret ) );
-        goto out;
     }
     start = *p;
     end_set = *p + len1;
@@ -207,8 +200,7 @@ static int pkcs7_get_certificates( unsigned char **p, unsigned char *end,
             | MBEDTLS_ASN1_SEQUENCE );
     if( ret != 0 )
     {
-        ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_CERT, ret );
+        return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_CERT, ret ) );
-        goto out;
     }
 
     end_cert = *p + len2;
@@ -221,15 +213,13 @@ static int pkcs7_get_certificates( unsigned char **p, unsigned char *end,
      */
     if ( end_cert != end_set )
     {
-        ret = MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE;
+        return( MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE );
-        goto out;
     }
 
     *p = start;
     if( ( ret = mbedtls_x509_crt_parse_der( certs, *p, len1 ) ) < 0 )
     {
-        ret = MBEDTLS_ERR_PKCS7_INVALID_CERT;
+        return( MBEDTLS_ERR_PKCS7_INVALID_CERT );
-        goto out;
     }
 
     *p = *p + len1;
@@ -238,10 +228,7 @@ static int pkcs7_get_certificates( unsigned char **p, unsigned char *end,
      * Since in this version we strictly support single certificate, and reaching
      * here implies we have parsed successfully, we return 1.
      */
-    ret = 1;
+    return( 1 );
-
-out:
-    return( ret );
 }
 
 /**
@@ -255,7 +242,7 @@ static int pkcs7_get_signature( unsigned char **p, unsigned char *end,
 
     ret = mbedtls_asn1_get_tag( p, end, &len, MBEDTLS_ASN1_OCTET_STRING );
     if( ret != 0 )
-        goto out;
+        return( ret );
 
     signature->tag = MBEDTLS_ASN1_OCTET_STRING;
     signature->len = len;
@@ -263,8 +250,7 @@ static int pkcs7_get_signature( unsigned char **p, unsigned char *end,
 
     *p = *p + len;
 
-out:
+    return( 0 );
-    return( ret );
 }
 
 /**
@@ -367,6 +353,7 @@ static void pkcs7_free_signer_info( mbedtls_pkcs7_signer_info *signer )
         name_cur = name_cur->next;
         mbedtls_free( name_prv );
     }
+    signer->issuer.next = NULL;
 }
 
 /**
@@ -382,34 +369,32 @@ static int pkcs7_get_signers_info_set( unsigned char **p, unsigned char *end,
     int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
     int count = 0;
     size_t len = 0;
-    mbedtls_pkcs7_signer_info *signer, *prev;
 
     ret = mbedtls_asn1_get_tag( p, end, &len, MBEDTLS_ASN1_CONSTRUCTED
                                 | MBEDTLS_ASN1_SET );
     if( ret != 0 )
     {
-        ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO, ret );
+        return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO, ret ) );
-        goto out;
     }
 
     /* Detect zero signers */
     if( len == 0 )
     {
-        ret = 0;
+        return( 0 );
-        goto out;
     }
 
     end_set = *p + len;
 
     ret = pkcs7_get_signer_info( p, end_set, signers_set );
     if( ret != 0 )
-        goto out;
+        goto cleanup;
     count++;
 
-    prev = signers_set;
+    mbedtls_pkcs7_signer_info *prev = signers_set;
     while( *p != end_set )
     {
-        signer = mbedtls_calloc( 1, sizeof( mbedtls_pkcs7_signer_info ) );
+        mbedtls_pkcs7_signer_info *signer =
+            mbedtls_calloc( 1, sizeof( mbedtls_pkcs7_signer_info ) );
         if( !signer )
         {
             ret = MBEDTLS_ERR_PKCS7_ALLOC_FAILED;
@@ -426,21 +411,19 @@ static int pkcs7_get_signers_info_set( unsigned char **p, unsigned char *end,
         count++;
     }
 
-    ret = count;
+    return( count );
-    goto out;
 
 cleanup:
-    signer = signers_set->next;
     pkcs7_free_signer_info( signers_set );
-    while( signer )
+    mbedtls_pkcs7_signer_info *signer = signers_set->next;
+    while( signer != NULL )
     {
         prev = signer;
         signer = signer->next;
         pkcs7_free_signer_info( prev );
         mbedtls_free( prev );
     }
-
+    signers_set->next = NULL;
-out:
     return( ret );
 }
 
@@ -470,8 +453,7 @@ static int pkcs7_get_signed_data( unsigned char *buf, size_t buflen,
                                 | MBEDTLS_ASN1_SEQUENCE );
     if( ret != 0 )
     {
-        ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_FORMAT, ret );
+        return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_FORMAT, ret ) );
-        goto out;
     }
 
     end_set = p + len;
@@ -479,37 +461,35 @@ static int pkcs7_get_signed_data( unsigned char *buf, size_t buflen,
     /* Get version of signed data */
     ret = pkcs7_get_version( &p, end_set, &signed_data->version );
     if( ret != 0 )
-        goto out;
+        return( ret );
 
     /* Get digest algorithm */
     ret = pkcs7_get_digest_algorithm_set( &p, end_set,
             &signed_data->digest_alg_identifiers );
     if( ret != 0 )
-        goto out;
+        return( ret );
 
     ret = mbedtls_oid_get_md_alg( &signed_data->digest_alg_identifiers, &md_alg );
     if( ret != 0 )
     {
-        ret = MBEDTLS_ERR_PKCS7_INVALID_ALG;
+        return( MBEDTLS_ERR_PKCS7_INVALID_ALG );
-        goto out;
     }
 
     /* Do not expect any content */
     ret = pkcs7_get_content_info_type( &p, end_set, &signed_data->content.oid );
     if( ret != 0 )
-        goto out;
+        return( ret );
 
     if( MBEDTLS_OID_CMP( MBEDTLS_OID_PKCS7_DATA, &signed_data->content.oid ) )
     {
-        ret = MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO;
+        return( MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO );
-        goto out;
     }
 
     /* Look for certificates, there may or may not be any */
     mbedtls_x509_crt_init( &signed_data->certs );
     ret = pkcs7_get_certificates( &p, end_set, &signed_data->certs );
     if( ret < 0 )
-        goto out;
+        return( ret );
 
     signed_data->no_of_certs = ret;
 
@@ -524,18 +504,15 @@ static int pkcs7_get_signed_data( unsigned char *buf, size_t buflen,
     /* Get signers info */
     ret = pkcs7_get_signers_info_set( &p, end_set, &signed_data->signers );
     if( ret < 0 )
-        goto out;
+        return( ret );
 
     signed_data->no_of_signers = ret;
 
     /* Don't permit trailing data */
     if ( p != end )
-        ret = MBEDTLS_ERR_PKCS7_INVALID_FORMAT;
+        return( MBEDTLS_ERR_PKCS7_INVALID_FORMAT );
-    else
-        ret = 0;
 
-out:
+    return( 0 );
-    return( ret );
 }
 
 int mbedtls_pkcs7_parse_der( mbedtls_pkcs7 *pkcs7, const unsigned char *buf,
@@ -547,10 +524,9 @@ int mbedtls_pkcs7_parse_der( mbedtls_pkcs7 *pkcs7, const unsigned char *buf,
     int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
     int isoidset = 0;
 
-    if( !pkcs7 )
+    if( pkcs7 == NULL )
     {
-        ret = MBEDTLS_ERR_PKCS7_BAD_INPUT_DATA;
+        return( MBEDTLS_ERR_PKCS7_BAD_INPUT_DATA );
-        goto out;
     }
 
     /* make an internal copy of the buffer for parsing */
@@ -630,15 +606,13 @@ static int mbedtls_pkcs7_data_or_hash_verify( mbedtls_pkcs7 *pkcs7,
 
     if( pkcs7->signed_data.no_of_signers == 0 )
     {
-        ret = MBEDTLS_ERR_PKCS7_INVALID_CERT;
+        return( MBEDTLS_ERR_PKCS7_INVALID_CERT );
-        goto out;
     }
 
     if( mbedtls_x509_time_is_past( &cert->valid_to ) ||
         mbedtls_x509_time_is_future( &cert->valid_from ))
     {
-        ret = MBEDTLS_ERR_PKCS7_CERT_DATE_INVALID;
+        return( MBEDTLS_ERR_PKCS7_CERT_DATE_INVALID );
-        goto out;
     }
 
     /*
@@ -672,9 +646,9 @@ static int mbedtls_pkcs7_data_or_hash_verify( mbedtls_pkcs7 *pkcs7,
 
         hash = mbedtls_calloc( mbedtls_md_get_size( md_info ), 1 );
         if( hash == NULL ) {
-            ret = MBEDTLS_ERR_PKCS7_ALLOC_FAILED;
+            return( MBEDTLS_ERR_PKCS7_ALLOC_FAILED );
-            goto out;
         }
+        /* BEGIN must free hash before jumping out */
         if( is_data_hash )
         {
             if( datalen != mbedtls_md_get_size( md_info ))
@@ -697,12 +671,12 @@ static int mbedtls_pkcs7_data_or_hash_verify( mbedtls_pkcs7 *pkcs7,
                                  mbedtls_md_get_size( md_info ),
                                  signer->sig.p, signer->sig.len );
         mbedtls_free( hash );
+        /* END must free hash before jumping out */
 
         if( ret == 0 )
             break;
     }
 
-out:
     return( ret );
 }
 int mbedtls_pkcs7_signed_data_verify( mbedtls_pkcs7 *pkcs7,

tests/suites/test_suite_pkcs7.data

@@ -62,6 +62,14 @@ PKCS7 Signed Data Parse Failure Corrupt signerInfo.serial #15.2
 depends_on:MBEDTLS_SHA256_C
 pkcs7_parse:"data_files/pkcs7_signerInfo_serial_invalid_size.der":MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO
 
+pkcs7_get_signers_info_set error handling (6213931373035520)
+depends_on:MBEDTLS_RIPEMD160_C
+pkcs7_parse:"data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der":MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG
+
+pkcs7_get_signers_info_set error handling (4541044530479104)
+depends_on:MBEDTLS_RIPEMD160_C
+pkcs7_parse:"data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der":MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG
+
 PKCS7 Only Signed Data Parse Pass #15
 depends_on:MBEDTLS_SHA256_C:MBEDTLS_RSA_C
 pkcs7_parse:"data_files/pkcs7_data_cert_signeddata_sha256.der":MBEDTLS_PKCS7_SIGNED_DATA

tests/suites/test_suite_pkcs7.function

@@ -26,10 +26,10 @@ void pkcs7_parse( char *pkcs7_file, int res_expect )
     mbedtls_pkcs7_init( &pkcs7 );
 
     res = mbedtls_pk_load_file( pkcs7_file, &pkcs7_buf, &buflen );
-    TEST_ASSERT( res == 0 );
+    TEST_EQUAL( res, 0 );
 
     res = mbedtls_pkcs7_parse_der( &pkcs7, pkcs7_buf, buflen );
-    TEST_ASSERT( res == res_expect );
+    TEST_EQUAL( res, res_expect );
 
 exit:
     mbedtls_free( pkcs7_buf );
@@ -54,22 +54,22 @@ void pkcs7_verify( char *pkcs7_file, char *crt, char *filetobesigned, int do_has
     mbedtls_pkcs7 pkcs7;
     mbedtls_x509_crt x509;
 
-    USE_PSA_INIT();
-
     mbedtls_pkcs7_init( &pkcs7 );
     mbedtls_x509_crt_init( &x509 );
 
+    USE_PSA_INIT();
+
     res = mbedtls_x509_crt_parse_file( &x509, crt );
-    TEST_ASSERT( res == 0 );
+    TEST_EQUAL( res, 0 );
 
     res = mbedtls_pk_load_file( pkcs7_file, &pkcs7_buf, &buflen );
-    TEST_ASSERT( res == 0 );
+    TEST_EQUAL( res, 0 );
 
     res = mbedtls_pkcs7_parse_der( &pkcs7, pkcs7_buf, buflen );
-    TEST_ASSERT( res == MBEDTLS_PKCS7_SIGNED_DATA );
+    TEST_EQUAL( res, MBEDTLS_PKCS7_SIGNED_DATA );
 
     res = stat( filetobesigned, &st );
-    TEST_ASSERT( res == 0 );
+    TEST_EQUAL( res, 0 );
 
     file = fopen( filetobesigned, "rb" );
     TEST_ASSERT( file != NULL );
@@ -79,18 +79,18 @@ void pkcs7_verify( char *pkcs7_file, char *crt, char *filetobesigned, int do_has
     TEST_ASSERT( data != NULL );
 
     buflen = fread( (void *)data , sizeof( unsigned char ), datalen, file );
-    TEST_ASSERT( buflen == datalen );
+    TEST_EQUAL( buflen, datalen );
     fclose( file );
 
     if( do_hash_alg )
     {
         res = mbedtls_oid_get_md_alg( &pkcs7.signed_data.digest_alg_identifiers, &md_alg );
-        TEST_ASSERT( res == 0 );
+        TEST_EQUAL( res, 0 );
-        TEST_ASSERT( md_alg == (mbedtls_md_type_t) do_hash_alg );
+        TEST_EQUAL( md_alg, (mbedtls_md_type_t) do_hash_alg );
         md_info = mbedtls_md_info_from_type( md_alg );
 
         res = mbedtls_md( md_info, data, datalen, hash );
-        TEST_ASSERT( res == 0 );
+        TEST_EQUAL( res, 0 );
 
         res = mbedtls_pkcs7_signed_hash_verify( &pkcs7, &x509, hash, sizeof(hash) );
     }
@@ -98,7 +98,7 @@ void pkcs7_verify( char *pkcs7_file, char *crt, char *filetobesigned, int do_has
     {
         res = mbedtls_pkcs7_signed_data_verify( &pkcs7, &x509, data, datalen );
     }
-    TEST_ASSERT( res == res_expect );
+    TEST_EQUAL( res, res_expect );
 
 exit:
     mbedtls_x509_crt_free( &x509 );
@@ -127,28 +127,28 @@ void pkcs7_verify_multiple_signers( char *pkcs7_file, char *crt1, char *crt2, ch
     mbedtls_x509_crt x509_1;
     mbedtls_x509_crt x509_2;
 
-    USE_PSA_INIT();
-
     mbedtls_pkcs7_init( &pkcs7 );
     mbedtls_x509_crt_init( &x509_1 );
     mbedtls_x509_crt_init( &x509_2 );
 
+    USE_PSA_INIT();
+
     res = mbedtls_pk_load_file( pkcs7_file, &pkcs7_buf, &buflen );
-    TEST_ASSERT( res == 0 );
+    TEST_EQUAL( res, 0 );
 
     res = mbedtls_pkcs7_parse_der( &pkcs7, pkcs7_buf, buflen );
-    TEST_ASSERT( res == MBEDTLS_PKCS7_SIGNED_DATA );
+    TEST_EQUAL( res, MBEDTLS_PKCS7_SIGNED_DATA );
 
-    TEST_ASSERT( pkcs7.signed_data.no_of_signers == 2 );
+    TEST_EQUAL( pkcs7.signed_data.no_of_signers, 2 );
 
     res = mbedtls_x509_crt_parse_file( &x509_1, crt1 );
-    TEST_ASSERT( res == 0 );
+    TEST_EQUAL( res, 0 );
 
     res = mbedtls_x509_crt_parse_file( &x509_2, crt2 );
-    TEST_ASSERT( res == 0 );
+    TEST_EQUAL( res, 0 );
 
     res = stat( filetobesigned, &st );
-    TEST_ASSERT( res == 0 );
+    TEST_EQUAL( res, 0 );
 
     file = fopen( filetobesigned, "rb" );
     TEST_ASSERT( file != NULL );
@@ -156,32 +156,32 @@ void pkcs7_verify_multiple_signers( char *pkcs7_file, char *crt1, char *crt2, ch
     datalen = st.st_size;
     ASSERT_ALLOC( data, datalen );
     buflen = fread( ( void * )data , sizeof( unsigned char ), datalen, file );
-    TEST_ASSERT( buflen == datalen );
+    TEST_EQUAL( buflen, datalen );
 
     fclose( file );
 
     if( do_hash_alg )
     {
         res = mbedtls_oid_get_md_alg( &pkcs7.signed_data.digest_alg_identifiers, &md_alg );
-        TEST_ASSERT( res == 0 );
+        TEST_EQUAL( res, 0 );
-        TEST_ASSERT( md_alg == MBEDTLS_MD_SHA256 );
+        TEST_EQUAL( md_alg, MBEDTLS_MD_SHA256 );
 
         md_info = mbedtls_md_info_from_type( md_alg );
 
         res = mbedtls_md( md_info, data, datalen, hash );
-        TEST_ASSERT( res == 0 );
+        TEST_EQUAL( res, 0 );
 
         res = mbedtls_pkcs7_signed_hash_verify( &pkcs7, &x509_1, hash, sizeof(hash) );
-        TEST_ASSERT( res == res_expect );
+        TEST_EQUAL( res, res_expect );
     }
     else
     {
         res = mbedtls_pkcs7_signed_data_verify( &pkcs7, &x509_1, data, datalen );
-        TEST_ASSERT( res == res_expect );
+        TEST_EQUAL( res, res_expect );
     }
 
     res = mbedtls_pkcs7_signed_data_verify( &pkcs7, &x509_2, data, datalen );
-    TEST_ASSERT( res == res_expect );
+    TEST_EQUAL( res, res_expect );
 
 exit:
     mbedtls_x509_crt_free( &x509_1 );