From ec81709516903a8590321eacdef0f6bf074e68f2 Mon Sep 17 00:00:00 2001 From: Nick Child Date: Thu, 15 Dec 2022 15:54:03 -0600 Subject: [PATCH] pkcs7: Ensure all data in asn1 structure is accounted for Several PKCS7 invalid ASN1 Tests were failing due to extra data bytes or incorrect content lengths going unnoticed. Make the parser aware of possible malformed ASN1 data. Signed-off-by: Nick Child --- include/mbedtls/pkcs7.h | 5 ++-- library/pkcs7.c | 40 ++++++++++++++++++++++++------ tests/suites/test_suite_pkcs7.data | 4 +-- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/include/mbedtls/pkcs7.h b/include/mbedtls/pkcs7.h index 835d2c1a5..b2cdabcfd 100644 --- a/include/mbedtls/pkcs7.h +++ b/include/mbedtls/pkcs7.h @@ -179,8 +179,9 @@ void mbedtls_pkcs7_init(mbedtls_pkcs7 *pkcs7); * \brief Parse a single DER formatted pkcs7 content. * * \param pkcs7 The pkcs7 structure to be filled by parser for the output. - * \param buf The buffer holding the DER encoded pkcs7. - * \param buflen The size in bytes of \p buf. + * \param buf The buffer holding only the DER encoded pkcs7. + * \param buflen The size in bytes of \p buf. The size must be exactly the + * length of the DER encoded pkcs7. * * \note This function makes an internal copy of the PKCS7 buffer * \p buf. In particular, \p buf may be destroyed or reused diff --git a/library/pkcs7.c b/library/pkcs7.c index 5fd8f6444..05d98c353 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -94,6 +94,7 @@ static int pkcs7_get_version(unsigned char **p, unsigned char *end, int *ver) * [0] EXPLICIT ANY DEFINED BY contentType OPTIONAL } **/ static int pkcs7_get_content_info_type(unsigned char **p, unsigned char *end, + unsigned char **seq_end, mbedtls_pkcs7_buf *pkcs7) { size_t len = 0; @@ -106,8 +107,8 @@ static int pkcs7_get_content_info_type(unsigned char **p, unsigned char *end, *p = start; return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, ret); } - - ret = mbedtls_asn1_get_tag(p, end, &len, MBEDTLS_ASN1_OID); + *seq_end = *p + len; + ret = mbedtls_asn1_get_tag(p, *seq_end, &len, MBEDTLS_ASN1_OID); if (ret != 0) { *p = start; return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, ret); @@ -289,7 +290,7 @@ static void pkcs7_free_signer_info(mbedtls_pkcs7_signer_info *signer) static int pkcs7_get_signer_info(unsigned char **p, unsigned char *end, mbedtls_pkcs7_signer_info *signer) { - unsigned char *end_signer; + unsigned char *end_signer, *end_issuer_and_sn; int asn1_ret = 0, ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; size_t len = 0; @@ -312,6 +313,7 @@ static int pkcs7_get_signer_info(unsigned char **p, unsigned char *end, goto out; } + end_issuer_and_sn = *p + len; /* Parsing IssuerAndSerialNumber */ signer->issuer_raw.p = *p; @@ -333,6 +335,12 @@ static int pkcs7_get_signer_info(unsigned char **p, unsigned char *end, goto out; } + /* ensure no extra or missing bytes */ + if (*p != end_issuer_and_sn) { + ret = MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO; + goto out; + } + ret = pkcs7_get_digest_algorithm(p, end_signer, &signer->alg_identifier); if (ret != 0) { goto out; @@ -449,7 +457,7 @@ static int pkcs7_get_signed_data(unsigned char *buf, size_t buflen, { unsigned char *p = buf; unsigned char *end = buf + buflen; - unsigned char *end_set; + unsigned char *end_set, *end_content_info; size_t len = 0; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_md_type_t md_alg; @@ -481,11 +489,16 @@ static int pkcs7_get_signed_data(unsigned char *buf, size_t buflen, } /* Do not expect any content */ - ret = pkcs7_get_content_info_type(&p, end_set, &signed_data->content.oid); + ret = pkcs7_get_content_info_type(&p, end_set, &end_content_info, + &signed_data->content.oid); if (ret != 0) { return ret; } + if (end_content_info != p) { + return MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO; + } + if (MBEDTLS_OID_CMP(MBEDTLS_OID_PKCS7_DATA, &signed_data->content.oid)) { return MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO; } @@ -527,7 +540,7 @@ int mbedtls_pkcs7_parse_der(mbedtls_pkcs7 *pkcs7, const unsigned char *buf, const size_t buflen) { unsigned char *p; - unsigned char *end; + unsigned char *end, *end_content_info; size_t len = 0; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; int isoidset = 0; @@ -546,12 +559,19 @@ int mbedtls_pkcs7_parse_der(mbedtls_pkcs7 *pkcs7, const unsigned char *buf, pkcs7->raw.len = buflen; end = p + buflen; - ret = pkcs7_get_content_info_type(&p, end, &pkcs7->content_type_oid); + ret = pkcs7_get_content_info_type(&p, end, &end_content_info, + &pkcs7->content_type_oid); if (ret != 0) { len = buflen; goto try_data; } + /* Ensure PKCS7 data uses the exact number of bytes specified in buflen */ + if (end_content_info != end) { + ret = MBEDTLS_ERR_PKCS7_BAD_INPUT_DATA; + goto out; + } + if (!MBEDTLS_OID_CMP(MBEDTLS_OID_PKCS7_DATA, &pkcs7->content_type_oid) || !MBEDTLS_OID_CMP(MBEDTLS_OID_PKCS7_ENCRYPTED_DATA, &pkcs7->content_type_oid) || !MBEDTLS_OID_CMP(MBEDTLS_OID_PKCS7_ENVELOPED_DATA, &pkcs7->content_type_oid) @@ -574,6 +594,12 @@ int mbedtls_pkcs7_parse_der(mbedtls_pkcs7 *pkcs7, const unsigned char *buf, goto out; } + /* ensure no extra/missing data */ + if (p + len != end) { + ret = MBEDTLS_ERR_PKCS7_BAD_INPUT_DATA; + goto out; + } + try_data: ret = pkcs7_get_signed_data(p, len, &pkcs7->signed_data); if (ret != 0) { diff --git a/tests/suites/test_suite_pkcs7.data b/tests/suites/test_suite_pkcs7.data index c916e422b..22dd8a446 100644 --- a/tests/suites/test_suite_pkcs7.data +++ b/tests/suites/test_suite_pkcs7.data @@ -64,11 +64,11 @@ pkcs7_parse:"data_files/pkcs7_signerInfo_serial_invalid_size.der":MBEDTLS_ERR_PK 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_parse:"data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der":MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO pkcs7_get_signers_info_set error handling (4541044530479104) depends_on:MBEDTLS_RIPEMD160_C -pkcs7_parse:"data_files/pkcs7_get_signers_info_set-leak-fuzz_pkcs7-4541044530479104.der":MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO +pkcs7_parse:"data_files/pkcs7_get_signers_info_set-leak-fuzz_pkcs7-4541044530479104.der":MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO PKCS7 Only Signed Data Parse Pass #15 depends_on:MBEDTLS_SHA256_C:MBEDTLS_RSA_C