From cc3f1af73e31f28927606c96a105c63b42bc05c1 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Thu, 31 Jul 2025 20:15:03 +0900 Subject: [PATCH 1/2] pkcs7: refactor error handling in PKCS7#add_data Raise an exception right after an OpenSSL function returns an error. Checking ERR_peek_error() is not reliable way to see if an error has occurred or not, as OpenSSL functions do not always populate the error queue. --- ext/openssl/ossl_pkcs7.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/ext/openssl/ossl_pkcs7.c b/ext/openssl/ossl_pkcs7.c index c53e512e8..b4e3b2ee1 100644 --- a/ext/openssl/ossl_pkcs7.c +++ b/ext/openssl/ossl_pkcs7.c @@ -838,30 +838,33 @@ ossl_pkcs7_add_data(VALUE self, VALUE data) PKCS7 *pkcs7; BIO *out, *in; char buf[4096]; - int len; + int len, ret; GetPKCS7(self, pkcs7); - if(PKCS7_type_is_signed(pkcs7)){ - if(!PKCS7_content_new(pkcs7, NID_pkcs7_data)) - ossl_raise(ePKCS7Error, NULL); + if (PKCS7_type_is_signed(pkcs7)) { + if (!PKCS7_content_new(pkcs7, NID_pkcs7_data)) + ossl_raise(ePKCS7Error, "PKCS7_content_new"); } in = ossl_obj2bio(&data); - if(!(out = PKCS7_dataInit(pkcs7, NULL))) goto err; - for(;;){ - if((len = BIO_read(in, buf, sizeof(buf))) <= 0) - break; - if(BIO_write(out, buf, len) != len) - goto err; + if (!(out = PKCS7_dataInit(pkcs7, NULL))) { + BIO_free(in); + ossl_raise(ePKCS7Error, "PKCS7_dataInit"); } - if(!PKCS7_dataFinal(pkcs7, out)) goto err; - ossl_pkcs7_set_data(self, Qnil); - - err: + for (;;) { + if ((len = BIO_read(in, buf, sizeof(buf))) <= 0) + break; + if (BIO_write(out, buf, len) != len) { + BIO_free_all(out); + BIO_free(in); + ossl_raise(ePKCS7Error, "BIO_write"); + } + } + ret = PKCS7_dataFinal(pkcs7, out); BIO_free_all(out); BIO_free(in); - if(ERR_peek_error()){ - ossl_raise(ePKCS7Error, NULL); - } + if (!ret) + ossl_raise(ePKCS7Error, "PKCS7_dataFinal"); + ossl_pkcs7_set_data(self, Qnil); return data; } From 9595ecf643d326fe9693b68cf8b7f28ab8f11913 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Thu, 31 Jul 2025 21:02:36 +0900 Subject: [PATCH 2/2] pkcs7: make PKCS7#add_recipient actually useful Add a simple test case that creates an enveloped-data structure without using the shorthand method, and fix two issues preventing this from working correctly. First, OpenSSL::PKey::PKCS7#add_recipient currently inserts an incomplete PKCS7_RECIP_INFO object into the PKCS7 object. When duplicating an unfinalized PKCS7_RECIP_INFO, the internal X509 reference must also be copied, as it is later used by #add_data to fill the rest. A similar issue with #add_signer was fixed in commit 20ca7a27a86e (pkcs7: keep private key when duplicating PKCS7_SIGNER_INFO, 2021-03-24). Second, #add_data calls PKCS7_dataFinal(), which for enveloped-data appears to require the BIO to be flushed explicitly with BIO_flush(). Without this, the last block of the encrypted data would be missing. --- ext/openssl/ossl_pkcs7.c | 21 +++++++++++++++++---- test/openssl/test_pkcs7.rb | 28 ++++++++++++++++++++++------ 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/ext/openssl/ossl_pkcs7.c b/ext/openssl/ossl_pkcs7.c index b4e3b2ee1..fa37f6d00 100644 --- a/ext/openssl/ossl_pkcs7.c +++ b/ext/openssl/ossl_pkcs7.c @@ -143,11 +143,19 @@ ossl_PKCS7_SIGNER_INFO_dup(PKCS7_SIGNER_INFO *si) } static PKCS7_RECIP_INFO * -ossl_PKCS7_RECIP_INFO_dup(PKCS7_RECIP_INFO *si) +ossl_PKCS7_RECIP_INFO_dup(PKCS7_RECIP_INFO *ri) { - return ASN1_dup((i2d_of_void *)i2d_PKCS7_RECIP_INFO, - (d2i_of_void *)d2i_PKCS7_RECIP_INFO, - si); + PKCS7_RECIP_INFO *ri_new = ASN1_dup((i2d_of_void *)i2d_PKCS7_RECIP_INFO, + (d2i_of_void *)d2i_PKCS7_RECIP_INFO, + ri); + if (ri_new && ri->cert) { + if (!X509_up_ref(ri->cert)) { + PKCS7_RECIP_INFO_free(ri_new); + return NULL; + } + ri_new->cert = ri->cert; + } + return ri_new; } static VALUE @@ -859,6 +867,11 @@ ossl_pkcs7_add_data(VALUE self, VALUE data) ossl_raise(ePKCS7Error, "BIO_write"); } } + if (BIO_flush(out) <= 0) { + BIO_free_all(out); + BIO_free(in); + ossl_raise(ePKCS7Error, "BIO_flush"); + } ret = PKCS7_dataFinal(pkcs7, out); BIO_free_all(out); BIO_free(in); diff --git a/test/openssl/test_pkcs7.rb b/test/openssl/test_pkcs7.rb index 5245ec1a6..8076be815 100644 --- a/test/openssl/test_pkcs7.rb +++ b/test/openssl/test_pkcs7.rb @@ -250,6 +250,28 @@ def test_enveloped } end + def test_enveloped_add_recipient + omit_on_fips # PKCS #1 v1.5 padding + + data = "aaaaa\nbbbbb\nccccc\n" + ktri_ee1 = OpenSSL::PKCS7::RecipientInfo.new(@ee1_cert) + ktri_ee2 = OpenSSL::PKCS7::RecipientInfo.new(@ee2_cert) + + tmp = OpenSSL::PKCS7.new + tmp.type = :enveloped + tmp.cipher = "AES-128-CBC" + tmp.add_recipient(ktri_ee1) + tmp.add_recipient(ktri_ee2) + tmp.add_data(data) + + p7 = OpenSSL::PKCS7.new(tmp.to_der) + assert_equal(:enveloped, p7.type) + assert_equal(data, p7.decrypt(@ee1_key, @ee1_cert)) + assert_equal(data, p7.decrypt(@ee2_key, @ee2_cert)) + assert_equal([@ee1_cert.serial, @ee2_cert.serial].sort, + p7.recipients.map(&:serial).sort) + end + def test_data asn1 = OpenSSL::ASN1::Sequence([ OpenSSL::ASN1::ObjectId("pkcs7-data"), @@ -317,12 +339,6 @@ def test_set_type_signed_and_enveloped assert_equal(:signedAndEnveloped, p7.type) end - def test_set_type_enveloped - p7 = OpenSSL::PKCS7.new - p7.type = "enveloped" - assert_equal(:enveloped, p7.type) - end - def test_set_type_encrypted p7 = OpenSSL::PKCS7.new p7.type = "encrypted"