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 <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/8944)

(cherry picked from commit 19e512a8244a6f527d0194339a8f9fc45468537a)

4 files changed, 170 insertions, 7 deletions

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 <openssl/err.h>
 #include <openssl/cms.h>
 #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;