CMS_add0_cert: if cert already present, do not throw error but ignore it

Also add checks on failing cert/CRL up_ref calls; improve coding style.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from https://github.com/openssl/openssl/pull/19199)

diff --git a/CHANGES.md b/CHANGES.md
index d02feefa6b76052ffa46b3b831de4b43fcd40a82..b5381e9847bc58edc5fafbbd01d2c5b002de66dc 100644 (file)
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -168,6 +168,13 @@ OpenSSL 3.2
 
    *David von Oheimb*
 
+ * `CMS_add0_cert()` and `CMS_add1_cert()` no more throw an error
+   if a certificate to be added is already present.
+ * `CMS_sign_ex()` and `CMS_sign()` now ignore any duplicate certificates
+   in their `certs` argument and no longer throw an error for them.
+
+   *David von Oheimb*
+
  * Fixed and extended `util/check-format.pl` for checking adherence to the
    coding style <https://www.openssl.org/policies/technical/coding-style.html>.
    The checks are meanwhile more complete and yield fewer false positives.

diff --git a/crypto/cms/cms_lib.c b/crypto/cms/cms_lib.c
index 2744306959e2084ff885f29f50cbf7786b85eb7e..a339f471e8fc73de14ef52b309ea7330a093753c 100644 (file)
--- a/crypto/cms/cms_lib.c
+++ b/crypto/cms/cms_lib.c
@@ -537,9 +537,9 @@ int CMS_add0_cert(CMS_ContentInfo *cms, X509 *cert)
     for (i = 0; i < sk_CMS_CertificateChoices_num(*pcerts); i++) {
         cch = sk_CMS_CertificateChoices_value(*pcerts, i);
         if (cch->type == CMS_CERTCHOICE_CERT) {
-            if (!X509_cmp(cch->d.certificate, cert)) {
-                ERR_raise(ERR_LIB_CMS, CMS_R_CERTIFICATE_ALREADY_PRESENT);
-                return 0;
+            if (X509_cmp(cch->d.certificate, cert) == 0) {
+                X509_free(cert);
+                return 1; /* cert already present */
             }
         }
     }
@@ -553,11 +553,12 @@ int CMS_add0_cert(CMS_ContentInfo *cms, X509 *cert)
 
 int CMS_add1_cert(CMS_ContentInfo *cms, X509 *cert)
 {
-    int r;
-    r = CMS_add0_cert(cms, cert);
-    if (r > 0)
-        X509_up_ref(cert);
-    return r;
+    if (!X509_up_ref(cert))
+        return 0;
+    if (CMS_add0_cert(cms, cert))
+        return 1;
+    X509_free(cert);
+    return 0;
 }
 
 static STACK_OF(CMS_RevocationInfoChoice)
@@ -609,9 +610,9 @@ CMS_RevocationInfoChoice *CMS_add0_RevocationInfoChoice(CMS_ContentInfo *cms)
 
 int CMS_add0_crl(CMS_ContentInfo *cms, X509_CRL *crl)
 {
-    CMS_RevocationInfoChoice *rch;
-    rch = CMS_add0_RevocationInfoChoice(cms);
-    if (!rch)
+    CMS_RevocationInfoChoice *rch = CMS_add0_RevocationInfoChoice(cms);
+
+    if (rch == NULL)
         return 0;
     rch->type = CMS_REVCHOICE_CRL;
     rch->d.crl = crl;
@@ -665,16 +666,15 @@ STACK_OF(X509_CRL) *CMS_get1_crls(CMS_ContentInfo *cms)
     for (i = 0; i < sk_CMS_RevocationInfoChoice_num(*pcrls); i++) {
         rch = sk_CMS_RevocationInfoChoice_value(*pcrls, i);
         if (rch->type == 0) {
-            if (!crls) {
-                crls = sk_X509_CRL_new_null();
-                if (!crls)
+            if (crls == NULL) {
+                if ((crls = sk_X509_CRL_new_null()) == NULL)
                     return NULL;
             }
-            if (!sk_X509_CRL_push(crls, rch->d.crl)) {
+            if (!sk_X509_CRL_push(crls, rch->d.crl)
+                    || !X509_CRL_up_ref(rch->d.crl)) {
                 sk_X509_CRL_pop_free(crls, X509_CRL_free);
                 return NULL;
             }
-            X509_CRL_up_ref(rch->d.crl);
         }
     }
     return crls;

diff --git a/doc/man3/CMS_add0_cert.pod b/doc/man3/CMS_add0_cert.pod
index 37b6551ffb26227afa2d57d1b8e43702f9d3eee4..eb105d72961f4b1b21c9d0ba4ffd2b64cd0b3b83 100644 (file)
--- a/doc/man3/CMS_add0_cert.pod
+++ b/doc/man3/CMS_add0_cert.pod
@@ -20,7 +20,13 @@ CMS_add0_crl, CMS_add1_crl, CMS_get1_crls
 
 =head1 DESCRIPTION
 
-CMS_add0_cert() and CMS_add1_cert() add certificate I<cert> to I<cms>.
+CMS_add0_cert() and CMS_add1_cert() add certificate I<cert> to I<cms>
+unless it is already present.
+This is used by L<CMS_sign_ex()> and L<CMS_sign()> and may be used before
+calling L<CMS_verify()> to help chain building in certificate validation.
+As the 0 implies, CMS_add0_cert() adds I<cert> internally to I<cms>
+and on success it must not be freed up by the caller.
+In contrast, the caller of CMS_add1_cert() must free I<cert>.
 I<cms> must be of type signed data or (authenticated) enveloped data.
 For signed data, such a certificate can be used when signing or verifying
 to fill in the signer certificate or to provide an extra CA certificate
@@ -30,7 +36,8 @@ CMS_get1_certs() returns all certificates in I<cms>.
 
 CMS_add0_crl() and CMS_add1_crl() add CRL I<crl> to I<cms>.
 I<cms> must be of type signed data or (authenticated) enveloped data.
-For signed data, such a CRL may be used in certificate validation.
+For signed data, such a CRL may be used in certificate validation
+with L<CMS_verify()>.
 It may be given both for inclusion when signing a CMS message
 and when verifying a signed CMS message.
 
@@ -45,13 +52,6 @@ For signed data, certificates and CRLs are added to the I<certificates> and
 I<crls> fields of SignedData structure.
 For enveloped data they are added to B<OriginatorInfo>.
 
-As the 0 implies, CMS_add0_cert() adds I<cert> internally to I<cms> and it
-must not be freed up after the call as opposed to CMS_add1_cert() where I<cert>
-must be freed up.
-
-The same certificate or CRL must not be added to the same cms structure more
-than once.
-
 =head1 RETURN VALUES
 
 CMS_add0_cert(), CMS_add1_cert() and CMS_add0_crl() and CMS_add1_crl() return
@@ -64,9 +64,14 @@ in practice is if the I<cms> type is invalid.
 =head1 SEE ALSO
 
 L<ERR_get_error(3)>,
-L<CMS_sign(3)>,
+L<CMS_sign(3)>, L<CMS_sign_ex(3)>, L<CMS_verify(3)>,
 L<CMS_encrypt(3)>
 
+=head1 HISTORY
+
+CMS_add0_cert() and CMS_add1_cert() have been changed in OpenSSL 3.2
+not to throw an error if a certificate to be added is already present.
+
 =head1 COPYRIGHT
 
 Copyright 2008-2016 The OpenSSL Project Authors. All Rights Reserved.

diff --git a/doc/man3/CMS_sign.pod b/doc/man3/CMS_sign.pod
index 0d812756aef55133eaca11c4b5c09b000ad325f2..7a0fcbb53c59846e52054e4a91136926bc3b011d 100644 (file)
--- a/doc/man3/CMS_sign.pod
+++ b/doc/man3/CMS_sign.pod
@@ -130,6 +130,9 @@ it is supported for embedded data in OpenSSL 1.0.0 and later.
 
 The CMS_sign_ex() method was added in OpenSSL 3.0.
 
+Since OpenSSL 3.2, CMS_sign_ex() and CMS_sign() ignore any duplicate
+certificates in their I<certs> argument and no longer throw an error for them.
+
 =head1 COPYRIGHT
 
 Copyright 2008-2020 The OpenSSL Project Authors. All Rights Reserved.

diff --git a/doc/man3/CMS_verify.pod b/doc/man3/CMS_verify.pod
index d124234ab15e5c806031985598856cab81582019..ddd2a8970cee7d307a7a6304b5bb71c4fd7da2fc 100644 (file)
--- a/doc/man3/CMS_verify.pod
+++ b/doc/man3/CMS_verify.pod
@@ -40,7 +40,7 @@ it operates on B<CMS SignedData> input in the I<sd> argument,
 it has some additional parameters described next,
 and on success it returns the verfied content as a memory BIO.
 The optional I<extra> parameter may be used to provide untrusted CA
-certificates that may be helpful for chain building in certificate valiation.
+certificates that may be helpful for chain building in certificate validation.
 This list of certificates must not contain duplicates.
 The optional I<crls> parameter may be used to provide extra CRLs.
 Also the list of CRLs must not contain duplicates.

diff --git a/test/cmsapitest.c b/test/cmsapitest.c
index ee08c0c97452a5c5ff14b6ded4138e381ba7c51b..6e59b48813293d230fc7ae7b0de83267bda5f84c 100644 (file)
--- a/test/cmsapitest.c
+++ b/test/cmsapitest.c
@@ -88,6 +88,19 @@ static int test_encrypt_decrypt_aes_256_gcm(void)
     return test_encrypt_decrypt(EVP_aes_256_gcm());
 }
 
+static int test_CMS_add1_cert(void)
+{
+    CMS_ContentInfo *cms = NULL;
+    int ret = 0;
+
+    ret = TEST_ptr(cms = CMS_ContentInfo_new())
+        && TEST_ptr(CMS_add1_signer(cms, cert, privkey, NULL, 0))
+        && TEST_true(CMS_add1_cert(cms, cert)); /* add cert again */
+
+    CMS_ContentInfo_free(cms);
+    return ret;
+}
+
 static int test_d2i_CMS_bio_NULL(void)
 {
     BIO *bio, *content = NULL;
@@ -413,6 +426,7 @@ int setup_tests(void)
     ADD_TEST(test_encrypt_decrypt_aes_128_gcm);
     ADD_TEST(test_encrypt_decrypt_aes_192_gcm);
     ADD_TEST(test_encrypt_decrypt_aes_256_gcm);
+    ADD_TEST(test_CMS_add1_cert);
     ADD_TEST(test_d2i_CMS_bio_NULL);
     ADD_ALL_TESTS(test_d2i_CMS_decode, 2);
     return 1;