Merge branch 'maint-3.2'

* maint-3.2: test_provider.rb: Make a legacy provider test optional. .github/workflows/test.yml: synchronize with master pkcs7: fix memory leak in error path of PKCS7.new and .read_smime asn1: fix ObjectId#== x509: fix handling of multiple URIs in Certificate#crl_uris test_x509cert.rb: break up test_extension into smaller units
ruby · Nov 12, 2024 · 9092c27 · 9092c27
2 parents bd647c3 + 1f07615
commit 9092c27
Show file tree

Hide file tree

Showing 5 changed files with 126 additions and 51 deletions.
diff --git a/ext/openssl/ossl_asn1.c b/ext/openssl/ossl_asn1.c
@@ -1167,30 +1167,6 @@ ossl_asn1obj_get_ln(VALUE self)
     return ret;
 }
 
-/*
- *  call-seq:
- *     oid == other_oid => true or false
- *
- *  Returns +true+ if _other_oid_ is the same as _oid_
- */
-static VALUE
-ossl_asn1obj_eq(VALUE self, VALUE other)
-{
-    VALUE valSelf, valOther;
-    int nidSelf, nidOther;
-
-    valSelf = ossl_asn1_get_value(self);
-    valOther = ossl_asn1_get_value(other);
-
-    if ((nidSelf = OBJ_txt2nid(StringValueCStr(valSelf))) == NID_undef)
-	ossl_raise(eASN1Error, "OBJ_txt2nid");
-
-    if ((nidOther = OBJ_txt2nid(StringValueCStr(valOther))) == NID_undef)
-	ossl_raise(eASN1Error, "OBJ_txt2nid");
-
-    return nidSelf == nidOther ? Qtrue : Qfalse;
-}
-
 static VALUE
 asn1obj_get_oid_i(VALUE vobj)
 {
@@ -1235,6 +1211,25 @@ ossl_asn1obj_get_oid(VALUE self)
     return str;
 }
 
