Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provides get/set for affine coordinates on OpenSSL::PKey::EC::Point #435

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
+262 −26
Open

Provides get/set for affine coordinates on OpenSSL::PKey::EC::Point #435

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2f73ffb
Normalize symbol names and affine check
Apr 3, 2021
50cfdcc
OpenSSL::PKey::EC::Point#dup
Apr 3, 2021
0899f13
OpenSSL::PKey::EC::Point#affine_coords
Apr 3, 2021
f7e6f64
Expose OpenSSL::BN coercion
Apr 3, 2021
ad43592
OpenSSL::PKey::EC::Point#affine_coords=
Apr 3, 2021
e40ad97
Ensure no symbol conflict ossl_try_convert_to_bn
Apr 3, 2021
22556fe
OpenSSL::BN#even?
Apr 4, 2021
86b9ced
OpenSSL::PKey::EC::Point#set_compressed_coords
Apr 4, 2021
b0eb019
OpenSSL::BN#dup
Apr 4, 2021
1a34823
OpenSSL::BN#positive?
Apr 4, 2021
872fc73
Fixed array initialize
rickmark Apr 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ext/openssl/extconf.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,11 @@ def find_openssl_library
have_func("EVP_PBE_scrypt")
have_func("SSL_CTX_set_post_handshake_auth")

# added in 1.1.1
have_func("EC_POINT_get_affine_coordinates")
have_func("EC_POINT_set_affine_coordinates")
have_func("EC_POINT_set_compressed_coordinates")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have an alternate implementation in ext/openssl/openssl_missing.c using the suffixed variants? EC_POINT_get_affine_coordinates() only exists in OpenSSL 1.1.1, and the latest LibreSSL still doesn't have it.

Checking only EC_POINT_get_affine_coordinates should be good enough since it's unlikely a future LibreSSL version implements just one or two of the three functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"alternate implementation" as in using the GFp and GF2m functions, or my nonsense encoding kludge?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant using the GFp and GF2m functions. They should both exist in OpenSSL <= 1.1.0 and LibreSSL.


Logging::message "=== Checking done. ===\n"

create_header
Expand Down
50 changes: 42 additions & 8 deletions ext/openssl/ossl_bn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,7 @@ integer_to_bnptr(VALUE obj, BIGNUM *orig)
return bn;
}

static VALUE
try_convert_to_bn(VALUE obj)
VALUE ossl_try_convert_to_bn(VALUE obj)
{
BIGNUM *bn;
VALUE newobj = Qnil;
Expand All @@ -142,7 +141,7 @@ ossl_bn_value_ptr(volatile VALUE *ptr)
VALUE tmp;
BIGNUM *bn;

tmp = try_convert_to_bn(*ptr);
tmp = ossl_try_convert_to_bn(*ptr);
if (NIL_P(tmp))
ossl_raise(rb_eTypeError, "Cannot convert into OpenSSL::BN");
GetBN(tmp, bn);
Expand Down Expand Up @@ -449,6 +448,22 @@ BIGNUM_BOOL1(is_one)
*/
BIGNUM_BOOL1(is_odd)

static VALUE
ossl_bn_is_even(VALUE self)
{
VALUE is_odd;

is_odd = ossl_bn_is_odd(self);
if (is_odd == Qtrue) {
return Qfalse;
}
else if (is_odd == Qfalse) {
return Qtrue;
}

rb_raise(eBNError, "ossl_bn_is_odd didn't return a boolean");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know this is unreachable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just defensive coding 🙃

}

/*
* call-seq:
* bn.negative? => true | false
Expand All @@ -464,6 +479,21 @@ ossl_bn_is_negative(VALUE self)
return BN_is_negative(bn) ? Qtrue : Qfalse;
}

/*
* call-seq:
* bn.positive? => true | false
*/
static VALUE
ossl_bn_is_positive(VALUE self)
{
BIGNUM *bn;

GetBN(self, bn);
if (BN_is_zero(bn))
return Qfalse;
return BN_is_negative(bn) ? Qfalse : Qtrue;
}

#define BIGNUM_1c(func) \
static VALUE \
ossl_bn_##func(VALUE self) \
Expand Down Expand Up @@ -953,10 +983,11 @@ ossl_bn_copy(VALUE self, VALUE other)

/*
* call-seq:
* +bn -> aBN
* bn.dup -> aBN
* +bn -> aBN
*/
static VALUE
ossl_bn_uplus(VALUE self)
ossl_bn_dup(VALUE self)
{
VALUE obj;
BIGNUM *bn1, *bn2;
Expand Down Expand Up @@ -1006,7 +1037,7 @@ ossl_bn_abs(VALUE self)
return ossl_bn_uminus(self);
}
else {
return ossl_bn_uplus(self);
return ossl_bn_dup(self);
}
}

