kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Tighten EC_KEY's association with its group.

Browse Source 
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>
This commit is contained in:
David Benjamin 2017-12-16 15:34:00 -05:00 committed by CQ bot account: commit-bot@chromium.org
parent e15019572b
commit 5bcaa113e2
3 changed files with 67 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
crypto/fipsmodule/ec/ec_key.c
View File

@ -233,20 +233,21 @@ int EC_KEY_is_opaque(const EC_KEY *key) {
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 @@ const BIGNUM *EC_KEY_get0_private_key(const EC_KEY *key) {
}
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 @@ const EC_POINT *EC_KEY_get0_public_key(const EC_KEY *key) {
}
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;

30
crypto/fipsmodule/ec/ec_test.cc
View File

@ -305,6 +305,36 @@ TEST(ECTest, ArbitraryCurve) {
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) {

10
include/openssl/ec_key.h
View File

@ -116,14 +116,16 @@ OPENSSL_EXPORT int EC_KEY_is_opaque(const EC_KEY *key);
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 int EC_KEY_set_private_key(EC_KEY *key, const BIGNUM *prv);
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