From 35598adb781c71b93b400bff9fc7f5b7c1e957c7 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 30 Nov 2022 02:06:07 -0500 Subject: [PATCH] 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; }