diff --git a/library/pkcs7.c b/library/pkcs7.c index ca0170a6d..e4238b6a3 100644 --- a/library/pkcs7.c +++ b/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 ); - goto out; + return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, ret ) ); } 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 ); - goto out; + return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, ret ) ); } 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 ); - goto out; + return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_ALG, ret ) ); } 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 ); - goto out; + return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_ALG, ret ) ); } /** 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( ret ); + return( 0 ); } /** @@ -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 ); - goto out; + return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_FORMAT, ret ) ); } 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 ); - goto out; + return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_CERT, ret ) ); } 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; - goto out; + return( MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE ); } *p = start; if( ( ret = mbedtls_x509_crt_parse_der( certs, *p, len1 ) ) < 0 ) { - ret = MBEDTLS_ERR_PKCS7_INVALID_CERT; - goto out; + return( MBEDTLS_ERR_PKCS7_INVALID_CERT ); } *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; - -out: - return( ret ); + return( 1 ); } /** @@ -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( ret ); + return( 0 ); } /** @@ -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 ); - goto out; + return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO, ret ) ); } /* Detect zero signers */ if( len == 0 ) { - ret = 0; - goto out; + return( 0 ); } 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; - goto out; + return( count ); 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 ); } - -out: + signers_set->next = NULL; 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 ); - goto out; + return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_PKCS7_INVALID_FORMAT, ret ) ); } 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; - goto out; + return( MBEDTLS_ERR_PKCS7_INVALID_ALG ); } /* 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; - goto out; + return( MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO ); } /* 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; - else - ret = 0; + return( MBEDTLS_ERR_PKCS7_INVALID_FORMAT ); -out: - return( ret ); + return( 0 ); } 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; - goto out; + return( MBEDTLS_ERR_PKCS7_BAD_INPUT_DATA ); } /* 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; - goto out; + return( MBEDTLS_ERR_PKCS7_INVALID_CERT ); } if( mbedtls_x509_time_is_past( &cert->valid_to ) || mbedtls_x509_time_is_future( &cert->valid_from )) { - ret = MBEDTLS_ERR_PKCS7_CERT_DATE_INVALID; - goto out; + return( MBEDTLS_ERR_PKCS7_CERT_DATE_INVALID ); } /* @@ -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; - goto out; + return( MBEDTLS_ERR_PKCS7_ALLOC_FAILED ); } + /* 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, diff --git a/tests/data_files/pkcs7_get_signers_info_set-leak-fuzz_pkcs7-4541044530479104.der b/tests/data_files/pkcs7_get_signers_info_set-leak-fuzz_pkcs7-4541044530479104.der new file mode 100644 index 000000000..51aef0d09 Binary files /dev/null and b/tests/data_files/pkcs7_get_signers_info_set-leak-fuzz_pkcs7-4541044530479104.der differ diff --git a/tests/data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der b/tests/data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der new file mode 100644 index 000000000..ce4fb3bd4 Binary files /dev/null and b/tests/data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der differ diff --git a/tests/suites/test_suite_pkcs7.data b/tests/suites/test_suite_pkcs7.data index 4f81b6f28..f3cbb628f 100644 --- a/tests/suites/test_suite_pkcs7.data +++ b/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 diff --git a/tests/suites/test_suite_pkcs7.function b/tests/suites/test_suite_pkcs7.function index e3961407d..3d7dec686 100644 --- a/tests/suites/test_suite_pkcs7.function +++ b/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_ASSERT( md_alg == (mbedtls_md_type_t) do_hash_alg ); + TEST_EQUAL( res, 0 ); + 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_ASSERT( md_alg == MBEDTLS_MD_SHA256 ); + TEST_EQUAL( res, 0 ); + 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 );