From 512818b1d2173e0ea906316075a6e01fd1654fac Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sun, 27 Nov 2022 22:48:55 -0500 Subject: [PATCH 01/13] pkcs7: check that content lengths fill whole buffer Otherwise invalid data could be accepted. Signed-off-by: Demi Marie Obenour Signed-off-by: Dave Rodgman --- library/pkcs7.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/pkcs7.c b/library/pkcs7.c index 4fdbe36288..ec5d569aa8 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -58,6 +58,9 @@ static int pkcs7_get_next_content_len(unsigned char **p, unsigned char *end, | MBEDTLS_ASN1_CONTEXT_SPECIFIC); if (ret != 0) { ret = MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_FORMAT, ret); + } else if ((size_t) (end - *p) != *len) { + ret = MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_FORMAT, + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); } return ret; From aaf3c0028d33262c2707f84ba228d0080eae7396 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 28 Nov 2022 00:20:42 -0500 Subject: [PATCH 02/13] pkcs7: do not store content type OID Since only one content type (signed data) is supported, storing the content type just wastes memory. Signed-off-by: Demi Marie Obenour --- include/mbedtls/pkcs7.h | 1 - library/pkcs7.c | 55 +++++++++++++++++++++-------------------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/include/mbedtls/pkcs7.h b/include/mbedtls/pkcs7.h index 5ddd5a3d71..f354db629e 100644 --- a/include/mbedtls/pkcs7.h +++ b/include/mbedtls/pkcs7.h @@ -165,7 +165,6 @@ mbedtls_pkcs7_signed_data; */ typedef struct mbedtls_pkcs7 { mbedtls_pkcs7_buf MBEDTLS_PRIVATE(raw); - mbedtls_pkcs7_buf MBEDTLS_PRIVATE(content_type_oid); mbedtls_pkcs7_signed_data MBEDTLS_PRIVATE(signed_data); } mbedtls_pkcs7; diff --git a/library/pkcs7.c b/library/pkcs7.c index ec5d569aa8..398c0c8264 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -556,7 +556,6 @@ int mbedtls_pkcs7_parse_der(mbedtls_pkcs7 *pkcs7, const unsigned char *buf, unsigned char *end, *end_content_info; size_t len = 0; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - int isoidset = 0; if (pkcs7 == NULL) { return MBEDTLS_ERR_PKCS7_BAD_INPUT_DATA; @@ -572,34 +571,42 @@ 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, &end_content_info, - &pkcs7->content_type_oid); + ret = mbedtls_asn1_get_tag(&p, end, &len, MBEDTLS_ASN1_CONSTRUCTED + | MBEDTLS_ASN1_SEQUENCE); if (ret != 0) { + ret = MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_FORMAT, ret); + goto out; + } + + if ((size_t) (end - p) != len) { + ret = MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_FORMAT, + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); + goto out; + } + + if ((ret = mbedtls_asn1_get_tag(&p, end, &len, MBEDTLS_ASN1_OID)) != 0) { + if (ret != MBEDTLS_ERR_ASN1_UNEXPECTED_TAG) { + goto out; + } + p = pkcs7->raw.p; 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; + if (MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS7_SIGNED_DATA, p, len)) { + if (!MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS7_DATA, p, len) + || !MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS7_ENCRYPTED_DATA, p, len) + || !MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS7_ENVELOPED_DATA, p, len) + || !MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS7_SIGNED_AND_ENVELOPED_DATA, p, len) + || !MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS7_DIGESTED_DATA, p, len)) { + ret = MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE; + } else { + 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_ENVELOPED_DATA, &pkcs7->content_type_oid) - || !MBEDTLS_OID_CMP(MBEDTLS_OID_PKCS7_SIGNED_AND_ENVELOPED_DATA, &pkcs7->content_type_oid) - || !MBEDTLS_OID_CMP(MBEDTLS_OID_PKCS7_DIGESTED_DATA, &pkcs7->content_type_oid) - || !MBEDTLS_OID_CMP(MBEDTLS_OID_PKCS7_ENCRYPTED_DATA, &pkcs7->content_type_oid)) { - ret = MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE; - goto out; - } - - if (MBEDTLS_OID_CMP(MBEDTLS_OID_PKCS7_SIGNED_DATA, &pkcs7->content_type_oid)) { - ret = MBEDTLS_ERR_PKCS7_BAD_INPUT_DATA; - goto out; - } - - isoidset = 1; + p += len; ret = pkcs7_get_next_content_len(&p, end, &len); if (ret != 0) { @@ -618,12 +625,6 @@ try_data: goto out; } - if (!isoidset) { - pkcs7->content_type_oid.tag = MBEDTLS_ASN1_OID; - pkcs7->content_type_oid.len = MBEDTLS_OID_SIZE(MBEDTLS_OID_PKCS7_SIGNED_DATA); - pkcs7->content_type_oid.p = (unsigned char *) MBEDTLS_OID_PKCS7_SIGNED_DATA; - } - ret = MBEDTLS_PKCS7_SIGNED_DATA; out: From 4ec835579509200b77864cec72a5f7324bfa491d Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 28 Nov 2022 00:23:00 -0500 Subject: [PATCH 03/13] Check for junk after SignedData There must not be any. Signed-off-by: Demi Marie Obenour --- library/pkcs7.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/library/pkcs7.c b/library/pkcs7.c index 398c0c8264..5d470dc15d 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -457,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, *end_content_info; + unsigned char *end_content_info; size_t len = 0; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_md_type_t md_alg; @@ -468,16 +468,19 @@ static int pkcs7_get_signed_data(unsigned char *buf, size_t buflen, return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_FORMAT, ret); } - end_set = p + len; + if (p + len != end) { + return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_FORMAT, + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); + } /* Get version of signed data */ - ret = pkcs7_get_version(&p, end_set, &signed_data->version); + ret = pkcs7_get_version(&p, end, &signed_data->version); if (ret != 0) { return ret; } /* Get digest algorithm */ - ret = pkcs7_get_digest_algorithm_set(&p, end_set, + ret = pkcs7_get_digest_algorithm_set(&p, end, &signed_data->digest_alg_identifiers); if (ret != 0) { return ret; @@ -518,7 +521,7 @@ static int pkcs7_get_signed_data(unsigned char *buf, size_t buflen, /* 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); + ret = pkcs7_get_certificates(&p, end, &signed_data->certs); if (ret < 0) { return ret; } @@ -534,7 +537,7 @@ static int pkcs7_get_signed_data(unsigned char *buf, size_t buflen, signed_data->no_of_crls = 0; /* Get signers info */ - ret = pkcs7_get_signers_info_set(&p, end_set, &signed_data->signers); + ret = pkcs7_get_signers_info_set(&p, end, &signed_data->signers); if (ret < 0) { return ret; } @@ -553,7 +556,7 @@ int mbedtls_pkcs7_parse_der(mbedtls_pkcs7 *pkcs7, const unsigned char *buf, const size_t buflen) { unsigned char *p; - unsigned char *end, *end_content_info; + unsigned char *end; size_t len = 0; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; From 55d9df25ef63c8a6e5ad7d9bdfd47f38b7fa303c Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 28 Nov 2022 00:29:32 -0500 Subject: [PATCH 04/13] Simple cleanup No change in behavior. Signed-off-by: Demi Marie Obenour --- library/pkcs7.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/library/pkcs7.c b/library/pkcs7.c index 5d470dc15d..39d9f8f20b 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -216,12 +216,11 @@ static int pkcs7_get_certificates(unsigned char **p, unsigned char *end, return MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE; } - *p = start; - if ((ret = mbedtls_x509_crt_parse_der(certs, *p, len1)) < 0) { + if ((ret = mbedtls_x509_crt_parse_der(certs, start, len1)) < 0) { return MBEDTLS_ERR_PKCS7_INVALID_CERT; } - *p = *p + len1; + *p = end_cert; /* * Since in this version we strictly support single certificate, and reaching From e373a254c42b661f50794250634a90554c3b95c0 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Tue, 13 Dec 2022 23:50:03 -0500 Subject: [PATCH 05/13] pkcs7: do not store content type OIDs They will always be constant. Signed-off-by: Demi Marie Obenour --- include/mbedtls/pkcs7.h | 1 - library/pkcs7.c | 12 +++++------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/include/mbedtls/pkcs7.h b/include/mbedtls/pkcs7.h index f354db629e..fb24604d00 100644 --- a/include/mbedtls/pkcs7.h +++ b/include/mbedtls/pkcs7.h @@ -139,7 +139,6 @@ mbedtls_pkcs7_signer_info; * Structure holding attached data as part of PKCS7 signed data format */ typedef struct mbedtls_pkcs7_data { - mbedtls_pkcs7_buf MBEDTLS_PRIVATE(oid); mbedtls_pkcs7_buf MBEDTLS_PRIVATE(data); } mbedtls_pkcs7_data; diff --git a/library/pkcs7.c b/library/pkcs7.c index 39d9f8f20b..9ef76089a8 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -490,12 +490,14 @@ static int pkcs7_get_signed_data(unsigned char *buf, size_t buflen, return MBEDTLS_ERR_PKCS7_INVALID_ALG; } - /* Do not expect any content */ - ret = pkcs7_get_content_info_type(&p, end_set, &end_content_info, - &signed_data->content.oid); + mbedtls_pkcs7_buf content_type; + ret = pkcs7_get_content_info_type(&p, end, &end_content_info, &content_type); if (ret != 0) { return ret; } + if (MBEDTLS_OID_CMP(MBEDTLS_OID_PKCS7_DATA, &content_type)) { + return MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO; + } if (p != end_content_info) { /* Determine if valid content is present */ @@ -514,10 +516,6 @@ static int pkcs7_get_signed_data(unsigned char *buf, size_t buflen, return MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE; } - if (MBEDTLS_OID_CMP(MBEDTLS_OID_PKCS7_DATA, &signed_data->content.oid)) { - 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, &signed_data->certs); From 6cfc4692961ba808d801ab2ffbf6e076523ccac0 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 28 Nov 2022 00:46:00 -0500 Subject: [PATCH 06/13] pkcs7: reject signatures with internal data A CMS signature can have internal data, but mbedTLS does not support verifying such signatures. Reject them during parsing. Signed-off-by: Demi Marie Obenour Signed-off-by: Dave Rodgman --- include/mbedtls/pkcs7.h | 31 ++++++++++++------------------- library/pkcs7.c | 25 ++++++++++++++++--------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/include/mbedtls/pkcs7.h b/include/mbedtls/pkcs7.h index fb24604d00..126eac4228 100644 --- a/include/mbedtls/pkcs7.h +++ b/include/mbedtls/pkcs7.h @@ -135,21 +135,12 @@ typedef struct mbedtls_pkcs7_signer_info { } mbedtls_pkcs7_signer_info; -/** - * Structure holding attached data as part of PKCS7 signed data format - */ -typedef struct mbedtls_pkcs7_data { - mbedtls_pkcs7_buf MBEDTLS_PRIVATE(data); -} -mbedtls_pkcs7_data; - /** * Structure holding the signed data section */ typedef struct mbedtls_pkcs7_signed_data { int MBEDTLS_PRIVATE(version); mbedtls_pkcs7_buf MBEDTLS_PRIVATE(digest_alg_identifiers); - struct mbedtls_pkcs7_data MBEDTLS_PRIVATE(content); int MBEDTLS_PRIVATE(no_of_certs); mbedtls_x509_crt MBEDTLS_PRIVATE(certs); int MBEDTLS_PRIVATE(no_of_crls); @@ -176,7 +167,7 @@ mbedtls_pkcs7; void mbedtls_pkcs7_init(mbedtls_pkcs7 *pkcs7); /** - * \brief Parse a single DER formatted pkcs7 content. + * \brief Parse a single DER formatted pkcs7 detached signature. * * \param pkcs7 The pkcs7 structure to be filled by parser for the output. * \param buf The buffer holding only the DER encoded pkcs7. @@ -186,6 +177,7 @@ void mbedtls_pkcs7_init(mbedtls_pkcs7 *pkcs7); * \note This function makes an internal copy of the PKCS7 buffer * \p buf. In particular, \p buf may be destroyed or reused * after this call returns. + * \note Signatures with internal data are not supported. * * \return The \c mbedtls_pkcs7_type of \p buf, if successful. * \return A negative error code on failure. @@ -205,7 +197,8 @@ int mbedtls_pkcs7_parse_der(mbedtls_pkcs7 *pkcs7, const unsigned char *buf, * matches. * * This function does not use the certificates held within the - * PKCS7 structure itself. + * PKCS7 structure itself, and does not check that the + * certificate is signed by a trusted certification authority. * * \param pkcs7 PKCS7 structure containing signature. * \param cert Certificate containing key to verify signature. @@ -226,15 +219,15 @@ int mbedtls_pkcs7_signed_data_verify(mbedtls_pkcs7 *pkcs7, * \brief Verification of PKCS7 signature against a caller-supplied * certificate. * - * For each signer in the PKCS structure, this function computes - * a signature over the supplied hash, using the supplied - * certificate and the same digest algorithm as specified by the - * signer. It then compares this signature against the - * signer's signature; verification succeeds if any comparison - * matches. + * For each signer in the PKCS structure, this function + * validates a signature over the supplied hash, using the + * supplied certificate and the same digest algorithm as + * specified by the signer. Verification succeeds if any + * signature is good. * * This function does not use the certificates held within the - * PKCS7 structure itself. + * PKCS7 structure itself, and does not check that the + * certificate is signed by a trusted certification authority. * * \param pkcs7 PKCS7 structure containing signature. * \param cert Certificate containing key to verify signature. @@ -242,7 +235,7 @@ int mbedtls_pkcs7_signed_data_verify(mbedtls_pkcs7 *pkcs7, * \param hashlen Length of the hash. * * \note This function is different from mbedtls_pkcs7_signed_data_verify() - * in a way that it directly receives the hash of the data. + * in that it is directly passed the hash of the data. * * \return 0 if the signature verifies, or a negative error code on failure. */ diff --git a/library/pkcs7.c b/library/pkcs7.c index 9ef76089a8..fbe959ef2e 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -57,9 +57,9 @@ static int pkcs7_get_next_content_len(unsigned char **p, unsigned char *end, ret = mbedtls_asn1_get_tag(p, end, len, MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_CONTEXT_SPECIFIC); if (ret != 0) { - ret = MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_FORMAT, ret); + ret = MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, ret); } else if ((size_t) (end - *p) != *len) { - ret = MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_FORMAT, + ret = MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); } @@ -187,13 +187,13 @@ static int pkcs7_get_certificates(unsigned char **p, unsigned char *end, size_t len2 = 0; unsigned char *end_set, *end_cert, *start; - if ((ret = mbedtls_asn1_get_tag(p, end, &len1, MBEDTLS_ASN1_CONSTRUCTED - | MBEDTLS_ASN1_CONTEXT_SPECIFIC)) != 0) { - if (ret == MBEDTLS_ERR_ASN1_UNEXPECTED_TAG) { - return 0; - } else { - return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_FORMAT, ret); - } + ret = mbedtls_asn1_get_tag(p, end, &len1, MBEDTLS_ASN1_CONSTRUCTED + | MBEDTLS_ASN1_CONTEXT_SPECIFIC); + if (ret == MBEDTLS_ERR_ASN1_UNEXPECTED_TAG) { + return 0; + } + if (ret != 0) { + return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_FORMAT, ret); } start = *p; end_set = *p + len1; @@ -716,11 +716,15 @@ static int mbedtls_pkcs7_data_or_hash_verify(mbedtls_pkcs7 *pkcs7, return ret; } + int mbedtls_pkcs7_signed_data_verify(mbedtls_pkcs7 *pkcs7, const mbedtls_x509_crt *cert, const unsigned char *data, size_t datalen) { + if (data == NULL) { + return MBEDTLS_ERR_PKCS7_BAD_INPUT_DATA; + } return mbedtls_pkcs7_data_or_hash_verify(pkcs7, cert, data, datalen, 0); } @@ -729,6 +733,9 @@ int mbedtls_pkcs7_signed_hash_verify(mbedtls_pkcs7 *pkcs7, const unsigned char *hash, size_t hashlen) { + if (hash == NULL) { + return MBEDTLS_ERR_PKCS7_BAD_INPUT_DATA; + } return mbedtls_pkcs7_data_or_hash_verify(pkcs7, cert, hash, hashlen, 1); } From 35598adb781c71b93b400bff9fc7f5b7c1e957c7 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 30 Nov 2022 02:06:07 -0500 Subject: [PATCH 07/13] pkcs7: Check that hash algs are in digestAlgorithms Since only a single hash algorithm is currenlty supported, this avoids having to perform hashing more than once. Signed-off-by: Demi Marie Obenour --- library/pkcs7.c | 96 +++++++++++++++++++++++++++---------------------- 1 file changed, 53 insertions(+), 43 deletions(-) diff --git a/library/pkcs7.c b/library/pkcs7.c index fbe959ef2e..36e1960e99 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -287,7 +287,8 @@ static void pkcs7_free_signer_info(mbedtls_pkcs7_signer_info *signer) * and unauthenticatedAttributes. **/ static int pkcs7_get_signer_info(unsigned char **p, unsigned char *end, - mbedtls_pkcs7_signer_info *signer) + mbedtls_pkcs7_signer_info *signer, + mbedtls_x509_buf *alg) { unsigned char *end_signer, *end_issuer_and_sn; int asn1_ret = 0, ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; @@ -345,8 +346,15 @@ static int pkcs7_get_signer_info(unsigned char **p, unsigned char *end, goto out; } - /* Assume authenticatedAttributes is nonexistent */ + /* Check that the digest algorithm used matches the one provided earlier */ + if (signer->alg_identifier.tag != alg->tag || + signer->alg_identifier.len != alg->len || + memcmp(signer->alg_identifier.p, alg->p, alg->len) != 0) { + ret = MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO; + goto out; + } + /* Asssume authenticatedAttributes is nonexistent */ ret = pkcs7_get_digest_algorithm(p, end_signer, &signer->sig_alg_identifier); if (ret != 0) { goto out; @@ -379,7 +387,8 @@ out: * Return negative error code for failure. **/ static int pkcs7_get_signers_info_set(unsigned char **p, unsigned char *end, - mbedtls_pkcs7_signer_info *signers_set) + mbedtls_pkcs7_signer_info *signers_set, + mbedtls_x509_buf *digest_alg) { unsigned char *end_set; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; @@ -399,7 +408,7 @@ static int pkcs7_get_signers_info_set(unsigned char **p, unsigned char *end, end_set = *p + len; - ret = pkcs7_get_signer_info(p, end_set, signers_set); + ret = pkcs7_get_signer_info(p, end_set, signers_set, digest_alg); if (ret != 0) { return ret; } @@ -414,7 +423,7 @@ static int pkcs7_get_signers_info_set(unsigned char **p, unsigned char *end, goto cleanup; } - ret = pkcs7_get_signer_info(p, end_set, signer); + ret = pkcs7_get_signer_info(p, end_set, signer, digest_alg); if (ret != 0) { mbedtls_free(signer); goto cleanup; @@ -534,7 +543,10 @@ static int pkcs7_get_signed_data(unsigned char *buf, size_t buflen, signed_data->no_of_crls = 0; /* Get signers info */ - ret = pkcs7_get_signers_info_set(&p, end, &signed_data->signers); + ret = pkcs7_get_signers_info_set(&p, + end, + &signed_data->signers, + &signed_data->digest_alg_identifiers); if (ret < 0) { return ret; } @@ -657,6 +669,39 @@ static int mbedtls_pkcs7_data_or_hash_verify(mbedtls_pkcs7 *pkcs7, return MBEDTLS_ERR_PKCS7_CERT_DATE_INVALID; } + ret = mbedtls_oid_get_md_alg(&pkcs7->signed_data.digest_alg_identifiers, &md_alg); + if (ret != 0) { + return ret; + } + + md_info = mbedtls_md_info_from_type(md_alg); + if (md_info == NULL) { + return MBEDTLS_ERR_PKCS7_VERIFY_FAIL; + } + + hash = mbedtls_calloc(mbedtls_md_get_size(md_info), 1); + if (hash == NULL) { + 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)) { + ret = MBEDTLS_ERR_PKCS7_VERIFY_FAIL; + } else { + memcpy(hash, data, datalen); + } + } else { + ret = mbedtls_md(md_info, data, datalen, hash); + } + if (ret != 0) { + mbedtls_free(hash); + return MBEDTLS_ERR_PKCS7_VERIFY_FAIL; + } + + /* assume failure */ + ret = MBEDTLS_ERR_PKCS7_VERIFY_FAIL; + /* * Potential TODOs * Currently we iterate over all signers and return success if any of them @@ -666,54 +711,19 @@ static int mbedtls_pkcs7_data_or_hash_verify(mbedtls_pkcs7 *pkcs7, * identification and SignerIdentifier fields first. That would also allow * us to distinguish between 'no signature for key' and 'signature for key * failed to validate'. - * - * We could also cache hashes by md, so if there are several sigs all using - * the same algo we don't recalculate the hash each time. */ for (signer = &pkcs7->signed_data.signers; signer; signer = signer->next) { - ret = mbedtls_oid_get_md_alg(&signer->alg_identifier, &md_alg); - if (ret != 0) { - ret = MBEDTLS_ERR_PKCS7_VERIFY_FAIL; - continue; - } - - md_info = mbedtls_md_info_from_type(md_alg); - if (md_info == NULL) { - ret = MBEDTLS_ERR_PKCS7_VERIFY_FAIL; - continue; - } - - hash = mbedtls_calloc(mbedtls_md_get_size(md_info), 1); - if (hash == NULL) { - 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)) { - ret = MBEDTLS_ERR_PKCS7_VERIFY_FAIL; - } else { - memcpy(hash, data, datalen); - } - } else { - ret = mbedtls_md(md_info, data, datalen, hash); - } - if (ret != 0) { - ret = MBEDTLS_ERR_PKCS7_VERIFY_FAIL; - mbedtls_free(hash); - continue; - } - ret = mbedtls_pk_verify(&pk_cxt, md_alg, hash, 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; } } + mbedtls_free(hash); + /* END must free hash before jumping out */ return ret; } From f691268ee9dc298325ea83e133e8a76cf4c1ddc9 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 9 Feb 2023 17:55:41 +0000 Subject: [PATCH 08/13] Add missing initialisers Signed-off-by: Dave Rodgman --- library/pkcs7.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/pkcs7.c b/library/pkcs7.c index 36e1960e99..60d1175289 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -465,7 +465,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_content_info; + unsigned char *end_content_info = NULL; size_t len = 0; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_md_type_t md_alg; @@ -500,6 +500,7 @@ static int pkcs7_get_signed_data(unsigned char *buf, size_t buflen, } mbedtls_pkcs7_buf content_type; + memset(&content_type, 0, sizeof(content_type)); ret = pkcs7_get_content_info_type(&p, end, &end_content_info, &content_type); if (ret != 0) { return ret; From c5874db5b079d491316b298724eff0702d10398c Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 16 Feb 2023 16:14:46 +0000 Subject: [PATCH 09/13] Add test-case for signature over zero-length data Signed-off-by: Dave Rodgman --- tests/data_files/Makefile | 9 +++++++++ tests/data_files/pkcs7_zerolendata.bin | 0 tests/data_files/pkcs7_zerolendata_detached.der | Bin 0 -> 435 bytes tests/suites/test_suite_pkcs7.data | 4 ++++ tests/suites/test_suite_pkcs7.function | 3 ++- 5 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 tests/data_files/pkcs7_zerolendata.bin create mode 100644 tests/data_files/pkcs7_zerolendata_detached.der diff --git a/tests/data_files/Makefile b/tests/data_files/Makefile index 070f538fe8..6680bf944a 100644 --- a/tests/data_files/Makefile +++ b/tests/data_files/Makefile @@ -1205,6 +1205,10 @@ $(pkcs7_test_file): echo -e "Hello\xd" > $@ all_final += $(pkcs7_test_file) +pkcs7_zerolendata.bin: + printf '' > $@ +all_final += pkcs7_zerolendata.bin + pkcs7_data_1.bin: echo -e "2\xd" > $@ all_final += pkcs7_data_1.bin @@ -1238,6 +1242,11 @@ pkcs7-rsa-sha256-2.der: $(pkcs7_test_cert_2) $(OPENSSL) x509 -in pkcs7-rsa-sha256-2.crt -out $@ -outform DER all_final += pkcs7-rsa-sha256-2.der +# pkcs7 signature file over zero-len data +pkcs7_zerolendata_detached.der: pkcs7_zerolendata.bin pkcs7-rsa-sha256-1.key pkcs7-rsa-sha256-1.crt + $(OPENSSL) smime -sign -md sha256 -nocerts -noattr -in pkcs7_zerolendata.bin -inkey pkcs7-rsa-sha256-1.key -outform DER -binary -signer pkcs7-rsa-sha256-1.crt -out pkcs7_zerolendata_detached.der +all_final += pkcs7_zerolendata_detached.der + # pkcs7 signature file with CERT pkcs7_data_cert_signed_sha256.der: $(pkcs7_test_file) $(pkcs7_test_cert_1) $(OPENSSL) smime -sign -binary -in pkcs7_data.bin -out $@ -md sha256 -signer pkcs7-rsa-sha256-1.pem -noattr -outform DER -out $@ diff --git a/tests/data_files/pkcs7_zerolendata.bin b/tests/data_files/pkcs7_zerolendata.bin new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/data_files/pkcs7_zerolendata_detached.der b/tests/data_files/pkcs7_zerolendata_detached.der new file mode 100644 index 0000000000000000000000000000000000000000..2a389ab484991c53322dc87f998c23666c7f40d8 GIT binary patch literal 435 zcmXqLVqDM0snzDu_MMlJooPW6;{t;w#yL!kjE4LMylk8aZ61uN%q&cdtPBR+2!)J> zO^oG0g~dRH20jKRhTI06Y|No7Y{E=_K8Ab-JRlAi4{Lz8bFjIgsDUtu&&B(8VOS!kJ#%#A}``N(1;c{&~>lHDkH?uEo%Mn-I)RcHtyiZfMa^p+ZsUNpi zPgR!4Nqe^XP|J(iZ9iYV4VC)5eD}1>LYA_Ut0%UoTPg6KocZ~>nDc7CFrFCCvQ4f5 z&!6=E=2F~OeeSW~iYJ$z8m>7r$$s(fnJ=H)XRc*m#@BnKV2R)M=?|hk7C)IVU>KY#=_d1=v@T Date: Thu, 16 Feb 2023 16:23:09 +0000 Subject: [PATCH 10/13] Adjust position of empty line Signed-off-by: Dave Rodgman --- library/pkcs7.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/pkcs7.c b/library/pkcs7.c index 60d1175289..ba43f49715 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -684,8 +684,8 @@ static int mbedtls_pkcs7_data_or_hash_verify(mbedtls_pkcs7 *pkcs7, if (hash == NULL) { return MBEDTLS_ERR_PKCS7_ALLOC_FAILED; } - /* BEGIN must free hash before jumping out */ + /* BEGIN must free hash before jumping out */ if (is_data_hash) { if (datalen != mbedtls_md_get_size(md_info)) { ret = MBEDTLS_ERR_PKCS7_VERIFY_FAIL; From d652dce9eacad8eba19b0f54abbcfd6cb8a5a64e Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 16 Feb 2023 16:39:34 +0000 Subject: [PATCH 11/13] Add failing test case (invalid signature) for zero-length data Signed-off-by: Dave Rodgman --- tests/suites/test_suite_pkcs7.data | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/suites/test_suite_pkcs7.data b/tests/suites/test_suite_pkcs7.data index 70233ded7a..9948537aa2 100644 --- a/tests/suites/test_suite_pkcs7.data +++ b/tests/suites/test_suite_pkcs7.data @@ -42,6 +42,10 @@ PKCS7 Signed Data Verification Pass zero-len data depends_on:MBEDTLS_SHA1_C:MBEDTLS_SHA256_C pkcs7_verify:"data_files/pkcs7_zerolendata_detached.der":"data_files/pkcs7-rsa-sha256-1.der":"data_files/pkcs7_zerolendata.bin":0:0 +PKCS7 Signed Data Verification Fail zero-len data +depends_on:MBEDTLS_SHA1_C:MBEDTLS_SHA256_C +pkcs7_verify:"data_files/pkcs7_zerolendata_detached.der":"data_files/pkcs7-rsa-sha256-2.der":"data_files/pkcs7_zerolendata.bin":0:MBEDTLS_ERR_RSA_VERIFY_FAILED + PKCS7 Signed Data Verification Pass SHA256 #9 depends_on:MBEDTLS_SHA256_C pkcs7_verify:"data_files/pkcs7_data_cert_signed_sha256.der":"data_files/pkcs7-rsa-sha256-1.der":"data_files/pkcs7_data.bin":0:0 From a1b2bfff467d114365d5b9cd17a798de9201651e Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Mon, 20 Feb 2023 14:45:09 +0000 Subject: [PATCH 12/13] Add clarifying comments Signed-off-by: Dave Rodgman --- library/pkcs7.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/pkcs7.c b/library/pkcs7.c index ba43f49715..010d7066e8 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -607,13 +607,16 @@ int mbedtls_pkcs7_parse_der(mbedtls_pkcs7 *pkcs7, const unsigned char *buf, } if (MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS7_SIGNED_DATA, p, len)) { + /* OID is not MBEDTLS_OID_PKCS7_SIGNED_DATA, which is the only supported feature */ if (!MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS7_DATA, p, len) || !MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS7_ENCRYPTED_DATA, p, len) || !MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS7_ENVELOPED_DATA, p, len) || !MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS7_SIGNED_AND_ENVELOPED_DATA, p, len) || !MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS7_DIGESTED_DATA, p, len)) { + /* OID is valid according to the spec, but unsupported */ ret = MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE; } else { + /* OID is invalid according to the spec */ ret = MBEDTLS_ERR_PKCS7_BAD_INPUT_DATA; } goto out; From 716163e82445dadc39ecb77ff8f4064b7e1a7846 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Mon, 20 Feb 2023 14:46:51 +0000 Subject: [PATCH 13/13] Improve allocation bounds in testing Signed-off-by: Dave Rodgman --- tests/suites/test_suite_pkcs7.function | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_pkcs7.function b/tests/suites/test_suite_pkcs7.function index 9dce25e04e..91fe47b897 100644 --- a/tests/suites/test_suite_pkcs7.function +++ b/tests/suites/test_suite_pkcs7.function @@ -125,8 +125,8 @@ void pkcs7_verify(char *pkcs7_file, TEST_ASSERT(file != NULL); datalen = st.st_size; - /* Add 1 so that data is non-NULL for zero length input */ - ASSERT_ALLOC(data, datalen + 1); + /* Special-case for zero-length input so that data will be non-NULL */ + ASSERT_ALLOC(data, datalen == 0 ? 1 : datalen); buflen = fread((void *) data, sizeof(unsigned char), datalen, file); TEST_EQUAL(buflen, datalen);