From 47a732635bfeb5c6a5b4260ccc2b841a00c71512 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 27 Nov 2022 21:46:56 +0100 Subject: [PATCH] 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 783aaa2887..c1446def77 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,