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
