From adb42b5b843e2a4808a525fdc179023144276499 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Sun, 9 Feb 2025 19:37:41 +0900 Subject: [PATCH 1/4] pkcs7: add a test case for the data content type While it is not useful alone, it is still a valid content type. Some methods on OpenSSL::PKCS7 are only meant to work with the signed-data or enveloped-data content type. Add some assertions for their behavior with unsupported content types. The next patches will update the relevant code. --- test/openssl/test_pkcs7.rb | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/openssl/test_pkcs7.rb b/test/openssl/test_pkcs7.rb index 862716b4d..7e5bd6f17 100644 --- a/test/openssl/test_pkcs7.rb +++ b/test/openssl/test_pkcs7.rb @@ -160,6 +160,34 @@ def test_enveloped } end + def test_data + asn1 = OpenSSL::ASN1::Sequence([ + OpenSSL::ASN1::ObjectId("pkcs7-data"), + OpenSSL::ASN1::OctetString("content", 0, :EXPLICIT), + ]) + p7 = OpenSSL::PKCS7.new + p7.type = :data + p7.data = "content" + assert_raise(OpenSSL::PKCS7::PKCS7Error) { p7.add_certificate(@ee1_cert) } + assert_raise(OpenSSL::PKCS7::PKCS7Error) { p7.certificates = [@ee1_cert] } + assert_raise(OpenSSL::PKCS7::PKCS7Error) { p7.cipher = "aes-128-cbc" } + assert_equal(asn1.to_der, p7.to_der) + + p7 = OpenSSL::PKCS7.new(asn1) + assert_equal(:data, p7.type) + assert_equal(false, p7.detached?) + # Not applicable + assert_nil(p7.certificates) + assert_nil(p7.crls) + # Not applicable. Should they return nil or raise an exception instead? + assert_equal([], p7.signers) + assert_equal([], p7.recipients) + # PKCS7#verify can't distinguish verification failure and other errors + store = OpenSSL::X509::Store.new + assert_equal(false, p7.verify([@ee1_cert], store)) + assert_raise(OpenSSL::PKCS7::PKCS7Error) { p7.decrypt(@rsa1024) } + end + def test_empty_signed_data_ruby_bug_19974 data = "-----BEGIN PKCS7-----\nMAsGCSqGSIb3DQEHAg==\n-----END PKCS7-----\n" assert_raise(ArgumentError) { OpenSSL::PKCS7.new(data) } From 84cffd4f77316204b9b215de5a1990d85c8b0ec6 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Tue, 7 Jan 2025 02:14:46 +0900 Subject: [PATCH 2/4] ossl.c: avoid using sk_*() functions with NULL Always use explicit NULL checks before interacting with STACK_OF(*). Even though most OpenSSL functions named sk_*() do not crash if we pass NULL as the receiver object, depending on this behavior would be a bad idea. Checks for a negative number return from sk_*_num() are removed. This can only happen when the stack is NULL. ossl_*_sk2ary() must no longer be called with NULL. --- ext/openssl/ossl.c | 11 ++------- ext/openssl/ossl_pkcs7.c | 46 +++++++++++++++++++++----------------- ext/openssl/ossl_ssl.c | 4 +++- ext/openssl/ossl_x509crl.c | 22 +++++++++--------- 4 files changed, 40 insertions(+), 43 deletions(-) diff --git a/ext/openssl/ossl.c b/ext/openssl/ossl.c index 27d7f9cfd..60780790b 100644 --- a/ext/openssl/ossl.c +++ b/ext/openssl/ossl.c @@ -69,16 +69,9 @@ ossl_##name##_sk2ary(const STACK_OF(type) *sk) \ int i, num; \ VALUE ary; \ \ - if (!sk) { \ - OSSL_Debug("empty sk!"); \ - return Qnil; \ - } \ + RUBY_ASSERT(sk != NULL); \ num = sk_##type##_num(sk); \ - if (num < 0) { \ - OSSL_Debug("items in sk < -1???"); \ - return rb_ary_new(); \ - } \ - ary = rb_ary_new2(num); \ + ary = rb_ary_new_capa(num); \ \ for (i=0; id.signed_and_enveloped->recipientinfo; else sk = NULL; if (!sk) return rb_ary_new(); - if ((num = sk_PKCS7_RECIP_INFO_num(sk)) < 0) { - ossl_raise(ePKCS7Error, "Negative number of recipient!"); - } - ary = rb_ary_new2(num); + num = sk_PKCS7_RECIP_INFO_num(sk); + ary = rb_ary_new_capa(num); for (i=0; i [x509name, ...] + * ssl.client_ca => [x509name, ...] or nil * * Returns the list of client CAs. Please note that in contrast to * SSLContext#client_ca= no array of X509::Certificate is returned but @@ -2468,6 +2468,8 @@ ossl_ssl_get_client_ca_list(VALUE self) GetSSL(self, ssl); ca = SSL_get_client_CA_list(ssl); + if (!ca) + return Qnil; return ossl_x509name_sk2ary(ca); } diff --git a/ext/openssl/ossl_x509crl.c b/ext/openssl/ossl_x509crl.c index 644d70a58..cfaf39640 100644 --- a/ext/openssl/ossl_x509crl.c +++ b/ext/openssl/ossl_x509crl.c @@ -276,21 +276,19 @@ ossl_x509crl_get_revoked(VALUE self) { X509_CRL *crl; int i, num; - X509_REVOKED *rev; - VALUE ary, revoked; + STACK_OF(X509_REVOKED) *sk; + VALUE ary; GetX509CRL(self, crl); - num = sk_X509_REVOKED_num(X509_CRL_get_REVOKED(crl)); - if (num < 0) { - OSSL_Debug("num < 0???"); - return rb_ary_new(); - } - ary = rb_ary_new2(num); + sk = X509_CRL_get_REVOKED(crl); + if (!sk) + return rb_ary_new(); + + num = sk_X509_REVOKED_num(sk); + ary = rb_ary_new_capa(num); for(i=0; i Date: Tue, 7 Jan 2025 02:14:50 +0900 Subject: [PATCH 3/4] x509: do not check for negative return from X509_*_get_ext_count() These functions wrap X509v3_get_ext_count(). The implementation can never return a negative number, and this behavior is documented in the man page. --- ext/openssl/ossl_x509cert.c | 5 +---- ext/openssl/ossl_x509crl.c | 6 +----- ext/openssl/ossl_x509revoked.c | 6 +----- 3 files changed, 3 insertions(+), 14 deletions(-) diff --git a/ext/openssl/ossl_x509cert.c b/ext/openssl/ossl_x509cert.c index 0505aac2a..62f4c5c24 100644 --- a/ext/openssl/ossl_x509cert.c +++ b/ext/openssl/ossl_x509cert.c @@ -619,10 +619,7 @@ ossl_x509_get_extensions(VALUE self) GetX509(self, x509); count = X509_get_ext_count(x509); - if (count < 0) { - return rb_ary_new(); - } - ary = rb_ary_new2(count); + ary = rb_ary_new_capa(count); for (i=0; i Date: Sun, 9 Feb 2025 19:42:54 +0900 Subject: [PATCH 4/4] x509name: do not check for negative return from X509_NAME_entry_count() The function never returns a negative number. --- ext/openssl/ossl_x509name.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/ext/openssl/ossl_x509name.c b/ext/openssl/ossl_x509name.c index 82ee0546a..f8373c81d 100644 --- a/ext/openssl/ossl_x509name.c +++ b/ext/openssl/ossl_x509name.c @@ -354,11 +354,7 @@ ossl_x509name_to_a(VALUE self) GetX509Name(self, name); entries = X509_NAME_entry_count(name); - if (entries < 0) { - OSSL_Debug("name entries < 0!"); - return rb_ary_new(); - } - ret = rb_ary_new2(entries); + ret = rb_ary_new_capa(entries); for (i=0; i