pkcs7: Respond to feeback on parsing logic
After recieving review on the pkcs7 parsing functions, attempt
to use better API's, increase consisitency and use better
documentation. The changes are in response to the following
comments:
- use mbedtls_x509_crt_parse_der instead of mbedtls_x509_crt_parse [1]
- make lack of support for authenticatedAttributes more clear [2]
- increment pointer in pkcs7_get_content_info_type rather than after [3]
- rename `start` to `p` for consistency in mbedtls_pkcs7_parse_der [4]
[1] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r992509630
[2] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r992562450
[3] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r992741877
[4] https://github.com/Mbed-TLS/mbedtls/pull/3431#discussion_r992754103
Signed-off-by: Nick Child <nick.child@ibm.com>
diff --git a/include/mbedtls/pkcs7.h b/include/mbedtls/pkcs7.h
index 9486c71..2a557bf 100644
--- a/include/mbedtls/pkcs7.h
+++ b/include/mbedtls/pkcs7.h
@@ -38,6 +38,9 @@
* - The RFC specifies the Signed Data type can contain
* certificate-revocation lists (crls). This implementation has no support
* for crls so it is assumed to be an empty list.
+ * - The RFC allows for SignerInfo structure to optionally contain
+ * unauthenticatedAttributes and authenticatedAttributes. In Mbed TLS it is
+ * assumed these fields are empty.
*/
#ifndef MBEDTLS_PKCS7_H
diff --git a/library/pkcs7.c b/library/pkcs7.c
index 56b6bb6..ab7bebd 100644
--- a/library/pkcs7.c
+++ b/library/pkcs7.c
@@ -126,6 +126,7 @@
pkcs7->tag = MBEDTLS_ASN1_OID;
pkcs7->len = len;
pkcs7->p = *p;
+ *p += len;
out:
return( ret );
@@ -197,8 +198,7 @@
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
size_t len1 = 0;
size_t len2 = 0;
- unsigned char *end_set, *end_cert;
- unsigned char *start = *p;
+ unsigned char *end_set, *end_cert, *start;
if( ( ret = mbedtls_asn1_get_tag( p, end, &len1, MBEDTLS_ASN1_CONSTRUCTED
| MBEDTLS_ASN1_CONTEXT_SPECIFIC ) ) != 0 )
@@ -235,7 +235,7 @@
}
*p = start;
- if( ( ret = mbedtls_x509_crt_parse( certs, *p, len1 ) ) < 0 )
+ if( ( ret = mbedtls_x509_crt_parse_der( certs, *p, len1 ) ) < 0 )
{
ret = MBEDTLS_ERR_PKCS7_INVALID_CERT;
goto out;
@@ -289,6 +289,8 @@
* [1] IMPLICIT Attributes OPTIONAL,
* Returns 0 if the signerInfo is valid.
* Return negative error code for failure.
+ * Structure must not contain vales for authenticatedAttributes
+ * and unauthenticatedAttributes.
**/
static int pkcs7_get_signer_info( unsigned char **p, unsigned char *end,
mbedtls_pkcs7_signer_info *signer )
@@ -335,6 +337,8 @@
if( ret != 0 )
goto out;
+ /* Asssume authenticatedAttributes is nonexistent */
+
ret = pkcs7_get_digest_algorithm( p, end_signer, &signer->sig_alg_identifier );
if( ret != 0 )
goto out;
@@ -510,8 +514,6 @@
goto out;
}
- p = p + signed_data->content.oid.len;
-
/* 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 );
@@ -548,7 +550,7 @@
int mbedtls_pkcs7_parse_der( mbedtls_pkcs7 *pkcs7, const unsigned char *buf,
const size_t buflen )
{
- unsigned char *start;
+ unsigned char *p;
unsigned char *end;
size_t len = 0;
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
@@ -561,17 +563,17 @@
}
/* make an internal copy of the buffer for parsing */
- pkcs7->raw.p = start = mbedtls_calloc( 1, buflen );
+ pkcs7->raw.p = p = mbedtls_calloc( 1, buflen );
if( pkcs7->raw.p == NULL )
{
ret = MBEDTLS_ERR_PKCS7_ALLOC_FAILED;
goto out;
}
- memcpy( start, buf, buflen );
+ memcpy( p, buf, buflen );
pkcs7->raw.len = buflen;
- end = start + buflen;
+ end = p + buflen;
- ret = pkcs7_get_content_info_type( &start, end, &pkcs7->content_type_oid );
+ ret = pkcs7_get_content_info_type( &p, end, &pkcs7->content_type_oid );
if( ret != 0 )
{
len = buflen;
@@ -596,14 +598,13 @@
}
isoidset = 1;
- start = start + pkcs7->content_type_oid.len;
- ret = pkcs7_get_next_content_len( &start, end, &len );
+ ret = pkcs7_get_next_content_len( &p, end, &len );
if( ret != 0 )
goto out;
try_data:
- ret = pkcs7_get_signed_data( start, len, &pkcs7->signed_data );
+ ret = pkcs7_get_signed_data( p, len, &pkcs7->signed_data );
if ( ret != 0 )
goto out;