Skip to content

Commit

Permalink
pkey: make OpenSSL::PKey::EC::Group wrap an EC_GROUP directly
Browse files Browse the repository at this point in the history 
As done for EC::Point, remove ossl_ec_group struct. This contains a
breaking change. Modifications to an EC::Group returned by EC#group
no longer affects the EC object unless set to the key explicitly using
EC#group=. This is the common behavior in Ruby/OpenSSL, including other
getter methods of EC such as EC#public_key.

EC#group currently returns a EC::Group linked with the key, i.e. the
EC::Group object holds a reference to an EC_GROUP that the EC_KEY owns.
We use some ugly workaround - the ossl_ec_group struct has a flag
'dont_free' that indicates we must not free the EC_GROUP. But it is
still not possible to control OpenSSL of free'ing the EC_GROUP, so,
for example, the following code behaves strangely:

  ec = OpenSSL::PKey::EC.generate("prime256v1")
  group = ec.group
  p group.curve_name #=> "prime256v1"
  ec.group = OpenSSL::PKey::EC::Group.new("prime256v1")
  p group.curve_name #=> nil
  • Loading branch information
@rhenium
rhenium committed Sep 7, 2016
1 parent 4076581 commit 9435c8b
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 107 deletions.
18 changes: 12 additions & 6 deletions History.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,19 @@ Notable changes
- OpenSSL::OCSP::BasicResponse#add_status accepts absolute times. They used to
accept only relative seconds from the current time.

