From 3dafc6c3b3a02bc19bb0fd54dbbd639d1c2ded47 Mon Sep 17 00:00:00 2001 From: Nick Child Date: Tue, 7 Feb 2023 19:59:58 +0000 Subject: [PATCH] pkcs7: Drop support for signature in contentInfo of signed data The contentInfo field of PKCS7 Signed Data structures can optionally contain the content of the signature. Per RFC 2315 it can also contain any of the PKCS7 data types. Add test and comments making it clear that the current implementation only supports the DATA content type and the data must be empty. Return codes should be clear whether content was invalid or unsupported. Identification and fix provided by: - Demi Marie Obenour - Dave Rodgman Signed-off-by: Nick Child --- include/mbedtls/pkcs7.h | 2 ++ library/pkcs7.c | 14 ++++++++++++-- tests/data_files/Makefile | 5 +++++ tests/data_files/pkcs7_data_with_signature.der | Bin 0 -> 446 bytes tests/suites/test_suite_pkcs7.data | 8 ++++++-- 5 files changed, 25 insertions(+), 4 deletions(-) create mode 100644 tests/data_files/pkcs7_data_with_signature.der diff --git a/include/mbedtls/pkcs7.h b/include/mbedtls/pkcs7.h index 82f282bc2b..5ddd5a3d71 100644 --- a/include/mbedtls/pkcs7.h +++ b/include/mbedtls/pkcs7.h @@ -46,6 +46,8 @@ * - The RFC allows for SignerInfo structure to optionally contain * unauthenticatedAttributes and authenticatedAttributes. In Mbed TLS it is * assumed these fields are empty. + * - The RFC allows for the signed Data type to contain contentInfo. This + * implementation assumes the type is DATA and the content is empty. */ #ifndef MBEDTLS_PKCS7_H diff --git a/library/pkcs7.c b/library/pkcs7.c index 273842b2fd..1159b9beea 100644 --- a/library/pkcs7.c +++ b/library/pkcs7.c @@ -495,8 +495,18 @@ static int pkcs7_get_signed_data(unsigned char *buf, size_t buflen, return ret; } - if (end_content_info != p) { - return MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO; + if (p != end_content_info) { + /* Determine if valid content is present */ + ret = mbedtls_asn1_get_tag(&p, end_content_info, &len, MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_CONTEXT_SPECIFIC); + if (ret != 0) { + return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, ret); + } + p += len; + if (p != end_content_info) { + return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, ret); + } + /* Valid content is present - this is not supported */ + return MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE; } if (MBEDTLS_OID_CMP(MBEDTLS_OID_PKCS7_DATA, &signed_data->content.oid)) { diff --git a/tests/data_files/Makefile b/tests/data_files/Makefile index 14c1744ac3..7121b5b170 100644 --- a/tests/data_files/Makefile +++ b/tests/data_files/Makefile @@ -1202,6 +1202,11 @@ pkcs7_data_without_cert_signed.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 -nocerts -noattr -outform DER -out $@ all_final += pkcs7_data_without_cert_signed.der +# pkcs7 signature file with signature +pkcs7_data_with_signature.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 -nocerts -noattr -nodetach -outform DER -out $@ +all_final += pkcs7_data_with_signature.der + # pkcs7 signature file with two signers pkcs7_data_multiple_signed.der: $(pkcs7_test_file) $(pkcs7_test_cert_1) $(pkcs7_test_cert_2) $(OPENSSL) smime -sign -binary -in pkcs7_data.bin -out $@ -md sha256 -signer pkcs7-rsa-sha256-1.pem -signer pkcs7-rsa-sha256-2.pem -nocerts -noattr -outform DER -out $@ diff --git a/tests/data_files/pkcs7_data_with_signature.der b/tests/data_files/pkcs7_data_with_signature.der new file mode 100644 index 0000000000000000000000000000000000000000..cb9d1267fb31d7633f5ef781447f8c82b8a8f118 GIT binary patch literal 446 zcmXqLV%){XsnzDu_MMlJooPW6<7$H@#^p?mjE4LMylk8aZ61uN%q&cdtPBQX2!)Ib zI9b>|Qgd?hdASUm7|Vfrih+6zd<;wsxeYkkm_u3Egqi$&4EYRrKpZX})&OtkU~@xJ z17Q%KnTH3=S8z@(Dp4?G5~;p-_mYdx#k2on99IbFbZ)<|m$M(;exP%p4rK&7hoy;; zA^OdY!l?)E{hx5iXQJBd(9It*_b9~}`=6A$31TTJeQSo#k{q9%?^>oE_MCSOGJ-7sJGoPqrLp^ Oo0h2Q`)dVsF8~18uco{J literal 0 HcmV?d00001 diff --git a/tests/suites/test_suite_pkcs7.data b/tests/suites/test_suite_pkcs7.data index 1319d7b221..840a24bb61 100644 --- a/tests/suites/test_suite_pkcs7.data +++ b/tests/suites/test_suite_pkcs7.data @@ -22,6 +22,10 @@ PKCS7 Signed Data Parse Fail with disabled alg #5.1 depends_on:MBEDTLS_RSA_C:!MBEDTLS_SHA512_C pkcs7_parse:"data_files/pkcs7_data_cert_signed_sha512.der":MBEDTLS_ERR_PKCS7_INVALID_ALG +PKCS7 Parse Fail with Inlined Content Info #5.2 +depends_on:MBEDTLS_SHA256_C:MBEDTLS_RSA_C +pkcs7_parse:"data_files/pkcs7_data_with_signature.der":MBEDTLS_ERR_PKCS7_FEATURE_UNAVAILABLE + PKCS7 Signed Data Parse Fail with corrupted signer info #6 depends_on:MBEDTLS_SHA256_C:MBEDTLS_RSA_C pkcs7_parse:"data_files/pkcs7_data_signed_badsigner.der":MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_SIGNER_INFO,MBEDTLS_ERR_ASN1_UNEXPECTED_TAG) @@ -68,11 +72,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_CONTENT_INFO +pkcs7_parse:"data_files/pkcs7_get_signers_info_set-missing_free-fuzz_pkcs7-6213931373035520.der":MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, MBEDTLS_ERR_ASN1_UNEXPECTED_TAG) 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_CONTENT_INFO +pkcs7_parse:"data_files/pkcs7_get_signers_info_set-leak-fuzz_pkcs7-4541044530479104.der": MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO, MBEDTLS_ERR_ASN1_UNEXPECTED_TAG) PKCS7 Only Signed Data Parse Pass #15 depends_on:MBEDTLS_SHA256_C:MBEDTLS_RSA_C