[ruby/openssl] pkcs7: keep private key when duplicating PKCS7_SIGNER_INFO

ASN1_dup() will not copy the 'pkey' field of a PKCS7_SIGNER_INFO object
by design; it is a temporary field kept until the PKCS7 structure is
finalized. Let's bump reference counter of the pkey in the original
object and use it in the new object, too.

This commit also removes PKCS7#add_signer's routine to add the
content-type attribute as a signed attribute automatically. This
behavior was not documented or tested. This change should not break any
working user code since the method was completely useless without the
change above.

https://github.com/ruby/openssl/commit/20ca7a27a8

2 files changed, 51 insertions, 48 deletions

diff --git a/ext/openssl/ossl_pkcs7.c b/ext/openssl/ossl_pkcs7.c
index ea8e92d1bc..0bcc76a9fd 100644
--- a/ext/openssl/ossl_pkcs7.c
+++ b/ext/openssl/ossl_pkcs7.c
@@ -101,19 +101,24 @@ static const rb_data_type_t ossl_pkcs7_recip_info_type = {
  * (MADE PRIVATE UNTIL SOMEBODY WILL NEED THEM)
  */
 static PKCS7_SIGNER_INFO *
-ossl_PKCS7_SIGNER_INFO_dup(const PKCS7_SIGNER_INFO *si)
+ossl_PKCS7_SIGNER_INFO_dup(PKCS7_SIGNER_INFO *si)
 {
-    return (PKCS7_SIGNER_INFO *)ASN1_dup((i2d_of_void *)i2d_PKCS7_SIGNER_INFO,
-					 (d2i_of_void *)d2i_PKCS7_SIGNER_INFO,
-					 (char *)si);
+    PKCS7_SIGNER_INFO *si_new = ASN1_dup((i2d_of_void *)i2d_PKCS7_SIGNER_INFO,
+                                         (d2i_of_void *)d2i_PKCS7_SIGNER_INFO,
+                                         si);
+    if (si_new && si->pkey) {
+        EVP_PKEY_up_ref(si->pkey);
+        si_new->pkey = si->pkey;
+    }
+    return si_new;
 }
 
 static PKCS7_RECIP_INFO *
-ossl_PKCS7_RECIP_INFO_dup(const PKCS7_RECIP_INFO *si)
+ossl_PKCS7_RECIP_INFO_dup(PKCS7_RECIP_INFO *si)
 {
-    return (PKCS7_RECIP_INFO *)ASN1_dup((i2d_of_void *)i2d_PKCS7_RECIP_INFO,
-					(d2i_of_void *)d2i_PKCS7_RECIP_INFO,
-					(char *)si);
+    return ASN1_dup((i2d_of_void *)i2d_PKCS7_RECIP_INFO,
+                    (d2i_of_void *)d2i_PKCS7_RECIP_INFO,
+                    si);
 }
 
 static VALUE
@@ -130,19 +135,6 @@ ossl_pkcs7si_new(PKCS7_SIGNER_INFO *p7si)
     return obj;
 }
 
-static PKCS7_SIGNER_INFO *
-DupPKCS7SignerPtr(VALUE obj)
-{
-    PKCS7_SIGNER_INFO *p7si, *pkcs7;
-
-    GetPKCS7si(obj, p7si);
-    if (!(pkcs7 = ossl_PKCS7_SIGNER_INFO_dup(p7si))) {
-	ossl_raise(ePKCS7Error, NULL);
-    }
-
-    return pkcs7;
-}
-
 static VALUE
 ossl_pkcs7ri_new(PKCS7_RECIP_INFO *p7ri)
 {
@@ -157,19 +149,6 @@ ossl_pkcs7ri_new(PKCS7_RECIP_INFO *p7ri)
     return obj;
 }
 
