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 <nick.child@ibm.com>
diff --git a/include/mbedtls/pkcs7.h b/include/mbedtls/pkcs7.h
index 835d2c1..b2cdabc 100644
--- a/include/mbedtls/pkcs7.h
+++ b/include/mbedtls/pkcs7.h
@@ -179,8 +179,9 @@
* \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 5fd8f64..05d98c3 100644
--- a/library/pkcs7.c
+++ b/library/pkcs7.c
@@ -94,6 +94,7 @@
* [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 @@
*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 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 @@
goto out;
}
+ end_issuer_and_sn = *p + len;
/* Parsing IssuerAndSerialNumber */
signer->issuer_raw.p = *p;
@@ -333,6 +335,12 @@
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 @@
{
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 @@
}
/* 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 @@
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 @@
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 @@
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 c916e42..22dd8a4 100644
--- a/tests/suites/test_suite_pkcs7.data
+++ b/tests/suites/test_suite_pkcs7.data
@@ -64,11 +64,11 @@
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