Handle empty signed data in PKCS7

[jeremyevans]

This will have certificates and crls return nil instead of segfaulting.

Fixes [Bug #19974]

[ryanseys]

LGTM 👍🏼

[rhenium]

I think we should reject such input in PKCS7.new and PKCS7.read_smime. Many OpenSSL functions don't correctly handle this, and it doesn't make sense anyway. Currently, #add_certificate, #data=, etc. will also segfault with this PKCS7 object:

$ ruby -ropenssl -e'p OpenSSL::PKCS7.new("MAsGCSqGSIb3DQEHAg==".unpack1("m")).data="a"'
[...]
/usr/lib64/libcrypto.so.3(PKCS7_set_content+0x84) [0x7fc3522d6794] ../openssl-3.1.3/crypto/pkcs7/pk7_lib.c:90
/usr/lib64/libcrypto.so.3(PKCS7_set_content) (null):0
/usr/lib64/libcrypto.so.3(PKCS7_content_new+0x43) [0x7fc3522d69c3] ../openssl-3.1.3/crypto/pkcs7/pk7_lib.c:74
/usr/lib64/libcrypto.so.3(PKCS7_content_new) (null):0
[...]

Apparently, this is technically a valid PKCS#7 structure (https://datatracker.ietf.org/doc/html/rfc2315#section-7):

   ContentInfo ::= SEQUENCE {
     contentType ContentType,
     content
       [0] EXPLICIT ANY DEFINED BY contentType OPTIONAL }

but the OPTIONAL was meant to be used by a ContentInfo embedded in another ContentInfo, for example, in a detached S/MIME signature. I don't think the outermost ContentInfo is supposed to ever omit content.

FWIW, IETF's successor of PKCS#7, CMS (https://datatracker.ietf.org/doc/html/rfc2630#section-3) defines a separate structure EncapsulatedContentInfo for this use case and disallows ContentInfo to omit content.

[rhenium]

This is not a new bug. I changed the base branch to the oldest supported one (gem 3.0/Ruby 3.1).

[jeremyevans]

I updated the PR to raise an error in #initialize if there should be signed data but there is not.

[rhenium]

Sorry for the slow followup. In addition to #initialize, PKCS7.read_smime also needs this check.

It seems the enveloped-data content type has the same issue:

OpenSSL::PKCS7.new(OpenSSL::ASN1.Sequence([OpenSSL::ASN1.ObjectId("pkcs7-envelopedData")])).cipher="aes-128-cbc"

I didn't check other content types, but since there doesn't seem a valid use case for such encoding anyway (considering that CMS disallows it while claiming compatibility with PKCS#7), I think we can safely reject all PKCS7object->d.ptr == NULL.

[jeremyevans]

@rhenium I updated PKCS7.read_smime to raise the same error.

[junaruga]

This is not a new bug. I changed the base branch to the oldest supported one (gem 3.0/Ruby 3.1).

@rhenium Now, we see 227 commits on this PR to the maint-3.0 branch. So, is your intention only to merge the one original commit to the maint-3.0 branch manually later if the commit looks good to you? Do you plan to merge the commit to the master, maint-3.2, and maint-3.1 branches later?

I just wanted to share how I was backporting something as a reference. I was working for the master branch, the newest branch first, then backporting to the stable branches from the new branch to the old branch such as the maint-3.2, main-3.1, and main-3.0 branches. But it's just my preference. It doesn't mean I want you to change your way of backporting.

[pkuzco]

To completely fix the null-def issue the following change should be made on this PR:

diff --git a/ext/openssl/ossl_pkcs7.c b/ext/openssl/ossl_pkcs7.c
index 09db06c..9167f57 100644
--- a/ext/openssl/ossl_pkcs7.c
+++ b/ext/openssl/ossl_pkcs7.c
@@ -159,24 +159,16 @@ ossl_pkcs7_s_read_smime(VALUE klass, VALUE arg)
     BIO *in, *out;
     PKCS7 *pkcs7;
     VALUE ret, data;
-    int i;
 
     ret = NewPKCS7(cPKCS7);
     in = ossl_obj2bio(&arg);
     out = NULL;
     pkcs7 = SMIME_read_PKCS7(in, &out);
     BIO_free(in);
-    if(!pkcs7) ossl_raise(ePKCS7Error, NULL);
-
-    i = OBJ_obj2nid(pkcs7->type);
-    switch(i){
-      case NID_pkcs7_signed:
-      case NID_pkcs7_signedAndEnveloped:
-        if (!pkcs7->d.sign)
-            ossl_raise(rb_eArgError, "No signed data in PKCS7");
-      default:
-        ; /* nothing */
-    }
+    if(!pkcs7)
+        ossl_raise(ePKCS7Error, "Could not parse the PKCS7");
+    if(!pkcs7->d.ptr)
+        ossl_raise(ePKCS7Error, "No content in PKCS7");
 
     data = out ? ossl_membio2str(out) : Qnil;
     SetPKCS7(ret, pkcs7);
@@ -345,7 +337,6 @@ ossl_pkcs7_initialize(int argc, VALUE *argv, VALUE self)
     PKCS7 *p7, *p7_orig = RTYPEDDATA_DATA(self);
     BIO *in;
     VALUE arg;
-    int i;
 
     if(rb_scan_args(argc, argv, "01", &arg) == 0)
        return self;
@@ -358,17 +349,9 @@ ossl_pkcs7_initialize(int argc, VALUE *argv, VALUE self)
     }
     BIO_free(in);
     if (!p7)
-        ossl_raise(rb_eArgError, "Could not parse the PKCS7");
-
-    i = OBJ_obj2nid(p7->type);
-    switch(i){
-      case NID_pkcs7_signed:
-      case NID_pkcs7_signedAndEnveloped:
-        if (!p7->d.sign)
-            ossl_raise(rb_eArgError, "No signed data in PKCS7");
-      default:
-        ; /* nothing */
-    }
+        ossl_raise(ePKCS7Error, "Could not parse the PKCS7");
+    if(!p7->d.ptr)
+        ossl_raise(ePKCS7Error, "No content in PKCS7");
 
     RTYPEDDATA_DATA(self) = p7;
     PKCS7_free(p7_orig);

[rhenium]

Please see #752. I rebased this branch on top of maint-3.0 and applied @pkuzco's diff.