Tighten EC_KEY's association with its group. This is to simplify https://boringssl-review.googlesource.com/c/boringssl/+/24445/. Setting or changing an EC_KEY's group after the public or private keys have been configured is quite awkward w.r.t. consistency checks. It becomes additionally messy if we mean to store private keys as EC_SCALARs (and avoid the BIGNUM timing leak), whose size is curve-dependent. Instead, require that callers configure the group before setting either half of the keypair. Additionally, reject EC_KEY_set_group calls that change the group. This will simplify clearing one more BIGNUM timing leak. Update-Note: This will break code which sets the group and key in a weird order. I checked calls of EC_KEY_new and confirmed they all set the group first. If I missed any, let me know. Change-Id: Ie89f90a318b31b6b98f71138e5ff3de5323bc9a6 Reviewed-on: https://boringssl-review.googlesource.com/24425 Commit-Queue: Adam Langley <agl@google.com> Reviewed-by: Adam Langley <agl@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/crypto/fipsmodule/ec/ec_key.c b/crypto/fipsmodule/ec/ec_key.c index f64cb21..4e0bcb2 100644 --- a/crypto/fipsmodule/ec/ec_key.c +++ b/crypto/fipsmodule/ec/ec_key.c
@@ -233,20 +233,21 @@ const EC_GROUP *EC_KEY_get0_group(const EC_KEY *key) { return key->group; } int EC_KEY_set_group(EC_KEY *key, const EC_GROUP *group) { + // If |key| already has a group, it is an error to switch to another one. + if (key->group != NULL) { + if (EC_GROUP_cmp(key->group, group, NULL) != 0) { + OPENSSL_PUT_ERROR(EC, EC_R_GROUP_MISMATCH); + return 0; + } + return 1; + } + + assert(key->priv_key == NULL); + assert(key->pub_key == NULL); + EC_GROUP_free(key->group); - // TODO(fork): duplicating the group seems wasteful but see - // |EC_KEY_set_conv_form|. key->group = EC_GROUP_dup(group); - if (key->group == NULL) { - return 0; - } - // XXX: |BN_cmp| is not constant time. - if (key->priv_key != NULL && - (BN_is_negative(key->priv_key) || - BN_cmp(key->priv_key, EC_GROUP_get0_order(group)) >= 0)) { - return 0; - } - return 1; + return key->group != NULL; } const BIGNUM *EC_KEY_get0_private_key(const EC_KEY *key) { @@ -254,10 +255,14 @@ } int EC_KEY_set_private_key(EC_KEY *key, const BIGNUM *priv_key) { + if (key->group == NULL) { + OPENSSL_PUT_ERROR(EC, EC_R_MISSING_PARAMETERS); + return 0; + } + // XXX: |BN_cmp| is not constant time. - if (key->group != NULL && - (BN_is_negative(priv_key) || - BN_cmp(priv_key, EC_GROUP_get0_order(key->group)) >= 0)) { + if (BN_is_negative(priv_key) || + BN_cmp(priv_key, EC_GROUP_get0_order(key->group)) >= 0) { OPENSSL_PUT_ERROR(EC, EC_R_WRONG_ORDER); return 0; } @@ -271,6 +276,16 @@ } int EC_KEY_set_public_key(EC_KEY *key, const EC_POINT *pub_key) { + if (key->group == NULL) { + OPENSSL_PUT_ERROR(EC, EC_R_MISSING_PARAMETERS); + return 0; + } + + if (EC_GROUP_cmp(key->group, pub_key->group, NULL) != 0) { + OPENSSL_PUT_ERROR(EC, EC_R_GROUP_MISMATCH); + return 0; + } + EC_POINT_free(key->pub_key); key->pub_key = EC_POINT_dup(pub_key, key->group); return (key->pub_key == NULL) ? 0 : 1;
diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc index 8e7a81d..d2cd2af 100644 --- a/crypto/fipsmodule/ec/ec_test.cc +++ b/crypto/fipsmodule/ec/ec_test.cc
@@ -305,6 +305,36 @@ EXPECT_NE(0, EC_GROUP_cmp(group.get(), group3.get(), NULL)); } +TEST(ECTest, SetKeyWithoutGroup) { + bssl::UniquePtr<EC_KEY> key(EC_KEY_new()); + ASSERT_TRUE(key); + + // Private keys may not be configured without a group. + EXPECT_FALSE(EC_KEY_set_private_key(key.get(), BN_value_one())); + + // Public keys may not be configured without a group. + bssl::UniquePtr<EC_GROUP> group( + EC_GROUP_new_by_curve_name(NID_X9_62_prime256v1)); + ASSERT_TRUE(group); + EXPECT_FALSE( + EC_KEY_set_public_key(key.get(), EC_GROUP_get0_generator(group.get()))); +} + +TEST(ECTest, GroupMismatch) { + bssl::UniquePtr<EC_KEY> key(EC_KEY_new_by_curve_name(NID_secp384r1)); + ASSERT_TRUE(key); + bssl::UniquePtr<EC_GROUP> p256( + EC_GROUP_new_by_curve_name(NID_X9_62_prime256v1)); + ASSERT_TRUE(p256); + + // Changing a key's group is invalid. + EXPECT_FALSE(EC_KEY_set_group(key.get(), p256.get())); + + // Configuring a public key with the wrong group is invalid. + EXPECT_FALSE( + EC_KEY_set_public_key(key.get(), EC_GROUP_get0_generator(p256.get()))); +} + class ECCurveTest : public testing::TestWithParam<EC_builtin_curve> {}; TEST_P(ECCurveTest, SetAffine) {
diff --git a/include/openssl/ec_key.h b/include/openssl/ec_key.h index 7ef1b14..5fe0318 100644 --- a/include/openssl/ec_key.h +++ b/include/openssl/ec_key.h
@@ -116,14 +116,16 @@ OPENSSL_EXPORT const EC_GROUP *EC_KEY_get0_group(const EC_KEY *key); // EC_KEY_set_group sets the |EC_GROUP| object that |key| will use to |group|. -// It returns one on success and zero otherwise. +// It returns one on success and zero otherwise. If |key| already has a group, +// it is an error to change to a different one. OPENSSL_EXPORT int EC_KEY_set_group(EC_KEY *key, const EC_GROUP *group); // EC_KEY_get0_private_key returns a pointer to the private key inside |key|. OPENSSL_EXPORT const BIGNUM *EC_KEY_get0_private_key(const EC_KEY *key); // EC_KEY_set_private_key sets the private key of |key| to |priv|. It returns -// one on success and zero otherwise. +// one on success and zero otherwise. |key| must already have had a group +// configured (see |EC_KEY_set_group| and |EC_KEY_new_by_curve_name|). OPENSSL_EXPORT int EC_KEY_set_private_key(EC_KEY *key, const BIGNUM *prv); // EC_KEY_get0_public_key returns a pointer to the public key point inside @@ -131,7 +133,9 @@ OPENSSL_EXPORT const EC_POINT *EC_KEY_get0_public_key(const EC_KEY *key); // EC_KEY_set_public_key sets the public key of |key| to |pub|, by copying it. -// It returns one on success and zero otherwise. +// It returns one on success and zero otherwise. |key| must already have had a +// group configured (see |EC_KEY_set_group| and |EC_KEY_new_by_curve_name|), and +// |pub| must also belong to that group. OPENSSL_EXPORT int EC_KEY_set_public_key(EC_KEY *key, const EC_POINT *pub); #define EC_PKEY_NO_PARAMETERS 0x001