+/*
+ *  call-seq:
+ *     oid == other_oid => true or false
+ *
+ *  Returns +true+ if _other_oid_ is the same as _oid_.
+ */
+static VALUE
+ossl_asn1obj_eq(VALUE self, VALUE other)
+{
+    VALUE oid1, oid2;
+
+    if (!rb_obj_is_kind_of(other, cASN1ObjectId))
+        return Qfalse;
+
+    oid1 = ossl_asn1obj_get_oid(self);
+    oid2 = ossl_asn1obj_get_oid(other);
+    return rb_str_equal(oid1, oid2);
+}
+
 #define OSSL_ASN1_IMPL_FACTORY_METHOD(klass) \
 static VALUE ossl_asn1_##klass(int argc, VALUE *argv, VALUE self)\
 { return rb_funcallv_public(cASN1##klass, rb_intern("new"), argc, argv); }

diff --git a/ext/openssl/ossl_pkcs7.c b/ext/openssl/ossl_pkcs7.c
@@ -167,8 +167,10 @@ ossl_pkcs7_s_read_smime(VALUE klass, VALUE arg)
     BIO_free(in);
     if (!pkcs7)
         ossl_raise(ePKCS7Error, "Could not parse the PKCS7");
-    if (!pkcs7->d.ptr)
+    if (!pkcs7->d.ptr) {
+        PKCS7_free(pkcs7);
         ossl_raise(ePKCS7Error, "No content in PKCS7");
+    }
 
     data = out ? ossl_membio2str(out) : Qnil;
     SetPKCS7(ret, pkcs7);
@@ -348,8 +350,10 @@ ossl_pkcs7_initialize(int argc, VALUE *argv, VALUE self)
     BIO_free(in);
     if (!p7)
         ossl_raise(rb_eArgError, "Could not parse the PKCS7");
-    if (!p7->d.ptr)
+    if (!p7->d.ptr) {
+        PKCS7_free(p7);
         ossl_raise(rb_eArgError, "No content in PKCS7");
+    }
 
     RTYPEDDATA_DATA(self) = p7;
     PKCS7_free(p7_orig);

diff --git a/lib/openssl/x509.rb b/lib/openssl/x509.rb
@@ -122,8 +122,8 @@ module CRLDistributionPoints
         include Helpers
 
         # Get the distributionPoint fullName URI from the certificate's CRL
-        # distribution points extension, as described in RFC5280 Section
-        # 4.2.1.13
+        # distribution points extension, as described in RFC 5280 Section
+        # 4.2.1.13.
         #
         # Returns an array of strings or nil or raises ASN1::ASN1Error.
         def crl_uris
@@ -135,19 +135,19 @@ def crl_uris
             raise ASN1::ASN1Error, "invalid extension"
           end
 
-          crl_uris = cdp_asn1.map do |crl_distribution_point|
+          crl_uris = cdp_asn1.flat_map do |crl_distribution_point|
             distribution_point = crl_distribution_point.value.find do |v|
               v.tag_class == :CONTEXT_SPECIFIC && v.tag == 0
             end
             full_name = distribution_point&.value&.find do |v|
               v.tag_class == :CONTEXT_SPECIFIC && v.tag == 0
             end
-            full_name&.value&.find do |v|
+            full_name&.value&.select do |v|
               v.tag_class == :CONTEXT_SPECIFIC && v.tag == 6 # uniformResourceIdentifier
             end
           end
 
-          crl_uris&.map(&:value)
+          crl_uris.empty? ? nil : crl_uris.map(&:value)
         end
       end
 

diff --git a/test/openssl/test_asn1.rb b/test/openssl/test_asn1.rb
@@ -326,7 +326,9 @@ def test_object_identifier
     oid = (0...100).to_a.join(".").b
     obj = OpenSSL::ASN1::ObjectId.new(oid)
     assert_equal oid, obj.oid
+  end
 
+  def test_object_identifier_equality
     aki = [
       OpenSSL::ASN1::ObjectId.new("authorityKeyIdentifier"),
       OpenSSL::ASN1::ObjectId.new("X509v3 Authority Key Identifier"),
@@ -341,17 +343,22 @@ def test_object_identifier
 
     aki.each do |a|
       aki.each do |b|
-        assert a == b
+        assert_equal true, a == b
       end
 
       ski.each do |b|
-        refute a == b
+        assert_equal false, a == b
       end
     end
 
-    assert_raise(TypeError) {
-      OpenSSL::ASN1::ObjectId.new("authorityKeyIdentifier") == nil
-    }
+    obj1 = OpenSSL::ASN1::ObjectId.new("1.2.34.56789.10")
+    obj2 = OpenSSL::ASN1::ObjectId.new("1.2.34.56789.10")
+    obj3 = OpenSSL::ASN1::ObjectId.new("1.2.34.56789.11")
+    omit "OID 1.2.34.56789.10 is registered" if obj1.sn
+    assert_equal true, obj1 == obj2
+    assert_equal false, obj1 == obj3
+
+    assert_equal false, OpenSSL::ASN1::ObjectId.new("authorityKeyIdentifier") == nil
   end
 
   def test_sequence

diff --git a/test/openssl/test_x509cert.rb b/test/openssl/test_x509cert.rb
@@ -68,17 +68,14 @@ def test_validity
     assert_equal(now.getutc, cert.not_after)
   end
 
-  def test_extension
+  def test_extension_factory
     ca_exts = [
       ["basicConstraints","CA:TRUE",true],
       ["keyUsage","keyCertSign, cRLSign",true],
       ["subjectKeyIdentifier","hash",false],
       ["authorityKeyIdentifier","issuer:always,keyid:always",false],
     ]
     ca_cert = issue_cert(@ca, @rsa2048, 1, ca_exts, nil, nil)
-    keyid = get_subject_key_id(ca_cert.to_der, hex: false)
-    assert_equal keyid, ca_cert.authority_key_identifier
-    assert_equal keyid, ca_cert.subject_key_identifier
     ca_cert.extensions.each_with_index{|ext, i|
       assert_equal(ca_exts[i].first, ext.oid)
       assert_equal(ca_exts[i].last, ext.critical?)
@@ -90,33 +87,112 @@ def test_extension
       ["authorityKeyIdentifier","issuer:always,keyid:always",false],
       ["extendedKeyUsage","clientAuth, emailProtection, codeSigning",false],
       ["subjectAltName","email:[email protected]",false],
-      ["authorityInfoAccess","caIssuers;URI:http://www.example.com/caIssuers,OCSP;URI:http://www.example.com/ocsp",false],
     ]
     ee1_cert = issue_cert(@ee1, @rsa1024, 2, ee1_exts, ca_cert, @rsa2048)
     assert_equal(ca_cert.subject.to_der, ee1_cert.issuer.to_der)
     ee1_cert.extensions.each_with_index{|ext, i|
       assert_equal(ee1_exts[i].first, ext.oid)
       assert_equal(ee1_exts[i].last, ext.critical?)
     }
-    assert_nil(ee1_cert.crl_uris)
+  end
+
+  def test_akiski
+    ca_cert = generate_cert(@ca, @rsa2048, 4, nil)
+    ef = OpenSSL::X509::ExtensionFactory.new(ca_cert, ca_cert)
+    ca_cert.add_extension(
+      ef.create_extension("subjectKeyIdentifier", "hash", false))
+    ca_cert.add_extension(
+      ef.create_extension("authorityKeyIdentifier", "issuer:always,keyid:always", false))
+    ca_cert.sign(@rsa2048, "sha256")
+
+    ca_keyid = get_subject_key_id(ca_cert.to_der, hex: false)
+    assert_equal ca_keyid, ca_cert.authority_key_identifier
+    assert_equal ca_keyid, ca_cert.subject_key_identifier
+
+    ee_cert = generate_cert(@ee1, Fixtures.pkey("p256"), 5, ca_cert)
+    ef = OpenSSL::X509::ExtensionFactory.new(ca_cert, ee_cert)
+    ee_cert.add_extension(
+      ef.create_extension("subjectKeyIdentifier", "hash", false))
+    ee_cert.add_extension(
+      ef.create_extension("authorityKeyIdentifier", "issuer:always,keyid:always", false))
+    ee_cert.sign(@rsa2048, "sha256")
+
+    ee_keyid = get_subject_key_id(ee_cert.to_der, hex: false)
+    assert_equal ca_keyid, ee_cert.authority_key_identifier
+    assert_equal ee_keyid, ee_cert.subject_key_identifier
+  end
+
+  def test_akiski_missing
+    cert = issue_cert(@ee1, @rsa2048, 1, [], nil, nil)
+    assert_nil(cert.authority_key_identifier)
+    assert_nil(cert.subject_key_identifier)
+  end
+
+  def test_crl_uris_no_crl_distribution_points
+    cert = issue_cert(@ee1, @rsa2048, 1, [], nil, nil)
+    assert_nil(cert.crl_uris)
+  end
 
+  def test_crl_uris
+    # Multiple DistributionPoint contains a single general name each
     ef = OpenSSL::X509::ExtensionFactory.new
     ef.config = OpenSSL::Config.parse(<<~_cnf_)
       [crlDistPts]
       URI.1 = http://www.example.com/crl
       URI.2 = ldap://ldap.example.com/cn=ca?certificateRevocationList;binary
     _cnf_
-    cdp_cert = generate_cert(@ee1, @rsa1024, 3, ca_cert)
+    cdp_cert = generate_cert(@ee1, @rsa2048, 3, nil)
     ef.subject_certificate = cdp_cert
     cdp_cert.add_extension(ef.create_extension("crlDistributionPoints", "@crlDistPts"))
     cdp_cert.sign(@rsa2048, "sha256")
     assert_equal(
       ["http://www.example.com/crl", "ldap://ldap.example.com/cn=ca?certificateRevocationList;binary"],
       cdp_cert.crl_uris
     )
+  end
 
+  def test_crl_uris_multiple_general_names
+    # Single DistributionPoint contains multiple general names of type URI
     ef = OpenSSL::X509::ExtensionFactory.new
-    aia_cert = generate_cert(@ee1, @rsa1024, 4, ca_cert)
+    ef.config = OpenSSL::Config.parse(<<~_cnf_)
+      [crlDistPts_section]
+      fullname = URI:http://www.example.com/crl, URI:ldap://ldap.example.com/cn=ca?certificateRevocationList;binary
+    _cnf_
+    cdp_cert = generate_cert(@ee1, @rsa2048, 3, nil)
+    ef.subject_certificate = cdp_cert
+    cdp_cert.add_extension(ef.create_extension("crlDistributionPoints", "crlDistPts_section"))
+    cdp_cert.sign(@rsa2048, "sha256")
+    assert_equal(
+      ["http://www.example.com/crl", "ldap://ldap.example.com/cn=ca?certificateRevocationList;binary"],
+      cdp_cert.crl_uris
+    )
+  end
+
+  def test_crl_uris_no_uris
+    # The only DistributionPointName is a directoryName
+    ef = OpenSSL::X509::ExtensionFactory.new
+    ef.config = OpenSSL::Config.parse(<<~_cnf_)
+      [crlDistPts_section]
+      fullname = dirName:dirname_section
+      [dirname_section]
+      CN = dirname
+    _cnf_
+    cdp_cert = generate_cert(@ee1, @rsa2048, 3, nil)
+    ef.subject_certificate = cdp_cert
+    cdp_cert.add_extension(ef.create_extension("crlDistributionPoints", "crlDistPts_section"))
+    cdp_cert.sign(@rsa2048, "sha256")
+    assert_nil(cdp_cert.crl_uris)
+  end
+
+  def test_aia_missing
+    cert = issue_cert(@ee1, @rsa2048, 1, [], nil, nil)
+    assert_nil(cert.ca_issuer_uris)
+    assert_nil(cert.ocsp_uris)
+  end
+
+  def test_aia
+    ef = OpenSSL::X509::ExtensionFactory.new
+    aia_cert = generate_cert(@ee1, @rsa2048, 4, nil)
     ef.subject_certificate = aia_cert
     aia_cert.add_extension(
       ef.create_extension(
@@ -137,13 +213,6 @@ def test_extension
       ["http://www.example.com/ocsp", "ldap://ldap.example.com/cn=ca?authorityInfoAccessOcsp;binary"],
       aia_cert.ocsp_uris
     )
-
-    no_exts_cert = issue_cert(@ca, @rsa2048, 5, [], nil, nil)
-    assert_equal nil, no_exts_cert.authority_key_identifier
-    assert_equal nil, no_exts_cert.subject_key_identifier
-    assert_equal nil, no_exts_cert.crl_uris
-    assert_equal nil, no_exts_cert.ca_issuer_uris
-    assert_equal nil, no_exts_cert.ocsp_uris
   end
 
   def test_invalid_extension