Expand Down Expand Up @@ -1051,7 +1082,7 @@ ossl_bn_eq(VALUE self, VALUE other)
BIGNUM *bn1, *bn2;

GetBN(self, bn1);
other = try_convert_to_bn(other);
other = ossl_try_convert_to_bn(other);
if (NIL_P(other))
return Qfalse;
GetBN(other, bn2);
Expand Down Expand Up @@ -1217,14 +1248,15 @@ Init_ossl_bn(void)

rb_define_method(cBN, "initialize_copy", ossl_bn_copy, 1);
rb_define_method(cBN, "copy", ossl_bn_copy, 1);
rb_define_method(cBN, "dup", ossl_bn_dup, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BN implements #initialize_copy. The #dup method defined by Object should already work without this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will resolve with a comment to that effect so others know as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we alias +@ to #dup then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current state (having #+@ as a normal method defined by ossl_bn_uplus()) is fine.


/* swap (=coerce?) */

rb_define_method(cBN, "num_bytes", ossl_bn_num_bytes, 0);
rb_define_method(cBN, "num_bits", ossl_bn_num_bits, 0);
/* num_bits_word */

rb_define_method(cBN, "+@", ossl_bn_uplus, 0);
rb_define_alias(cBN, "+@", "dup");
rb_define_method(cBN, "-@", ossl_bn_uminus, 0);
rb_define_method(cBN, "abs", ossl_bn_abs, 0);

Expand Down Expand Up @@ -1261,7 +1293,9 @@ Init_ossl_bn(void)
rb_define_method(cBN, "one?", ossl_bn_is_one, 0);
/* is_word */
rb_define_method(cBN, "odd?", ossl_bn_is_odd, 0);
rb_define_method(cBN, "even?", ossl_bn_is_even, 0);
rb_define_method(cBN, "negative?", ossl_bn_is_negative, 0);
rb_define_method(cBN, "positive?", ossl_bn_is_positive, 0);

/* zero
* one
Expand Down
1 change: 1 addition & 0 deletions ext/openssl/ossl_bn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ BN_CTX *ossl_bn_ctx_get(void);
#define GetBNPtr(obj) ossl_bn_value_ptr(&(obj))

VALUE ossl_bn_new(const BIGNUM *);
VALUE ossl_try_convert_to_bn(VALUE obj);
BIGNUM *ossl_bn_value_ptr(volatile VALUE *);
void Init_ossl_bn(void);

Expand Down
175 changes: 157 additions & 18 deletions ext/openssl/ossl_pkey_ec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,13 @@ VALUE eEC_GROUP;
VALUE cEC_POINT;
VALUE eEC_POINT;

static ID s_GFp, s_GF2m;
static ID id_GFp, id_GF2m;

static ID ID_uncompressed;
static ID ID_compressed;
static ID ID_hybrid;
static ID id_odd, id_even;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not used.


static ID id_uncompressed;
static ID id_compressed;
static ID id_hybrid;

static ID id_i_group;

Expand Down Expand Up @@ -637,10 +639,10 @@ static VALUE ossl_ec_group_initialize(int argc, VALUE *argv, VALUE self)
const BIGNUM *a = GetBNPtr(arg3);
const BIGNUM *b = GetBNPtr(arg4);

if (id == s_GFp) {
if (id == id_GFp) {
new_curve = EC_GROUP_new_curve_GFp;
#if !defined(OPENSSL_NO_EC2M)
} else if (id == s_GF2m) {
} else if (id == id_GF2m) {
new_curve = EC_GROUP_new_curve_GF2m;
#endif
} else {
Expand Down Expand Up @@ -922,9 +924,9 @@ static VALUE ossl_ec_group_get_point_conversion_form(VALUE self)
form = EC_GROUP_get_point_conversion_form(group);

switch (form) {
case POINT_CONVERSION_UNCOMPRESSED: ret = ID_uncompressed; break;
case POINT_CONVERSION_COMPRESSED: ret = ID_compressed; break;
case POINT_CONVERSION_HYBRID: ret = ID_hybrid; break;
case POINT_CONVERSION_UNCOMPRESSED: ret = id_uncompressed; break;
case POINT_CONVERSION_COMPRESSED: ret = id_compressed; break;
case POINT_CONVERSION_HYBRID: ret = id_hybrid; break;
default: ossl_raise(eEC_GROUP, "unsupported point conversion form: %d, this module should be updated", form);
}

Expand All @@ -936,11 +938,11 @@ parse_point_conversion_form_symbol(VALUE sym)
{
ID id = SYM2ID(sym);

if (id == ID_uncompressed)
if (id == id_uncompressed)
return POINT_CONVERSION_UNCOMPRESSED;
else if (id == ID_compressed)
else if (id == id_compressed)
return POINT_CONVERSION_COMPRESSED;
else if (id == ID_hybrid)
else if (id == id_hybrid)
return POINT_CONVERSION_HYBRID;
else
ossl_raise(rb_eArgError, "unsupported point conversion form %+"PRIsVALUE
Expand Down Expand Up @@ -1243,6 +1245,14 @@ ossl_ec_point_initialize_copy(VALUE self, VALUE other)
return self;
}

static VALUE
ossl_ec_point_dup(VALUE self)
{
VALUE other;
other = ossl_ec_point_alloc(eEC_POINT);
return ossl_ec_point_initialize_copy(other, self);
}

/*
* call-seq:
* point1.eql?(point2) => true | false
Expand Down Expand Up @@ -1364,6 +1374,120 @@ static VALUE ossl_ec_point_set_to_infinity(VALUE self)
return self;
}

#ifdef HAVE_EC_POINT_GET_AFFINE_COORDINATES

/*
* call-seq:
* point.affine_coords => [ xBN, yBN ]
*/
static VALUE
ossl_ec_point_get_affine_coords(VALUE self)
{
VALUE x, y, result;
EC_POINT *point;
BIGNUM *xBN, *yBN;
const EC_GROUP *group;

GetECPoint(self, point);
GetECPointGroup(self, group);

if (point == NULL || group == NULL) {
rb_raise(eEC_POINT, "unable to get point and group");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

point and group are never NULL. GetECPoint() and GetECPointGroup() must be checking it.


x = ossl_bn_new(NULL);
y = ossl_bn_new(NULL);

xBN = GetBNPtr(x);
yBN = GetBNPtr(y);

if (!EC_POINT_get_affine_coordinates(group, point, xBN, yBN, NULL)) {
ossl_raise(eEC_POINT, "EC_POINT_get_affine_coordinates");
}

result = rb_ary_new2(2);
rb_ary_push(result, x);
rb_ary_push(result, y);
rickmark marked this conversation as resolved.
Show resolved Hide resolved

return result;
}
#endif

#ifdef HAVE_EC_POINT_SET_AFFINE_COORDINATES
/*
* call-seq:
* point.affine_coords = [ xBN, yBN ]
*/
static VALUE
ossl_ec_point_set_affine_coords(VALUE self, VALUE coords)
{
VALUE x, y;
EC_POINT *point;
BIGNUM *xBN, *yBN;
const EC_GROUP *group;

GetECPoint(self, point);
GetECPointGroup(self, group);

if (point == NULL || group == NULL) {
rb_raise(eEC_POINT, "unable to get point and group");
}

if (TYPE(coords) != T_ARRAY || RARRAY_LEN(coords) != 2) {
rb_raise(eEC_POINT, "argument must be an array of [ x, y ]");
}

x = ossl_try_convert_to_bn(RARRAY_AREF(coords, 0));
y = ossl_try_convert_to_bn(RARRAY_AREF(coords, 1));

xBN = GetBNPtr(x);
yBN = GetBNPtr(y);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetBNPtr() uses try_convert_to_bn() internally. So, simply x = RARRAY_AREF(coords, 0); followed by xBN = GetBNPtr(x) should be enough.

The value from the array still needs to be stored into a separate VALUE variable (x, y) once, though.


if (!EC_POINT_set_affine_coordinates(group, point, xBN, yBN, NULL)) {
ossl_raise(eEC_POINT, "EC_POINT_set_affine_coordinates");
}

return self;
}
#endif

#ifdef HAVE_EC_POINT_SET_COMPRESSED_COORDINATES
/*
* call-seq:
* point.set_compressed_coords x, y_bit
*/
static VALUE
ossl_ec_point_set_compressed_coords(VALUE self, VALUE x, VALUE y_bit)
{
EC_POINT *point;
BIGNUM *xBN;
const EC_GROUP *group;
int y;

GetECPoint(self, point);
GetECPointGroup(self, group);

if (point == NULL || group == NULL) {
rb_raise(eEC_POINT, "unable to get point and group");
}

if (RB_INTEGER_TYPE_P(y_bit)) {
y = NUM2INT(y_bit);
} else {
rb_raise(eEC_POINT, "y_bit must be Integer");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to explicitly check here, NUM2INT() will also do a type check (and calls #to_int if necessary).


x = ossl_try_convert_to_bn(x);
xBN = GetBNPtr(x);

if (!EC_POINT_set_compressed_coordinates(group, point, xBN, y, NULL)) {
ossl_raise(eEC_POINT, "EC_POINT_set_affine_coordinates");
}

return self;
}
#endif

/*
* call-seq:
* point.to_octet_string(conversion_form) -> String
Expand Down Expand Up @@ -1554,12 +1678,15 @@ void Init_ossl_ec(void)
eEC_GROUP = rb_define_class_under(cEC_GROUP, "Error", eOSSLError);
eEC_POINT = rb_define_class_under(cEC_POINT, "Error", eOSSLError);

s_GFp = rb_intern("GFp");
s_GF2m = rb_intern("GF2m");
id_GFp = rb_intern("GFp");
id_GF2m = rb_intern("GF2m");

ID_uncompressed = rb_intern("uncompressed");
ID_compressed = rb_intern("compressed");
ID_hybrid = rb_intern("hybrid");
id_even = rb_intern("even");
id_odd = rb_intern("odd");

id_uncompressed = rb_intern("uncompressed");
id_compressed = rb_intern("compressed");
id_hybrid = rb_intern("hybrid");

rb_define_const(cEC, "NAMED_CURVE", INT2NUM(OPENSSL_EC_NAMED_CURVE));
#if defined(OPENSSL_EC_EXPLICIT_CURVE)
Expand Down Expand Up @@ -1643,7 +1770,10 @@ void Init_ossl_ec(void)
rb_define_alloc_func(cEC_POINT, ossl_ec_point_alloc);
rb_define_method(cEC_POINT, "initialize", ossl_ec_point_initialize, -1);
rb_define_method(cEC_POINT, "initialize_copy", ossl_ec_point_initialize_copy, 1);
rb_define_method(cEC_POINT, "dup", ossl_ec_point_dup, 0);

rb_attr(cEC_POINT, rb_intern("group"), 1, 0, 0);

rb_define_method(cEC_POINT, "eql?", ossl_ec_point_eql, 1);
rb_define_alias(cEC_POINT, "==", "eql?");

Expand All @@ -1652,7 +1782,16 @@ void Init_ossl_ec(void)
rb_define_method(cEC_POINT, "make_affine!", ossl_ec_point_make_affine, 0);
rb_define_method(cEC_POINT, "invert!", ossl_ec_point_invert, 0);
rb_define_method(cEC_POINT, "set_to_infinity!", ossl_ec_point_set_to_infinity, 0);
/* all the other methods */

#ifdef HAVE_EC_POINT_GET_AFFINE_COORDINATES
rb_define_method(cEC_POINT, "affine_coords", ossl_ec_point_get_affine_coords, 0);
#endif
#ifdef HAVE_EC_POINT_SET_AFFINE_COORDINATES
rb_define_method(cEC_POINT, "affine_coords=", ossl_ec_point_set_affine_coords, 1);
#endif
#ifdef HAVE_EC_POINT_SET_COMPRESSED_COORDINATES
rb_define_method(cEC_POINT, "set_compressed_coords", ossl_ec_point_set_compressed_coords, 2);
#endif

rb_define_method(cEC_POINT, "to_octet_string", ossl_ec_point_to_octet_string, 1);
rb_define_method(cEC_POINT, "add", ossl_ec_point_add, 1);
Expand Down
Loading