* OpenSSL::PKey::EC follows the general PKey interface.
[[Bug #6567]](https://bugs.ruby-lang.org/issues/6567)
* OpenSSL::PKey

- OpenSSL::PKey::EC follows the general PKey interface.
[[Bug #6567]](https://bugs.ruby-lang.org/issues/6567)

- OpenSSL::PKey.read raises OpenSSL::PKey::PKeyError instead of ArgumentError
for consistency with OpenSSL::PKey::{DH,DSA,RSA,EC}#new.
[[Bug #11774]](https://bugs.ruby-lang.org/issues/11774),
[[GH ruby/openssl#55]](https://github.com/ruby/openssl/pull/55)

* OpenSSL::PKey.read raises OpenSSL::PKey::PKeyError instead of ArgumentError
for consistency with OpenSSL::PKey::{DH,DSA,RSA,EC}#new.
[[Bug #11774]](https://bugs.ruby-lang.org/issues/11774),
[[GH ruby/openssl#55]](https://github.com/ruby/openssl/pull/55)
- OpenSSL::PKey::EC::Group retrieved by OpenSSL::PKey::EC#group is no longer
linked with the EC key. Modifications to the EC::Group have no effect on the
key. [[GH ruby/openssl#71]](https://github.com/ruby/openssl/pull/71)

* OpenSSL::SSL

Expand Down
143 changes: 42 additions & 101 deletions ext/openssl/ossl_pkey_ec.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,6 @@

#if !defined(OPENSSL_NO_EC) && (OPENSSL_VERSION_NUMBER >= 0x0090802fL)

typedef struct {
EC_GROUP *group;
int dont_free;
} ossl_ec_group;


#define EXPORT_PEM 0
#define EXPORT_DER 1

Expand All @@ -34,15 +28,9 @@ static const rb_data_type_t ossl_ec_point_type;
GetEC(obj, key); \
} while (0)

#define SafeGet_ec_group(obj, group) do { \
OSSL_Check_Kind((obj), cEC_GROUP); \
TypedData_Get_Struct((obj), ossl_ec_group, &ossl_ec_group_type, (group)); \
} while(0)
#define GetECGroup(obj, g) do { \
ossl_ec_group *ec_group; \
TypedData_Get_Struct((obj), ossl_ec_group, &ossl_ec_group_type, ec_group); \
(g) = ec_group->group; \
if ((g) == NULL) \
#define GetECGroup(obj, group) do { \
TypedData_Get_Struct(obj, EC_GROUP, &ossl_ec_group_type, group); \
if ((group) == NULL) \
ossl_raise(eEC_GROUP, "EC_GROUP is not initialized"); \
} while (0)
#define SafeGetECGroup(obj, group) do { \
Expand Down Expand Up @@ -82,7 +70,7 @@ static ID ID_uncompressed;
static ID ID_compressed;
static ID ID_hybrid;

static ID id_i_group, id_i_key;
static ID id_i_group;

static VALUE ec_group_new(const EC_GROUP *group);
static VALUE ec_point_new(const EC_POINT *point, const EC_GROUP *group);
Expand Down Expand Up @@ -257,8 +245,6 @@ static VALUE ossl_ec_key_initialize(int argc, VALUE *argv, VALUE self)
ossl_raise(eECError, "EVP_PKEY_assign_EC_KEY");
}

rb_ivar_set(self, id_i_group, Qnil);

return self;
}

Expand All @@ -280,81 +266,47 @@ ossl_ec_key_initialize_copy(VALUE self, VALUE other)
EC_KEY_free(ec_new);
ossl_raise(eECError, "EVP_PKEY_assign_EC_KEY");
}
rb_ivar_set(self, id_i_group, Qnil); /* EC_KEY_dup() also copies the EC_GROUP */

return self;
}

/*
* call-seq:
* key.group => group
* call-seq:
* key.group => group
*
* Returns a constant <code>OpenSSL::EC::Group</code> that is tied to the key.
* Modifying the returned group can make the key invalid.
* Returns the EC::Group that the key is associated with. Modifying the returned
* group does not affect +key+.
*/
static VALUE ossl_ec_key_get_group(VALUE self)
static VALUE
ossl_ec_key_get_group(VALUE self)
{
VALUE group_v;
EC_KEY *ec;
ossl_ec_group *ec_group;
EC_GROUP *group;
const EC_GROUP *group;

GetEC(self, ec);
group = EC_KEY_get0_group(ec);
if (!group)
return Qnil;

group_v = rb_attr_get(self, id_i_group);
if (!NIL_P(group_v))
return group_v;

if ((group = (EC_GROUP *)EC_KEY_get0_group(ec)) != NULL) {
group_v = rb_obj_alloc(cEC_GROUP);
SafeGet_ec_group(group_v, ec_group);
ec_group->group = group;
ec_group->dont_free = 1;
rb_ivar_set(group_v, id_i_key, self);
rb_ivar_set(self, id_i_group, group_v);
return group_v;
}

return Qnil;
return ec_group_new(group);
}

/*
* call-seq:
* key.group = group => group
*
* Returns the same object passed, not the group object associated with the key.
* If you wish to access the group object tied to the key call key.group after setting
* the group.
*
* Setting the group will immediately destroy any previously assigned group object.
* The group is internally copied by OpenSSL. Modifying the original group after
* assignment will not effect the internal key structure.
* (your changes may be lost). BE CAREFUL.
* call-seq:
* key.group = group
*
* EC_KEY_set_group calls EC_GROUP_free(key->group) then EC_GROUP_dup(), not EC_GROUP_copy.
* This documentation is accurate for OpenSSL 0.9.8b.
* Sets the EC::Group for the key. The group structure is internally copied so
* modifition to +group+ after assigning to a key has no effect on the key.
*/
static VALUE ossl_ec_key_set_group(VALUE self, VALUE group_v)
static VALUE
ossl_ec_key_set_group(VALUE self, VALUE group_v)
{
VALUE old_group_v;
EC_KEY *ec;
EC_GROUP *group;

GetEC(self, ec);
SafeGetECGroup(group_v, group);

old_group_v = rb_attr_get(self, id_i_group);
if (!NIL_P(old_group_v)) {
ossl_ec_group *old_ec_group;
SafeGet_ec_group(old_group_v, old_ec_group);

old_ec_group->group = NULL;
old_ec_group->dont_free = 0;
rb_ivar_set(old_group_v, id_i_key, Qnil);
}

rb_ivar_set(self, id_i_group, Qnil);

if (EC_KEY_set_group(ec, group) != 1)
ossl_raise(eECError, "EC_KEY_set_group");

Expand Down Expand Up @@ -731,10 +683,7 @@ static VALUE ossl_ec_key_dsa_verify_asn1(VALUE self, VALUE data, VALUE sig)
static void
ossl_ec_group_free(void *ptr)
{
ossl_ec_group *ec_group = ptr;
if (!ec_group->dont_free && ec_group->group)
EC_GROUP_clear_free(ec_group->group);
ruby_xfree(ec_group);
EC_GROUP_clear_free(ptr);
}

static const rb_data_type_t ossl_ec_group_type = {
Expand All @@ -745,26 +694,23 @@ static const rb_data_type_t ossl_ec_group_type = {
0, 0, RUBY_TYPED_FREE_IMMEDIATELY,
};

static VALUE ossl_ec_group_alloc(VALUE klass)
static VALUE
ossl_ec_group_alloc(VALUE klass)
{
ossl_ec_group *ec_group;
VALUE obj;

obj = TypedData_Make_Struct(klass, ossl_ec_group, &ossl_ec_group_type, ec_group);

return obj;
return TypedData_Wrap_Struct(klass, &ossl_ec_group_type, NULL);
}

static VALUE
ec_group_new(const EC_GROUP *group)
{
ossl_ec_group *ec_group;
VALUE obj;
EC_GROUP *group_new;

obj = TypedData_Make_Struct(cEC_GROUP, ossl_ec_group, &ossl_ec_group_type, ec_group);
ec_group->group = EC_GROUP_dup(group);
if (!ec_group->group)
obj = ossl_ec_group_alloc(cEC_GROUP);
group_new = EC_GROUP_dup(group);
if (!group_new)
ossl_raise(eEC_GROUP, "EC_GROUP_dup");
RTYPEDDATA_DATA(obj) = group_new;

return obj;
}
Expand Down Expand Up @@ -793,11 +739,10 @@ ec_group_new(const EC_GROUP *group)
static VALUE ossl_ec_group_initialize(int argc, VALUE *argv, VALUE self)
{
VALUE arg1, arg2, arg3, arg4;
ossl_ec_group *ec_group;
EC_GROUP *group = NULL;
EC_GROUP *group;

TypedData_Get_Struct(self, ossl_ec_group, &ossl_ec_group_type, ec_group);
if (ec_group->group != NULL)
TypedData_Get_Struct(self, EC_GROUP, &ossl_ec_group_type, group);
if (group)
ossl_raise(rb_eRuntimeError, "EC_GROUP is already initialized");

switch (rb_scan_args(argc, argv, "13", &arg1, &arg2, &arg3, &arg4)) {
Expand Down Expand Up @@ -890,28 +835,25 @@ static VALUE ossl_ec_group_initialize(int argc, VALUE *argv, VALUE self)

if (group == NULL)
ossl_raise(eEC_GROUP, "");

ec_group->group = group;
RTYPEDDATA_DATA(self) = group;

return self;
}

static VALUE
ossl_ec_group_initialize_copy(VALUE self, VALUE other)
{
ossl_ec_group *ec_group;
EC_GROUP *orig;
EC_GROUP *group, *group_new;

TypedData_Get_Struct(self, ossl_ec_group, &ossl_ec_group_type, ec_group);
if (ec_group->group)
TypedData_Get_Struct(self, EC_GROUP, &ossl_ec_group_type, group_new);
if (group_new)
ossl_raise(eEC_GROUP, "EC::Group already initialized");
SafeGetECGroup(other, orig);
SafeGetECGroup(other, group);

ec_group->group = EC_GROUP_dup(orig);
if (!ec_group->group)
group_new = EC_GROUP_dup(group);
if (!group_new)
ossl_raise(eEC_GROUP, "EC_GROUP_dup");

rb_ivar_set(self, id_i_key, Qnil);
RTYPEDDATA_DATA(self) = group_new;

return self;
}
Expand Down Expand Up @@ -1373,7 +1315,7 @@ ec_point_new(const EC_POINT *point, const EC_GROUP *group)
EC_POINT *point_new;
VALUE obj;

obj = TypedData_Wrap_Struct(cEC_POINT, &ossl_ec_point_type, NULL);
obj = ossl_ec_point_alloc(cEC_POINT);
point_new = EC_POINT_dup(point, group);
if (!point_new)
ossl_raise(eEC_POINT, "EC_POINT_dup");
Expand Down Expand Up @@ -1855,7 +1797,6 @@ void Init_ossl_ec(void)
rb_define_method(cEC_POINT, "mul", ossl_ec_point_mul, -1);

id_i_group = rb_intern("@group");
id_i_key = rb_intern("@key");
}

#else /* defined NO_EC */
Expand Down

0 comments on commit 9435c8b

Please sign in to comment.