-static PKCS7_RECIP_INFO *
-DupPKCS7RecipientPtr(VALUE obj)
-{
-    PKCS7_RECIP_INFO *p7ri, *pkcs7;
-
-    GetPKCS7ri(obj, p7ri);
-    if (!(pkcs7 = ossl_PKCS7_RECIP_INFO_dup(p7ri))) {
-	ossl_raise(ePKCS7Error, NULL);
-    }
-
-    return pkcs7;
-}
-
 /*
  * call-seq:
  *    PKCS7.read_smime(string) => pkcs7
@@ -521,17 +500,18 @@ static VALUE
 ossl_pkcs7_add_signer(VALUE self, VALUE signer)
 {
     PKCS7 *pkcs7;
-    PKCS7_SIGNER_INFO *p7si;
+    PKCS7_SIGNER_INFO *si, *si_new;
 
-    p7si = DupPKCS7SignerPtr(signer); /* NEED TO DUP */
     GetPKCS7(self, pkcs7);
-    if (!PKCS7_add_signer(pkcs7, p7si)) {
-	PKCS7_SIGNER_INFO_free(p7si);
-	ossl_raise(ePKCS7Error, "Could not add signer.");
-    }
-    if (PKCS7_type_is_signed(pkcs7)){
-	PKCS7_add_signed_attribute(p7si, NID_pkcs9_contentType,
-				   V_ASN1_OBJECT, OBJ_nid2obj(NID_pkcs7_data));
+    GetPKCS7si(signer, si);
+
+    si_new = ossl_PKCS7_SIGNER_INFO_dup(si);
+    if (!si_new)
+        ossl_raise(ePKCS7Error, "PKCS7_SIGNER_INFO_dup");
+
+    if (PKCS7_add_signer(pkcs7, si_new) != 1) {
+        PKCS7_SIGNER_INFO_free(si_new);
+        ossl_raise(ePKCS7Error, "PKCS7_add_signer");
     }
 
     return self;
@@ -567,13 +547,18 @@ static VALUE
 ossl_pkcs7_add_recipient(VALUE self, VALUE recip)
 {
     PKCS7 *pkcs7;
-    PKCS7_RECIP_INFO *ri;
+    PKCS7_RECIP_INFO *ri, *ri_new;
 
-    ri = DupPKCS7RecipientPtr(recip); /* NEED TO DUP */
     GetPKCS7(self, pkcs7);
-    if (!PKCS7_add_recipient_info(pkcs7, ri)) {
-	PKCS7_RECIP_INFO_free(ri);
-	ossl_raise(ePKCS7Error, "Could not add recipient.");
+    GetPKCS7ri(recip, ri);
+
+    ri_new = ossl_PKCS7_RECIP_INFO_dup(ri);
+    if (!ri_new)
+        ossl_raise(ePKCS7Error, "PKCS7_RECIP_INFO_dup");
+
+    if (PKCS7_add_recipient_info(pkcs7, ri_new) != 1) {
+        PKCS7_RECIP_INFO_free(ri_new);
+        ossl_raise(ePKCS7Error, "PKCS7_add_recipient_info");
     }
 
     return self;
diff --git a/test/openssl/test_pkcs7.rb b/test/openssl/test_pkcs7.rb
index d0d9dcaf81..ba8b93d034 100644
--- a/test/openssl/test_pkcs7.rb
+++ b/test/openssl/test_pkcs7.rb
@@ -89,6 +89,24 @@ class OpenSSL::TestPKCS7 < OpenSSL::TestCase
     assert_equal(@ee2_cert.issuer.to_s, signers[1].issuer.to_s)
   end
 
+  def test_signed_add_signer
+    data = "aaaaa\nbbbbb\nccccc\n"
+    psi = OpenSSL::PKCS7::SignerInfo.new(@ee1_cert, @rsa1024, "sha256")
+    p7 = OpenSSL::PKCS7.new
+    p7.type = :signed
+    p7.add_signer(psi)
+    p7.add_certificate(@ee1_cert)
+    p7.add_certificate(@ca_cert)
+    p7.add_data(data)
+
+    store = OpenSSL::X509::Store.new
+    store.add_cert(@ca_cert)
+
+    assert_equal(true, p7.verify([], store))
+    assert_equal(true, OpenSSL::PKCS7.new(p7.to_der).verify([], store))
+    assert_equal(1, p7.signers.size)
+  end
+
   def test_detached_sign
     store = OpenSSL::X509::Store.new
     store.add_cert(@ca_cert)