Revert the behavior change of CMS_get1_certs() and CMS_get1_crls()

Fixes #26079

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from https://github.com/openssl/openssl/pull/26100)

(cherry picked from commit afd36cbef8b3b7b00bd4bcdc33802d4cb39fdffa)

diff --git a/CHANGES.md b/CHANGES.md
index ce92129a3c637f96c4fc217f5f81001add2947b2..9a103d9f590d1930fb22966d1642e57b57024c24 100644 (file)
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -81,7 +81,15 @@ OpenSSL 3.5
 OpenSSL 3.4
 -----------
 
-### Changes between 3.3 and 3.4 [xx XXX xxxx]
+### Changes between 3.4.0 and 3.4.1 [xx XXX xxxx]
+
+ * Reverted the behavior change of CMS_get1_certs() and CMS_get1_crls()
+   that happened in the 3.4.0 release. These functions now return NULL
+   again if there are no certs or crls in the CMS object.
+
+   *Tomáš Mráz*
+
+### Changes between 3.3 and 3.4.0 [22 Oct 2024]
 
  * For the FIPS provider only, replaced the primary DRBG with a continuous
    health check module.  This also removes the now forbidden DRBG chaining.

diff --git a/crypto/cms/cms_lib.c b/crypto/cms/cms_lib.c
index 1d7cd7e31f18912d5cf9eb2e97cd2c444c96a2e1..ede016dff0c75902f3a4a77e7b6fcd791c5f4ac9 100644 (file)
--- a/crypto/cms/cms_lib.c
+++ b/crypto/cms/cms_lib.c
@@ -620,59 +620,91 @@ int CMS_add1_crl(CMS_ContentInfo *cms, X509_CRL *crl)
 STACK_OF(X509) *CMS_get1_certs(CMS_ContentInfo *cms)
 {
     STACK_OF(X509) *certs = NULL;
+
+    if (!ossl_cms_get1_certs_ex(cms, &certs))
+        return NULL;
+    if (sk_X509_num(certs) == 0) {
+        sk_X509_free(certs);
+        return NULL;
+    }
+    return certs;
+}
+
+int ossl_cms_get1_certs_ex(CMS_ContentInfo *cms, STACK_OF(X509) **certs)
+{
     CMS_CertificateChoices *cch;
     STACK_OF(CMS_CertificateChoices) **pcerts;
     int i, n;
 
+    if (certs == NULL)
+        return 0;
+    *certs = NULL;
     pcerts = cms_get0_certificate_choices(cms);
     if (pcerts == NULL)
-        return NULL;
+        return 0;
 
-    /* make sure to return NULL only on error */
+    /* make sure to return NULL *certs only on error */
     n = sk_CMS_CertificateChoices_num(*pcerts);
-    if ((certs = sk_X509_new_reserve(NULL, n)) == NULL)
-        return NULL;
+    if ((*certs = sk_X509_new_reserve(NULL, n)) == NULL)
+        return 0;
 
     for (i = 0; i < n; i++) {
         cch = sk_CMS_CertificateChoices_value(*pcerts, i);
         if (cch->type == 0) {
-            if (!ossl_x509_add_cert_new(&certs, cch->d.certificate,
-                                        X509_ADD_FLAG_UP_REF)) {
-                OSSL_STACK_OF_X509_free(certs);
-                return NULL;
+            if (!X509_add_cert(*certs, cch->d.certificate,
+                               X509_ADD_FLAG_UP_REF)) {
+                OSSL_STACK_OF_X509_free(*certs);
+                *certs = NULL;
+                return 0;
             }
         }
     }
-    return certs;
+    return 1;
 }
 
 STACK_OF(X509_CRL) *CMS_get1_crls(CMS_ContentInfo *cms)
 {
     STACK_OF(X509_CRL) *crls = NULL;
+
+    if (!ossl_cms_get1_crls_ex(cms, &crls))
+        return NULL;
+    if (sk_X509_CRL_num(crls) == 0) {
+        sk_X509_CRL_free(crls);
+        return NULL;
+    }
+    return crls;
+}
+
+int ossl_cms_get1_crls_ex(CMS_ContentInfo *cms, STACK_OF(X509_CRL) **crls)
+{
     STACK_OF(CMS_RevocationInfoChoice) **pcrls;
     CMS_RevocationInfoChoice *rch;
     int i, n;
 
+    if (crls == NULL)
+        return 0;
+    *crls = NULL;
     pcrls = cms_get0_revocation_choices(cms);
     if (pcrls == NULL)
-        return NULL;
+        return 0;
 
-    /* make sure to return NULL only on error */
+    /* make sure to return NULL *crls only on error */
     n = sk_CMS_RevocationInfoChoice_num(*pcrls);
-    if ((crls = sk_X509_CRL_new_reserve(NULL, n)) == NULL)
-        return NULL;
+    if ((*crls = sk_X509_CRL_new_reserve(NULL, n)) == NULL)
+        return 0;
 
     for (i = 0; i < n; i++) {
         rch = sk_CMS_RevocationInfoChoice_value(*pcrls, i);
         if (rch->type == 0) {
-            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;
+                sk_X509_CRL_pop_free(*crls, X509_CRL_free);
+                *crls = NULL;
+                return 0;
             }
         }
     }
-    return crls;
+    return 1;
 }
 
 int ossl_cms_ias_cert_cmp(CMS_IssuerAndSerialNumber *ias, X509 *cert)

