Skip to content

Commit

Permalink
pkey: avoid creating multiple wrapper objects for single EVP_PKEY
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rhenium committed Nov 15, 2024
1 parent 62261f9 commit 2f90c71
Show file tree
Hide file tree
Showing 7 changed files with 59 additions and 4 deletions.
30 changes: 29 additions & 1 deletion ext/openssl/ossl_pkey.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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;
}

Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions ext/openssl/ossl_pkey.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 */
Expand Down
3 changes: 3 additions & 0 deletions ext/openssl/ossl_pkey_dh.c
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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;
}

Expand Down Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions ext/openssl/ossl_pkey_dsa.c
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand Down
11 changes: 8 additions & 3 deletions ext/openssl/ossl_pkey_ec.c
Original file line number Diff line number Diff line change
Expand Up @@ -111,16 +111,18 @@ 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);
EC_KEY_free(ec);
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;
}
Expand Down Expand Up @@ -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:
Expand All @@ -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;
}

Expand All @@ -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;
}
Expand Down
3 changes: 3 additions & 0 deletions ext/openssl/ossl_pkey_rsa.c
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand Down
11 changes: 11 additions & 0 deletions test/openssl/test_x509cert.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 2f90c71

Please sign in to comment.