From 290f01b3f54a16045be201699becda8f500eebd5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 27 Nov 2022 21:28:31 +0100 Subject: [PATCH 1/7] Fix dangling freed pointer on error in pkcs7_get_signers_info_set This fixes a use-after-free in PKCS#7 parsing when the signer data is malformed. Credit to OSS-Fuzz (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53798). Signed-off-by: Gilles Peskine --- library/pkcs7.c | 5 +++-- ...t-missing_free-fuzz_pkcs7-6213931373035520.der | Bin 0 -> 108 bytes tests/suites/test_suite_pkcs7.data | 3 +++ 3 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 tests/data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der diff --git a/library/pkcs7.c b/library/pkcs7.c index ca0170a6d..783aaa288 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -430,15 +430,16 @@ static int pkcs7_get_signers_info_set( unsigned char **p, unsigned char *end, goto out; cleanup: - signer = signers_set->next; pkcs7_free_signer_info( signers_set ); - while( signer ) + 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 ); 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 0000000000000000000000000000000000000000..ce4fb3bd49fdaf0ccd10069af549eb55ec9554fe GIT binary patch literal 108 zcmXrWVq#=8FQ)N1o+`_9YA&S>avAPZDrz-_=`$Y#L8#=y+L V!~mq36ch}Y*cezC2uLd+0|0Qt3R(aF literal 0 HcmV?d00001 diff --git a/tests/suites/test_suite_pkcs7.data b/tests/suites/test_suite_pkcs7.data index 4f81b6f28..5ecfb9111 100644 --- a/tests/suites/test_suite_pkcs7.data +++ b/tests/suites/test_suite_pkcs7.data @@ -62,6 +62,9 @@ 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) +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 From 2336555444a7fe4e0efc20f8017af914d7b5869c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 27 Nov 2022 21:30:58 +0100 Subject: [PATCH 2/7] Improve test failure reporting Signed-off-by: Gilles Peskine --- tests/suites/test_suite_pkcs7.function | 48 +++++++++++++------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/tests/suites/test_suite_pkcs7.function b/tests/suites/test_suite_pkcs7.function index e3961407d..f938f4237 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 ); @@ -60,16 +60,16 @@ void pkcs7_verify( char *pkcs7_file, char *crt, char *filetobesigned, int do_has mbedtls_x509_crt_init( &x509 ); 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 ); @@ -134,21 +134,21 @@ void pkcs7_verify_multiple_signers( char *pkcs7_file, char *crt1, char *crt2, ch mbedtls_x509_crt_init( &x509_2 ); 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 ); From 391005cb3b2b195636570108dba30cf71894a7d6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 27 Nov 2022 21:32:37 +0100 Subject: [PATCH 3/7] Fix structures initialized too late in tests Signed-off-by: Gilles Peskine --- tests/suites/test_suite_pkcs7.function | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/suites/test_suite_pkcs7.function b/tests/suites/test_suite_pkcs7.function index f938f4237..3d7dec686 100644 --- a/tests/suites/test_suite_pkcs7.function +++ b/tests/suites/test_suite_pkcs7.function @@ -54,11 +54,11 @@ 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_EQUAL( res, 0 ); @@ -127,12 +127,12 @@ 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_EQUAL( res, 0 ); From 47a732635bfeb5c6a5b4260ccc2b841a00c71512 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 27 Nov 2022 21:46:56 +0100 Subject: [PATCH 4/7] Simplify control flow in PKCS7 functions Remove useless goto in several functions. Signed-off-by: Gilles Peskine --- library/pkcs7.c | 106 ++++++++++++++++++------------------------------ 1 file changed, 39 insertions(+), 67 deletions(-) diff --git a/library/pkcs7.c b/library/pkcs7.c index 783aaa288..c1446def7 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 ); } /** @@ -382,34 +368,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; + return( ret ); 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,12 +410,11 @@ static int pkcs7_get_signers_info_set( unsigned char **p, unsigned char *end, count++; } - ret = count; - goto out; + return( count ); cleanup: pkcs7_free_signer_info( signers_set ); - signer = signers_set->next; + mbedtls_pkcs7_signer_info *signer = signers_set->next; while( signer != NULL ) { prev = signer; @@ -440,8 +423,6 @@ cleanup: mbedtls_free( prev ); } signers_set->next = NULL; - -out: return( ret ); } @@ -471,8 +452,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; @@ -480,37 +460,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; @@ -525,18 +503,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, @@ -548,10 +523,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 */ @@ -631,15 +605,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 ); } /* @@ -673,9 +645,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 )) @@ -698,12 +670,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, From e7f8c616d0b9388fd20ffd6c9730ea8188f27716 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 27 Nov 2022 21:51:19 +0100 Subject: [PATCH 5/7] Fix dangling freed pointer in pkcs7_free_signer_info This may have been a use-after-free, but I haven't worked out whether it was a problem or not. Even if it turns out to have been ok, keeping invalid pointers around is fragile. Signed-off-by: Gilles Peskine --- library/pkcs7.c | 1 + 1 file changed, 1 insertion(+) diff --git a/library/pkcs7.c b/library/pkcs7.c index c1446def7..fc6dd33f3 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -353,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; } /** From 4f01121f6e598c51e42a69f3fd9a54846013117a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 27 Nov 2022 22:02:10 +0100 Subject: [PATCH 6/7] Fix memory leak on error in pkcs7_get_signers_info_set mbedtls_x509_name allocates memory, which must be freed if there is a subsequent error. Credit to OSS-Fuzz (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53811). Signed-off-by: Gilles Peskine --- library/pkcs7.c | 2 +- ..._info_set-leak-fuzz_pkcs7-4541044530479104.der | Bin 0 -> 108 bytes tests/suites/test_suite_pkcs7.data | 3 +++ 3 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 tests/data_files/pkcs7_get_signers_info_set-leak-fuzz_pkcs7-4541044530479104.der diff --git a/library/pkcs7.c b/library/pkcs7.c index fc6dd33f3..e4238b6a3 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -387,7 +387,7 @@ static int pkcs7_get_signers_info_set( unsigned char **p, unsigned char *end, ret = pkcs7_get_signer_info( p, end_set, signers_set ); if( ret != 0 ) - return( ret ); + goto cleanup; count++; mbedtls_pkcs7_signer_info *prev = signers_set; 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 0000000000000000000000000000000000000000..51aef0d0929043a6c080846758c96bf08a945216 GIT binary patch literal 108 zcmXrWVq#=8FQ)N1o+`_9YA&S+?7APZDrz-_=`$Y#L8#=yhC l!~mq36ch}Y*cezCVA3LnLJ(;XDFadhBo)BmKZH_H004ib3yc5& literal 0 HcmV?d00001 diff --git a/tests/suites/test_suite_pkcs7.data b/tests/suites/test_suite_pkcs7.data index 5ecfb9111..c329a771e 100644 --- a/tests/suites/test_suite_pkcs7.data +++ b/tests/suites/test_suite_pkcs7.data @@ -65,6 +65,9 @@ pkcs7_parse:"data_files/pkcs7_signerInfo_serial_invalid_size.der":MBEDTLS_ERR_PK pkcs7_get_signers_info_set error handling (6213931373035520) 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) +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 From a13f5eb7b840cc0d0472eebf5e7c224c8bbf8ec2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 28 Nov 2022 21:30:04 +0100 Subject: [PATCH 7/7] Add missing dependency for the fuzzer-constructed test data Signed-off-by: Gilles Peskine --- tests/suites/test_suite_pkcs7.data | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/suites/test_suite_pkcs7.data b/tests/suites/test_suite_pkcs7.data index c329a771e..f3cbb628f 100644 --- a/tests/suites/test_suite_pkcs7.data +++ b/tests/suites/test_suite_pkcs7.data @@ -63,9 +63,11 @@ 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