From: Shane Lontis Date: Mon, 3 Jun 2019 05:19:48 +0000 (+1000) Subject: Add the content type attribute to additional CMS signerinfo. X-Git-Tag: OpenSSL_1_1_1d~159 X-Git-Url: https://git.librecmc.org/?a=commitdiff_plain;h=d63d841fb510a920275c66d3e486089c5c718797;p=oweals%2Fopenssl.git Add the content type attribute to additional CMS signerinfo. Fixes #8923 Found using the openssl cms -resign option. This uses an alternate path to do the signing which was not adding the required signed attribute content type. The content type attribute should always exist since it is required is there are any signed attributes. As the signing time attribute is always added in code, the content type attribute is also required. The CMS_si_check_attributes() method adds validity checks for signed and unsigned attributes e.g. The message digest attribute is a signed attribute that must exist if any signed attributes exist, it cannot be an unsigned attribute and there must only be one instance containing a single value. Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/8944) (cherry picked from commit 19e512a8244a6f527d0194339a8f9fc45468537a) --- diff --git a/crypto/cms/cms_att.c b/crypto/cms/cms_att.c index 664e64971b..588db4dd25 100644 --- a/crypto/cms/cms_att.c +++ b/crypto/cms/cms_att.c @@ -13,6 +13,56 @@ #include #include #include "cms_lcl.h" +#include "internal/nelem.h" + +/*- + * Attribute flags. + * CMS attribute restrictions are discussed in + * - RFC 5652 Section 11. + * ESS attribute restrictions are discussed in + * - RFC 2634 Section 1.3.4 AND + * - RFC 5035 Section 5.4 + */ +/* This is a signed attribute */ +#define CMS_ATTR_F_SIGNED 0x01 +/* This is an unsigned attribute */ +#define CMS_ATTR_F_UNSIGNED 0x02 +/* Must be present if there are any other attributes of the same type */ +#define CMS_ATTR_F_REQUIRED_COND 0x10 +/* There can only be one instance of this attribute */ +#define CMS_ATTR_F_ONLY_ONE 0x20 +/* The Attribute's value must have exactly one entry */ +#define CMS_ATTR_F_ONE_ATTR_VALUE 0x40 + +/* Attributes rules for different attributes */ +static const struct { + int nid; /* The attribute id */ + int flags; +} cms_attribute_properties[] = { + /* See RFC Section 11 */ + { NID_pkcs9_contentType, CMS_ATTR_F_SIGNED + | CMS_ATTR_F_ONLY_ONE + | CMS_ATTR_F_ONE_ATTR_VALUE + | CMS_ATTR_F_REQUIRED_COND }, + { NID_pkcs9_messageDigest, CMS_ATTR_F_SIGNED + | CMS_ATTR_F_ONLY_ONE + | CMS_ATTR_F_ONE_ATTR_VALUE + | CMS_ATTR_F_REQUIRED_COND }, + { NID_pkcs9_signingTime, CMS_ATTR_F_SIGNED + | CMS_ATTR_F_ONLY_ONE + | CMS_ATTR_F_ONE_ATTR_VALUE }, + { NID_pkcs9_countersignature, CMS_ATTR_F_UNSIGNED }, + /* ESS */ + { NID_id_smime_aa_signingCertificate, CMS_ATTR_F_SIGNED + | CMS_ATTR_F_ONLY_ONE + | CMS_ATTR_F_ONE_ATTR_VALUE }, + { NID_id_smime_aa_signingCertificateV2, CMS_ATTR_F_SIGNED + | CMS_ATTR_F_ONLY_ONE + | CMS_ATTR_F_ONE_ATTR_VALUE }, + { NID_id_smime_aa_receiptRequest, CMS_ATTR_F_SIGNED + | CMS_ATTR_F_ONLY_ONE + | CMS_ATTR_F_ONE_ATTR_VALUE } +}; /* CMS SignedData Attribute utilities */ @@ -149,4 +199,86 @@ void *CMS_unsigned_get0_data_by_OBJ(CMS_SignerInfo *si, ASN1_OBJECT *oid, return X509at_get0_data_by_OBJ(si->unsignedAttrs, oid, lastpos, type); } -/* Specific attribute cases */ +/* + * Retrieve an attribute by nid from a stack of attributes starting at index + * *lastpos + 1. + * Returns the attribute or NULL if there is no attribute. + * If an attribute was found *lastpos returns the index of the found attribute. + */ +static X509_ATTRIBUTE *cms_attrib_get(int nid, + const STACK_OF(X509_ATTRIBUTE) *attrs, + int *lastpos) +{ + X509_ATTRIBUTE *at; + int loc; + + loc = X509at_get_attr_by_NID(attrs, nid, *lastpos); + if (loc < 0) + return NULL; + + at = X509at_get_attr(attrs, loc); + *lastpos = loc; + return at; +} + +static int cms_check_attribute(int nid, int flags, int type, + const STACK_OF(X509_ATTRIBUTE) *attrs, + int have_attrs) +{ + int lastpos = -1; + X509_ATTRIBUTE *at = cms_attrib_get(nid, attrs, &lastpos); + + if (at != NULL) { + int count = X509_ATTRIBUTE_count(at); + + /* Is this attribute allowed? */ + if (((flags & type) == 0) + /* check if multiple attributes of the same type are allowed */ + || (((flags & CMS_ATTR_F_ONLY_ONE) != 0) + && cms_attrib_get(nid, attrs, &lastpos) != NULL) + /* Check if attribute should have exactly one value in its set */ + || (((flags & CMS_ATTR_F_ONE_ATTR_VALUE) != 0) + && count != 1) + /* There should be at least one value */ + || count == 0) + return 0; + } else { + /* fail if a required attribute is missing */ + if (have_attrs + && ((flags & CMS_ATTR_F_REQUIRED_COND) != 0) + && (flags & type) != 0) + return 0; + } + return 1; +} + +/* + * Check that the signerinfo attributes obey the attribute rules which includes + * the following checks + * - If any signed attributes exist then there must be a Content Type + * and Message Digest attribute in the signed attributes. + * - The countersignature attribute is an optional unsigned attribute only. + * - Content Type, Message Digest, and Signing time attributes are signed + * attributes. Only one instance of each is allowed, with each of these + * attributes containing a single attribute value in its set. + */ +int CMS_si_check_attributes(const CMS_SignerInfo *si) +{ + int i; + int have_signed_attrs = (CMS_signed_get_attr_count(si) > 0); + int have_unsigned_attrs = (CMS_unsigned_get_attr_count(si) > 0); + + for (i = 0; i < (int)OSSL_NELEM(cms_attribute_properties); ++i) { + int nid = cms_attribute_properties[i].nid; + int flags = cms_attribute_properties[i].flags; + + if (!cms_check_attribute(nid, flags, CMS_ATTR_F_SIGNED, + si->signedAttrs, have_signed_attrs) + || !cms_check_attribute(nid, flags, CMS_ATTR_F_UNSIGNED, + si->unsignedAttrs, have_unsigned_attrs)) { + CMSerr(CMS_F_CMS_SI_CHECK_ATTRIBUTES, CMS_R_ATTRIBUTE_ERROR); + return 0; + } + } + return 1; +} diff --git a/crypto/cms/cms_err.c b/crypto/cms/cms_err.c index 4432b471ee..a211f4954c 100644 --- a/crypto/cms/cms_err.c +++ b/crypto/cms/cms_err.c @@ -1,6 +1,6 @@ /* * Generated by util/mkerr.pl DO NOT EDIT - * Copyright 1995-2018 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 1995-2019 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the OpenSSL license (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy @@ -146,6 +146,8 @@ static const ERR_STRING_DATA CMS_str_functs[] = { {ERR_PACK(ERR_LIB_CMS, CMS_F_CMS_SIGNERINFO_VERIFY_CONTENT, 0), "CMS_SignerInfo_verify_content"}, {ERR_PACK(ERR_LIB_CMS, CMS_F_CMS_SIGN_RECEIPT, 0), "CMS_sign_receipt"}, + {ERR_PACK(ERR_LIB_CMS, CMS_F_CMS_SI_CHECK_ATTRIBUTES, 0), + "CMS_si_check_attributes"}, {ERR_PACK(ERR_LIB_CMS, CMS_F_CMS_STREAM, 0), "CMS_stream"}, {ERR_PACK(ERR_LIB_CMS, CMS_F_CMS_UNCOMPRESS, 0), "CMS_uncompress"}, {ERR_PACK(ERR_LIB_CMS, CMS_F_CMS_VERIFY, 0), "CMS_verify"}, @@ -155,6 +157,7 @@ static const ERR_STRING_DATA CMS_str_functs[] = { static const ERR_STRING_DATA CMS_str_reasons[] = { {ERR_PACK(ERR_LIB_CMS, 0, CMS_R_ADD_SIGNER_ERROR), "add signer error"}, + {ERR_PACK(ERR_LIB_CMS, 0, CMS_R_ATTRIBUTE_ERROR), "attribute error"}, {ERR_PACK(ERR_LIB_CMS, 0, CMS_R_CERTIFICATE_ALREADY_PRESENT), "certificate already present"}, {ERR_PACK(ERR_LIB_CMS, 0, CMS_R_CERTIFICATE_HAS_NO_KEYID), diff --git a/crypto/cms/cms_lcl.h b/crypto/cms/cms_lcl.h index 916fcbfbe1..efc958d6bd 100644 --- a/crypto/cms/cms_lcl.h +++ b/crypto/cms/cms_lcl.h @@ -416,6 +416,8 @@ int cms_RecipientInfo_kari_encrypt(CMS_ContentInfo *cms, /* PWRI routines */ int cms_RecipientInfo_pwri_crypt(CMS_ContentInfo *cms, CMS_RecipientInfo *ri, int en_de); +/* SignerInfo routines */ +int CMS_si_check_attributes(const CMS_SignerInfo *si); DECLARE_ASN1_ITEM(CMS_CertificateChoices) DECLARE_ASN1_ITEM(CMS_DigestedData) diff --git a/crypto/cms/cms_sd.c b/crypto/cms/cms_sd.c index ff2d540b6a..1cb6757289 100644 --- a/crypto/cms/cms_sd.c +++ b/crypto/cms/cms_sd.c @@ -109,6 +109,27 @@ static void cms_sd_set_version(CMS_SignedData *sd) } +/* + * RFC 5652 Section 11.1 Content Type + * The content-type attribute within signed-data MUST + * 1) be present if there are signed attributes + * 2) match the content type in the signed-data, + * 3) be a signed attribute. + * 4) not have more than one copy of the attribute. + * + * Note that since the CMS_SignerInfo_sign() always adds the "signing time" + * attribute, the content type attribute MUST be added also. + * Assumptions: This assumes that the attribute does not already exist. + */ +static int cms_set_si_contentType_attr(CMS_ContentInfo *cms, CMS_SignerInfo *si) +{ + ASN1_OBJECT *ctype = cms->d.signedData->encapContentInfo->eContentType; + + /* Add the contentType attribute */ + return CMS_signed_add1_attr_by_NID(si, NID_pkcs9_contentType, + V_ASN1_OBJECT, ctype, -1) > 0; +} + /* Copy an existing messageDigest value */ static int cms_copy_messageDigest(CMS_ContentInfo *cms, CMS_SignerInfo *si) @@ -328,6 +349,8 @@ CMS_SignerInfo *CMS_add1_signer(CMS_ContentInfo *cms, if (flags & CMS_REUSE_DIGEST) { if (!cms_copy_messageDigest(cms, si)) goto err; + if (!cms_set_si_contentType_attr(cms, si)) + goto err; if (!(flags & (CMS_PARTIAL | CMS_KEY_PARAM)) && !CMS_SignerInfo_sign(si)) goto err; @@ -558,8 +581,6 @@ static int cms_SignerInfo_content_sign(CMS_ContentInfo *cms, */ if (CMS_signed_get_attr_count(si) >= 0) { - ASN1_OBJECT *ctype = - cms->d.signedData->encapContentInfo->eContentType; unsigned char md[EVP_MAX_MD_SIZE]; unsigned int mdlen; if (!EVP_DigestFinal_ex(mctx, md, &mdlen)) @@ -568,9 +589,9 @@ static int cms_SignerInfo_content_sign(CMS_ContentInfo *cms, V_ASN1_OCTET_STRING, md, mdlen)) goto err; /* Copy content type across */ - if (CMS_signed_add1_attr_by_NID(si, NID_pkcs9_contentType, - V_ASN1_OBJECT, ctype, -1) <= 0) + if (!cms_set_si_contentType_attr(cms, si)) goto err; + if (!CMS_SignerInfo_sign(si)) goto err; } else if (si->pctx) { @@ -650,6 +671,9 @@ int CMS_SignerInfo_sign(CMS_SignerInfo *si) goto err; } + if (!CMS_si_check_attributes(si)) + goto err; + if (si->pctx) pctx = si->pctx; else { @@ -696,7 +720,6 @@ int CMS_SignerInfo_sign(CMS_SignerInfo *si) OPENSSL_free(abuf); EVP_MD_CTX_reset(mctx); return 0; - } int CMS_SignerInfo_verify(CMS_SignerInfo *si) @@ -711,6 +734,9 @@ int CMS_SignerInfo_verify(CMS_SignerInfo *si) return -1; } + if (!CMS_si_check_attributes(si)) + return -1; + md = EVP_get_digestbyobj(si->digestAlgorithm->algorithm); if (md == NULL) return -1; diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index feff1dccde..bf440f33b6 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -314,6 +314,7 @@ CMS_F_CMS_SIGNERINFO_VERIFY:152:CMS_SignerInfo_verify CMS_F_CMS_SIGNERINFO_VERIFY_CERT:153:cms_signerinfo_verify_cert CMS_F_CMS_SIGNERINFO_VERIFY_CONTENT:154:CMS_SignerInfo_verify_content CMS_F_CMS_SIGN_RECEIPT:163:CMS_sign_receipt +CMS_F_CMS_SI_CHECK_ATTRIBUTES:183:CMS_si_check_attributes CMS_F_CMS_STREAM:155:CMS_stream CMS_F_CMS_UNCOMPRESS:156:CMS_uncompress CMS_F_CMS_VERIFY:157:CMS_verify @@ -1930,6 +1931,7 @@ BN_R_P_IS_NOT_PRIME:112:p is not prime BN_R_TOO_MANY_ITERATIONS:113:too many iterations BN_R_TOO_MANY_TEMPORARY_VARIABLES:109:too many temporary variables CMS_R_ADD_SIGNER_ERROR:99:add signer error +CMS_R_ATTRIBUTE_ERROR:161:attribute error CMS_R_CERTIFICATE_ALREADY_PRESENT:175:certificate already present CMS_R_CERTIFICATE_HAS_NO_KEYID:160:certificate has no keyid CMS_R_CERTIFICATE_VERIFY_ERROR:100:certificate verify error diff --git a/include/openssl/cmserr.h b/include/openssl/cmserr.h index 3f8ae26da8..f011965af9 100644 --- a/include/openssl/cmserr.h +++ b/include/openssl/cmserr.h @@ -101,6 +101,7 @@ int ERR_load_CMS_strings(void); # define CMS_F_CMS_SIGNERINFO_VERIFY_CERT 153 # define CMS_F_CMS_SIGNERINFO_VERIFY_CONTENT 154 # define CMS_F_CMS_SIGN_RECEIPT 163 +# define CMS_F_CMS_SI_CHECK_ATTRIBUTES 183 # define CMS_F_CMS_STREAM 155 # define CMS_F_CMS_UNCOMPRESS 156 # define CMS_F_CMS_VERIFY 157 @@ -110,6 +111,7 @@ int ERR_load_CMS_strings(void); * CMS reason codes. */ # define CMS_R_ADD_SIGNER_ERROR 99 +# define CMS_R_ATTRIBUTE_ERROR 161 # define CMS_R_CERTIFICATE_ALREADY_PRESENT 175 # define CMS_R_CERTIFICATE_HAS_NO_KEYID 160 # define CMS_R_CERTIFICATE_VERIFY_ERROR 100 diff --git a/test/recipes/80-test_cms.t b/test/recipes/80-test_cms.t index b57ca66dac..5dc6a3aebe 100644 --- a/test/recipes/80-test_cms.t +++ b/test/recipes/80-test_cms.t @@ -21,12 +21,13 @@ setup("test_cms"); plan skip_all => "CMS is not supported by this OpenSSL build" if disabled("cms"); +my $datadir = srctop_dir("test", "recipes", "80-test_cms_data"); my $smdir = srctop_dir("test", "smime-certs"); my $smcont = srctop_file("test", "smcont.txt"); my ($no_des, $no_dh, $no_dsa, $no_ec, $no_ec2m, $no_rc2, $no_zlib) = disabled qw/des dh dsa ec ec2m rc2 zlib/; -plan tests => 4; +plan tests => 6; my @smime_pkcs7_tests = ( @@ -400,6 +401,26 @@ my @smime_cms_param_tests = ( ] ); +my @contenttype_cms_test = ( + [ "signed content test - check that content type is added to additional signerinfo, RSA keys", + [ "-sign", "-binary", "-nodetach", "-stream", "-in", $smcont, "-outform", "DER", + "-signer", catfile($smdir, "smrsa1.pem"), "-md", "SHA256", + "-out", "test.cms" ], + [ "-resign", "-binary", "-nodetach", "-in", "test.cms", "-inform", "DER", "-outform", "DER", + "-signer", catfile($smdir, "smrsa2.pem"), "-md", "SHA256", + "-out", "test2.cms" ], + [ "-verify", "-in", "test2.cms", "-inform", "DER", + "-CAfile", catfile($smdir, "smroot.pem"), "-out", "smtst.txt" ] + ], +); + +my @incorrect_attribute_cms_test = ( + "bad_signtime_attr.cms", + "no_ct_attr.cms", + "no_md_attr.cms", + "ct_multiple_attr.cms" +); + subtest "CMS => PKCS#7 compatibility tests\n" => sub { plan tests => scalar @smime_pkcs7_tests; @@ -493,6 +514,52 @@ subtest "CMS <=> CMS consistency tests, modified key parameters\n" => sub { } }; +# Returns the number of matches of a Content Type Attribute in a binary file. +sub contentType_matches { + # Read in a binary file + my ($in) = @_; + open (HEX_IN, "$in") or die("open failed for $in : $!"); + binmode(HEX_IN); + local $/; + my $str = ; + + # Find ASN1 data for a Content Type Attribute (with a OID of PKCS7 data) + my @c = $str =~ /\x30\x18\x06\x09\x2A\x86\x48\x86\xF7\x0D\x01\x09\x03\x31\x0B\x06\x09\x2A\x86\x48\x86\xF7\x0D\x01\x07\x01/gs; + + close(HEX_IN); + return scalar(@c); +} + +subtest "CMS Check the content type attribute is added for additional signers\n" => sub { + plan tests => + (scalar @contenttype_cms_test); + + foreach (@contenttype_cms_test) { + SKIP: { + my $skip_reason = check_availability($$_[0]); + skip $skip_reason, 1 if $skip_reason; + + ok(run(app(["openssl", "cms", @{$$_[1]}])) + && run(app(["openssl", "cms", @{$$_[2]}])) + && contentType_matches("test2.cms") == 2 + && run(app(["openssl", "cms", @{$$_[3]}])), + $$_[0]); + } + } +}; + +subtest "CMS Check that bad attributes fail when verifying signers\n" => sub { + plan tests => + (scalar @incorrect_attribute_cms_test); + + foreach my $name (@incorrect_attribute_cms_test) { + ok(!run(app(["openssl", "cms", "-verify", "-in", + catfile($datadir, $name), "-inform", "DER", "-CAfile", + catfile($smdir, "smroot.pem"), "-out", "smtst.txt" ])), + $name); + } +}; + unlink "test.cms"; unlink "test2.cms"; unlink "smtst.txt"; diff --git a/test/recipes/80-test_cms_data/bad_signtime_attr.cms b/test/recipes/80-test_cms_data/bad_signtime_attr.cms new file mode 100644 index 0000000000..048a493581 Binary files /dev/null and b/test/recipes/80-test_cms_data/bad_signtime_attr.cms differ diff --git a/test/recipes/80-test_cms_data/ct_multiple_attr.cms b/test/recipes/80-test_cms_data/ct_multiple_attr.cms new file mode 100644 index 0000000000..974db6ef9a Binary files /dev/null and b/test/recipes/80-test_cms_data/ct_multiple_attr.cms differ diff --git a/test/recipes/80-test_cms_data/no_ct_attr.cms b/test/recipes/80-test_cms_data/no_ct_attr.cms new file mode 100644 index 0000000000..64b688b91c Binary files /dev/null and b/test/recipes/80-test_cms_data/no_ct_attr.cms differ diff --git a/test/recipes/80-test_cms_data/no_md_attr.cms b/test/recipes/80-test_cms_data/no_md_attr.cms new file mode 100644 index 0000000000..d0a3afa523 Binary files /dev/null and b/test/recipes/80-test_cms_data/no_md_attr.cms differ