diff --git a/crypto/cms/cms_local.h b/crypto/cms/cms_local.h
index fd5c7c9a6f4f47fb320c01a996b6ec39b29800e8..1d03fd7d7e2244de53cad7811cec679f36e2b1e4 100644 (file)
--- a/crypto/cms/cms_local.h
+++ b/crypto/cms/cms_local.h
@@ -485,6 +485,9 @@ int ossl_cms_ecdh_envelope(CMS_RecipientInfo *ri, int decrypt);
 int ossl_cms_rsa_envelope(CMS_RecipientInfo *ri, int decrypt);
 int ossl_cms_rsa_sign(CMS_SignerInfo *si, int verify);
 
+int ossl_cms_get1_certs_ex(CMS_ContentInfo *cms, STACK_OF(X509) **certs);
+int ossl_cms_get1_crls_ex(CMS_ContentInfo *cms, STACK_OF(X509_CRL) **crls);
+
 DECLARE_ASN1_ITEM(CMS_CertificateChoices)
 DECLARE_ASN1_ITEM(CMS_DigestedData)
 DECLARE_ASN1_ITEM(CMS_EncryptedData)

diff --git a/crypto/cms/cms_smime.c b/crypto/cms/cms_smime.c
index 6b1ab927f55b02da37c9390519546e44e33b5806..27abae7461bf5f90e4ea224729f1ca82d7555118 100644 (file)
--- a/crypto/cms/cms_smime.c
+++ b/crypto/cms/cms_smime.c
@@ -361,7 +361,7 @@ int CMS_verify(CMS_ContentInfo *cms, STACK_OF(X509) *certs,
             if (si_chains == NULL)
                 goto err;
         }
-        if ((untrusted = CMS_get1_certs(cms)) == NULL)
+        if (!ossl_cms_get1_certs_ex(cms, &untrusted))
             goto err;
         if (sk_X509_num(certs) > 0
             && !ossl_x509_add_certs_new(&untrusted, certs,
@@ -370,7 +370,7 @@ int CMS_verify(CMS_ContentInfo *cms, STACK_OF(X509) *certs,
             goto err;
 
         if ((flags & CMS_NOCRL) == 0
-            && (crls = CMS_get1_crls(cms)) == NULL)
+            && !ossl_cms_get1_crls_ex(cms, &crls))
             goto err;
         for (i = 0; i < scount; i++) {
             si = sk_CMS_SignerInfo_value(sinfos, i);

diff --git a/doc/man3/CMS_add0_cert.pod b/doc/man3/CMS_add0_cert.pod
index 39b54b240897d18a6e63f103f3d6fe43fa4a5d73..8f7e0e9e42f58c8a063a6a5622ec8e5270061c73 100644 (file)
--- a/doc/man3/CMS_add0_cert.pod
+++ b/doc/man3/CMS_add0_cert.pod
@@ -57,8 +57,8 @@ For enveloped data they are added to B<OriginatorInfo>.
 CMS_add0_cert(), CMS_add1_cert() and CMS_add0_crl() and CMS_add1_crl() return
 1 for success and 0 for failure.
 
-CMS_get1_certs() and CMS_get1_crls() return the STACK of certificates or CRLs,
-which is empty if there are none. They return NULL on error.
+CMS_get1_certs() and CMS_get1_crls() return the STACK of certificates or CRLs
+or NULL if there are none or an error occurs.
 Besides out-of-memory, the only error which will occur
 in practice is if the I<cms> type is invalid.