From 2f90c71036ed10ac58f5100aa12dee503b393112 Mon Sep 17 00:00:00 2001 From: Kazuki Yamaguchi Date: Fri, 15 Nov 2024 17:01:04 +0900 Subject: [PATCH] pkey: avoid creating multiple wrapper objects for single EVP_PKEY Currently, it is possible to create multiple OpenSSL::PKey::PKey instances that wrap the same EVP_PKEY object through ossl_pkey_wrap(). This behavior was not intentional and doesn't offer any useful functionality. As a result, the frozen state of an OpenSSL::PKey::PKey instance is meaningless. An upcoming change to make OpenSSL classes shareable between ractors relies on the assumption that frozen objects are thread-safe without the GVL. Let's keep track of the wrapper Ruby object associated with EVP_PKEY to ensure that only one Ruby object wraps a given EVP_PKEY. While other OpenSSL types have reference counters, EVP_PKEY is the only type in ruby/openssl where duplicate wrapper objects can be created. --- ext/openssl/ossl_pkey.c | 30 +++++++++++++++++++++++++++++- ext/openssl/ossl_pkey.h | 2 ++ ext/openssl/ossl_pkey_dh.c | 3 +++ ext/openssl/ossl_pkey_dsa.c | 3 +++ ext/openssl/ossl_pkey_ec.c | 11 ++++++++--- ext/openssl/ossl_pkey_rsa.c | 3 +++ test/openssl/test_x509cert.rb | 11 +++++++++++ 7 files changed, 59 insertions(+), 4 deletions(-) diff --git a/ext/openssl/ossl_pkey.c b/ext/openssl/ossl_pkey.c index e0f90b672..cd296f141 100644 --- a/ext/openssl/ossl_pkey.c +++ b/ext/openssl/ossl_pkey.c @@ -20,20 +20,38 @@ VALUE mPKey; VALUE cPKey; VALUE ePKeyError; static ID id_private_q; +int ossl_pkey_ex_ptr_idx; + +static void +ossl_evp_pkey_mark(void *ptr) +{ + VALUE obj = (VALUE)EVP_PKEY_get_ex_data(ptr, ossl_pkey_ex_ptr_idx); + rb_gc_mark_movable(obj); +} static void ossl_evp_pkey_free(void *ptr) { + EVP_PKEY_set_ex_data(ptr, ossl_pkey_ex_ptr_idx, NULL); EVP_PKEY_free(ptr); } +static void +ossl_evp_pkey_compact(void *ptr) +{ + VALUE obj = (VALUE)EVP_PKEY_get_ex_data(ptr, ossl_pkey_ex_ptr_idx); + EVP_PKEY_set_ex_data(ptr, ossl_pkey_ex_ptr_idx, (void *)rb_gc_location(obj)); +} + /* * Public */ const rb_data_type_t ossl_evp_pkey_type = { "OpenSSL/EVP_PKEY", { - 0, ossl_evp_pkey_free, + .dmark = ossl_evp_pkey_mark, + .dfree = ossl_evp_pkey_free, + .dcompact = ossl_evp_pkey_compact, }, 0, 0, RUBY_TYPED_FREE_IMMEDIATELY | RUBY_TYPED_WB_PROTECTED, }; @@ -61,6 +79,7 @@ pkey_wrap0(VALUE arg) } obj = rb_obj_alloc(klass); RTYPEDDATA_DATA(obj) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)obj); return obj; } @@ -70,6 +89,10 @@ ossl_pkey_wrap(EVP_PKEY *pkey) VALUE obj; int status; + obj = (VALUE)EVP_PKEY_get_ex_data(pkey, ossl_pkey_ex_ptr_idx); + if (obj) + return obj; + obj = rb_protect(pkey_wrap0, (VALUE)pkey, &status); if (status) { EVP_PKEY_free(pkey); @@ -630,6 +653,7 @@ ossl_pkey_initialize_copy(VALUE self, VALUE other) if (!pkey) ossl_raise(ePKeyError, "EVP_PKEY_dup"); RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); } return self; } @@ -1678,6 +1702,10 @@ Init_ossl_pkey(void) eOSSLError = rb_define_class_under(mOSSL, "OpenSSLError", rb_eStandardError); #endif + ossl_pkey_ex_ptr_idx = EVP_PKEY_get_ex_new_index(0, (void *)"ossl_pkey_ex_ptr_idx", 0, 0, 0); + if (ossl_pkey_ex_ptr_idx < 0) + ossl_raise(rb_eRuntimeError, "EVP_PKEY_get_ex_new_index"); + /* Document-module: OpenSSL::PKey * * == Asymmetric Public Key Algorithms diff --git a/ext/openssl/ossl_pkey.h b/ext/openssl/ossl_pkey.h index 0540a6604..6cbce60fc 100644 --- a/ext/openssl/ossl_pkey.h +++ b/ext/openssl/ossl_pkey.h @@ -14,6 +14,7 @@ extern VALUE mPKey; extern VALUE cPKey; extern VALUE ePKeyError; extern const rb_data_type_t ossl_evp_pkey_type; +extern int ossl_pkey_ex_ptr_idx; /* For ENGINE */ #define OSSL_PKEY_SET_PRIVATE(obj) rb_ivar_set((obj), rb_intern("private"), Qtrue) @@ -24,6 +25,7 @@ extern const rb_data_type_t ossl_evp_pkey_type; if (!(pkey)) { \ rb_raise(rb_eRuntimeError, "PKEY wasn't initialized!");\ } \ + RUBY_ASSERT(obj == (VALUE)EVP_PKEY_get_ex_data(pkey, ossl_pkey_ex_ptr_idx)); \ } while (0) /* Takes ownership of the EVP_PKEY */ diff --git a/ext/openssl/ossl_pkey_dh.c b/ext/openssl/ossl_pkey_dh.c index 00699b9b0..46ce2170c 100644 --- a/ext/openssl/ossl_pkey_dh.c +++ b/ext/openssl/ossl_pkey_dh.c @@ -113,6 +113,7 @@ ossl_dh_initialize(int argc, VALUE *argv, VALUE self) rb_raise(eDHError, "incorrect pkey type: %s", OBJ_nid2sn(type)); } RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); return self; legacy: @@ -124,6 +125,7 @@ ossl_dh_initialize(int argc, VALUE *argv, VALUE self) ossl_raise(eDHError, "EVP_PKEY_assign_DH"); } RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); return self; } @@ -164,6 +166,7 @@ ossl_dh_initialize_copy(VALUE self, VALUE other) ossl_raise(eDHError, "EVP_PKEY_assign_DH"); } RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); return self; } #endif diff --git a/ext/openssl/ossl_pkey_dsa.c b/ext/openssl/ossl_pkey_dsa.c index a7598d1e8..17266d133 100644 --- a/ext/openssl/ossl_pkey_dsa.c +++ b/ext/openssl/ossl_pkey_dsa.c @@ -125,6 +125,7 @@ ossl_dsa_initialize(int argc, VALUE *argv, VALUE self) rb_raise(eDSAError, "incorrect pkey type: %s", OBJ_nid2sn(type)); } RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); return self; legacy: @@ -136,6 +137,7 @@ ossl_dsa_initialize(int argc, VALUE *argv, VALUE self) ossl_raise(eDSAError, "EVP_PKEY_assign_DSA"); } RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); return self; } @@ -164,6 +166,7 @@ ossl_dsa_initialize_copy(VALUE self, VALUE other) ossl_raise(eDSAError, "EVP_PKEY_assign_DSA"); } RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); return self; } diff --git a/ext/openssl/ossl_pkey_ec.c b/ext/openssl/ossl_pkey_ec.c index 4b3a1fd0f..50128536e 100644 --- a/ext/openssl/ossl_pkey_ec.c +++ b/ext/openssl/ossl_pkey_ec.c @@ -111,6 +111,10 @@ ossl_ec_key_s_generate(VALUE klass, VALUE arg) obj = rb_obj_alloc(klass); ec = ec_key_new_from_group(arg); + if (!EC_KEY_generate_key(ec)) { + EC_KEY_free(ec); + ossl_raise(eECError, "EC_KEY_generate_key"); + } pkey = EVP_PKEY_new(); if (!pkey || EVP_PKEY_assign_EC_KEY(pkey, ec) != 1) { EVP_PKEY_free(pkey); @@ -118,9 +122,7 @@ ossl_ec_key_s_generate(VALUE klass, VALUE arg) ossl_raise(eECError, "EVP_PKEY_assign_EC_KEY"); } RTYPEDDATA_DATA(obj) = pkey; - - if (!EC_KEY_generate_key(ec)) - ossl_raise(eECError, "EC_KEY_generate_key"); + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)obj); return obj; } @@ -177,6 +179,7 @@ static VALUE ossl_ec_key_initialize(int argc, VALUE *argv, VALUE self) rb_raise(eDSAError, "incorrect pkey type: %s", OBJ_nid2sn(type)); } RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); return self; legacy: @@ -187,6 +190,7 @@ static VALUE ossl_ec_key_initialize(int argc, VALUE *argv, VALUE self) ossl_raise(eECError, "EVP_PKEY_assign_EC_KEY"); } RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); return self; } @@ -212,6 +216,7 @@ ossl_ec_key_initialize_copy(VALUE self, VALUE other) ossl_raise(eECError, "EVP_PKEY_assign_EC_KEY"); } RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); return self; } diff --git a/ext/openssl/ossl_pkey_rsa.c b/ext/openssl/ossl_pkey_rsa.c index 7d986989e..d278fd372 100644 --- a/ext/openssl/ossl_pkey_rsa.c +++ b/ext/openssl/ossl_pkey_rsa.c @@ -121,6 +121,7 @@ ossl_rsa_initialize(int argc, VALUE *argv, VALUE self) rb_raise(eRSAError, "incorrect pkey type: %s", OBJ_nid2sn(type)); } RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); return self; legacy: @@ -132,6 +133,7 @@ ossl_rsa_initialize(int argc, VALUE *argv, VALUE self) ossl_raise(eRSAError, "EVP_PKEY_assign_RSA"); } RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); return self; } @@ -159,6 +161,7 @@ ossl_rsa_initialize_copy(VALUE self, VALUE other) ossl_raise(eRSAError, "EVP_PKEY_assign_RSA"); } RTYPEDDATA_DATA(self) = pkey; + EVP_PKEY_set_ex_data(pkey, ossl_pkey_ex_ptr_idx, (void *)self); return self; } diff --git a/test/openssl/test_x509cert.rb b/test/openssl/test_x509cert.rb index 76359552e..81f383a71 100644 --- a/test/openssl/test_x509cert.rb +++ b/test/openssl/test_x509cert.rb @@ -42,6 +42,17 @@ def test_public_key } end + def test_public_key_instance + # Repeated calls of X509_get_pubkey() return the same EVP_PKEY object with + # increased reference count + cert = OpenSSL::X509::Certificate.new + cert.public_key = @rsa2048 + + p1 = cert.public_key + p2 = cert.public_key + assert_same(p1, p2) + end + def test_validity now = Time.at(Time.now.to_i + 0.9) cert = issue_cert(@ca, @rsa2048, 1, [], nil, nil,