Fold EC_GROUP_new_curve_GFp and EC_GROUP_set_generator into a EC_GROUP_new_arbitrary. This is only for Conscrypt which always calls the pair in succession. (Indeed it wouldn't make any sense to not call it.) Remove those two APIs and replace with a single merged API. This way incomplete EC_GROUPs never escape outside our API boundary and EC_GROUPs may *finally* be made immutable. Also add a test for this to make sure I didn't mess it up. Add a temporary BORINGSSL_201512 define to ease the transition for Conscrypt. Conscrypt requires https://android-review.googlesource.com/#/c/187801/ before picking up this change. Change-Id: I3706c2ceac31ed2313175ba5ee724bd5c74ef6e1 Reviewed-on: https://boringssl-review.googlesource.com/6550 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/ec/ec.c b/crypto/ec/ec.c index 827cc57..2f8d2b1 100644 --- a/crypto/ec/ec.c +++ b/crypto/ec/ec.c
@@ -350,8 +350,8 @@ return ret; } -EC_GROUP *EC_GROUP_new_curve_GFp(const BIGNUM *p, const BIGNUM *a, - const BIGNUM *b, BN_CTX *ctx) { +static EC_GROUP *ec_group_new_curve_GFp(const BIGNUM *p, const BIGNUM *a, + const BIGNUM *b, BN_CTX *ctx) { const EC_METHOD *meth = EC_GFp_mont_method(); EC_GROUP *ret; @@ -371,42 +371,38 @@ return ret; } -int EC_GROUP_set_generator(EC_GROUP *group, const EC_POINT *generator, - const BIGNUM *order, const BIGNUM *cofactor) { - if (group->curve_name != NID_undef) { - /* |EC_GROUP_set_generator| should only be used with |EC_GROUP|s returned - * by |EC_GROUP_new_curve_GFp|. */ - return 0; +EC_GROUP *EC_GROUP_new_arbitrary(const BIGNUM *p, const BIGNUM *a, + const BIGNUM *b, const BIGNUM *gx, + const BIGNUM *gy, const BIGNUM *order, + const BIGNUM *cofactor) { + EC_GROUP *ret = NULL; + BN_CTX *ctx; + + ctx = BN_CTX_new(); + if (ctx == NULL) { + goto err; } - if (group->generator == NULL) { - group->generator = EC_POINT_new(group); - if (group->generator == NULL) { - return 0; - } + ret = ec_group_new_curve_GFp(p, a, b, ctx); + if (ret == NULL) { + goto err; } - if (!EC_POINT_copy(group->generator, generator)) { - return 0; + ret->generator = EC_POINT_new(ret); + if (ret->generator == NULL || + !EC_POINT_set_affine_coordinates_GFp(ret, ret->generator, gx, gy, ctx) || + !BN_copy(&ret->order, order) || + !BN_copy(&ret->cofactor, cofactor)) { + goto err; } - if (order != NULL) { - if (!BN_copy(&group->order, order)) { - return 0; - } - } else { - BN_zero(&group->order); - } + BN_CTX_free(ctx); + return ret; - if (cofactor != NULL) { - if (!BN_copy(&group->cofactor, cofactor)) { - return 0; - } - } else { - BN_zero(&group->cofactor); - } - - return 1; +err: + EC_GROUP_free(ret); + BN_CTX_free(ctx); + return NULL; } static EC_GROUP *ec_group_new_from_data(unsigned built_in_index) { @@ -442,7 +438,7 @@ goto err; } } else { - if ((group = EC_GROUP_new_curve_GFp(p, a, b, ctx)) == NULL) { + if ((group = ec_group_new_curve_GFp(p, a, b, ctx)) == NULL) { OPENSSL_PUT_ERROR(EC, ERR_R_EC_LIB); goto err; }
diff --git a/crypto/ec/ec_test.cc b/crypto/ec/ec_test.cc index 2088e72..a20911b 100644 --- a/crypto/ec/ec_test.cc +++ b/crypto/ec/ec_test.cc
@@ -240,6 +240,92 @@ return true; } +static bool TestArbitraryCurve() { + // Make a P-256 key and extract the affine coordinates. + ScopedEC_KEY key(EC_KEY_new_by_curve_name(NID_X9_62_prime256v1)); + if (!key || !EC_KEY_generate_key(key.get())) { + return false; + } + + // Make an arbitrary curve which is identical to P-256. + static const uint8_t kP[] = { + 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + }; + static const uint8_t kA[] = { + 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfc, + }; + static const uint8_t kB[] = { + 0x5a, 0xc6, 0x35, 0xd8, 0xaa, 0x3a, 0x93, 0xe7, 0xb3, 0xeb, 0xbd, + 0x55, 0x76, 0x98, 0x86, 0xbc, 0x65, 0x1d, 0x06, 0xb0, 0xcc, 0x53, + 0xb0, 0xf6, 0x3b, 0xce, 0x3c, 0x3e, 0x27, 0xd2, 0x60, 0x4b, + }; + static const uint8_t kX[] = { + 0x6b, 0x17, 0xd1, 0xf2, 0xe1, 0x2c, 0x42, 0x47, 0xf8, 0xbc, 0xe6, + 0xe5, 0x63, 0xa4, 0x40, 0xf2, 0x77, 0x03, 0x7d, 0x81, 0x2d, 0xeb, + 0x33, 0xa0, 0xf4, 0xa1, 0x39, 0x45, 0xd8, 0x98, 0xc2, 0x96, + }; + static const uint8_t kY[] = { + 0x4f, 0xe3, 0x42, 0xe2, 0xfe, 0x1a, 0x7f, 0x9b, 0x8e, 0xe7, 0xeb, + 0x4a, 0x7c, 0x0f, 0x9e, 0x16, 0x2b, 0xce, 0x33, 0x57, 0x6b, 0x31, + 0x5e, 0xce, 0xcb, 0xb6, 0x40, 0x68, 0x37, 0xbf, 0x51, 0xf5, + }; + static const uint8_t kOrder[] = { + 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xbc, 0xe6, 0xfa, 0xad, 0xa7, 0x17, + 0x9e, 0x84, 0xf3, 0xb9, 0xca, 0xc2, 0xfc, 0x63, 0x25, 0x51, + }; + ScopedBIGNUM p(BN_bin2bn(kP, sizeof(kP), nullptr)); + ScopedBIGNUM a(BN_bin2bn(kA, sizeof(kA), nullptr)); + ScopedBIGNUM b(BN_bin2bn(kB, sizeof(kB), nullptr)); + ScopedBIGNUM x(BN_bin2bn(kX, sizeof(kX), nullptr)); + ScopedBIGNUM y(BN_bin2bn(kY, sizeof(kY), nullptr)); + ScopedBIGNUM order(BN_bin2bn(kOrder, sizeof(kOrder), nullptr)); + ScopedBIGNUM cofactor(BN_new()); + if (!p || !a || !b || !x || !y || !order || !cofactor || + !BN_set_word(cofactor.get(), 1)) { + return false; + } + ScopedEC_GROUP group(EC_GROUP_new_arbitrary(p.get(), a.get(), b.get(), + x.get(), y.get(), order.get(), + cofactor.get())); + if (!group) { + return false; + } + + // |group| should not have a curve name. + if (EC_GROUP_get_curve_name(group.get()) != NID_undef) { + return false; + } + + // Copy |key| to |key2| using |group|. + ScopedEC_KEY key2(EC_KEY_new()); + ScopedEC_POINT point(EC_POINT_new(group.get())); + if (!key2 || !point || + !EC_KEY_set_group(key2.get(), group.get()) || + !EC_KEY_set_private_key(key2.get(), EC_KEY_get0_private_key(key.get())) || + !EC_POINT_get_affine_coordinates_GFp(EC_KEY_get0_group(key.get()), + EC_KEY_get0_public_key(key.get()), + x.get(), y.get(), nullptr) || + !EC_POINT_set_affine_coordinates_GFp(group.get(), point.get(), x.get(), + y.get(), nullptr) || + !EC_KEY_set_public_key(key2.get(), point.get())) { + fprintf(stderr, "Could not copy key.\n"); + return false; + } + + // The key must be valid according to the new group too. + if (!EC_KEY_check_key(key2.get())) { + fprintf(stderr, "Copied key is not valid.\n"); + return false; + } + + return true; +} + int main(void) { CRYPTO_library_init(); ERR_load_crypto_strings(); @@ -249,7 +335,8 @@ !TestSetAffine(NID_secp224r1) || !TestSetAffine(NID_X9_62_prime256v1) || !TestSetAffine(NID_secp384r1) || - !TestSetAffine(NID_secp521r1)) { + !TestSetAffine(NID_secp521r1) || + !TestArbitraryCurve()) { fprintf(stderr, "failed\n"); return 1; }
diff --git a/include/openssl/base.h b/include/openssl/base.h index 65ab3f7..79f02cd 100644 --- a/include/openssl/base.h +++ b/include/openssl/base.h
@@ -108,7 +108,7 @@ #endif #define OPENSSL_IS_BORINGSSL -#define BORINGSSL_201510 +#define BORINGSSL_201512 #define OPENSSL_VERSION_NUMBER 0x10002000 #define SSLEAY_VERSION_NUMBER OPENSSL_VERSION_NUMBER
diff --git a/include/openssl/ec.h b/include/openssl/ec.h index 667be3b..26f40b7 100644 --- a/include/openssl/ec.h +++ b/include/openssl/ec.h
@@ -271,16 +271,16 @@ /* Deprecated functions. */ -/* EC_GROUP_new_curve_GFp creates a new, arbitrary elliptic curve group based - * on the equation y² = x³ + a·x + b. It returns the new group or NULL on - * error. +/* EC_GROUP_new_arbitrary creates a new, arbitrary elliptic curve group based on + * the equation y² = x³ + a·x + b. The generator is set to (gx, gy) which must + * have the given order and cofactor. It returns the new group or NULL on error. * * |EC_GROUP|s returned by this function will always compare as unequal via * |EC_GROUP_cmp| (even to themselves). |EC_GROUP_get_curve_name| will always * return |NID_undef|. */ -OPENSSL_EXPORT EC_GROUP *EC_GROUP_new_curve_GFp(const BIGNUM *p, - const BIGNUM *a, - const BIGNUM *b, BN_CTX *ctx); +OPENSSL_EXPORT EC_GROUP *EC_GROUP_new_arbitrary( + const BIGNUM *p, const BIGNUM *a, const BIGNUM *b, const BIGNUM *gx, + const BIGNUM *gy, const BIGNUM *order, const BIGNUM *cofactor); /* EC_GROUP_get_order sets |*order| to the order of |group|, if it's not * NULL. It returns one on success and zero otherwise. |ctx| is ignored. Use @@ -288,14 +288,6 @@ OPENSSL_EXPORT int EC_GROUP_get_order(const EC_GROUP *group, BIGNUM *order, BN_CTX *ctx); -/* EC_GROUP_set_generator sets the generator for |group| to |generator|, which - * must have the given order and cofactor. This should only be used with - * |EC_GROUP| objects returned by |EC_GROUP_new_curve_GFp|. */ -OPENSSL_EXPORT int EC_GROUP_set_generator(EC_GROUP *group, - const EC_POINT *generator, - const BIGNUM *order, - const BIGNUM *cofactor); - /* EC_GROUP_set_asn1_flag does nothing. */ OPENSSL_EXPORT void EC_GROUP_set_asn1_flag(EC_GROUP *group, int flag);