Skip to content

Commit

Permalink
Merge pull request #6670 from gilles-peskine-arm/pkcs7-use-after-free…
Browse files Browse the repository at this point in the history 
…-20221127

PKCS7: Fix some memory management errors
  • Loading branch information
@mpg
mpg authored Nov 29, 2022
2 parents ffc330f + a13f5eb commit f9720cf
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 96 deletions.
110 changes: 42 additions & 68 deletions library/pkcs7.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,23 +103,20 @@ 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;
pkcs7->len = len;
pkcs7->p = *p;
*p += len;

out:
return( ret );
}

Expand Down Expand Up @@ -153,25 +150,22 @@ 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;

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 );
}

/**
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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 );
}

/**
Expand All @@ -255,16 +242,15 @@ 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;
signature->p = *p;

*p = *p + len;

out:
return( ret );
return( 0 );
}

/**
Expand Down Expand Up @@ -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;
}

/**
Expand All @@ -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;
Expand All @@ -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 );
}

Expand Down Expand Up @@ -470,46 +453,43 @@ 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;

/* 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;

Expand All @@ -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,
Expand All @@ -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 */
Expand Down Expand Up @@ -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 );
}

/*
Expand Down Expand Up @@ -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 ))
Expand All @@ -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,
Expand Down
Binary file added tests/data_files/pkcs7_get_signers_info_set-leak-fuzz_pkcs7-4541044530479104.der
View file
Binary file not shown.
Binary file added tests/data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der
View file
Binary file not shown.
8 changes: 8 additions & 0 deletions tests/suites/test_suite_pkcs7.data
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit f9720cf

Please sign in to comment.