From a6bfc45b6286e358ba83f7daa769e1e9012cc7bb Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 6 Mar 2018 20:21:28 -0500 Subject: [PATCH] Store EC_KEY's private key as an EC_SCALAR. This isn't strictly necessary now that BIGNUMs are safe, but we get to rely on type-system annotations from EC_SCALAR. Additionally, EC_POINT_mul depends on BN_div, while the EC_SCALAR version does not. Change-Id: I75e6967f3d35aef17278b94862f4e506baff5c23 Reviewed-on: https://boringssl-review.googlesource.com/26424 Reviewed-by: Steven Valdez Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org --- crypto/ec_extra/ec_asn1.c | 19 +++-- crypto/ecdh/ecdh.c | 7 +- crypto/fipsmodule/bn/random.c | 3 +- crypto/fipsmodule/ec/ec_key.c | 123 +++++++++++++++----------------- crypto/fipsmodule/ec/ec_test.cc | 8 +++ crypto/fipsmodule/ec/internal.h | 9 ++- crypto/fipsmodule/ecdsa/ecdsa.c | 16 ++--- 7 files changed, 92 insertions(+), 93 deletions(-) diff --git a/crypto/ec_extra/ec_asn1.c b/crypto/ec_extra/ec_asn1.c index c125af2b..bde6d0b3 100644 --- a/crypto/ec_extra/ec_asn1.c +++ b/crypto/ec_extra/ec_asn1.c @@ -86,6 +86,7 @@ EC_KEY *EC_KEY_parse_private_key(CBS *cbs, const EC_GROUP *group) { // Parse the optional parameters field. EC_GROUP *inner_group = NULL; EC_KEY *ret = NULL; + BIGNUM *priv_key = NULL; if (CBS_peek_asn1_tag(&ec_private_key, kParametersTag)) { // Per SEC 1, as an alternative to omitting it, one is allowed to specify // this field and put in a NULL to mean inheriting this value. This was @@ -126,15 +127,10 @@ EC_KEY *EC_KEY_parse_private_key(CBS *cbs, const EC_GROUP *group) { // Although RFC 5915 specifies the length of the key, OpenSSL historically // got this wrong, so accept any length. See upstream's // 30cd4ff294252c4b6a4b69cbef6a5b4117705d22. - ret->priv_key = - BN_bin2bn(CBS_data(&private_key), CBS_len(&private_key), NULL); + priv_key = BN_bin2bn(CBS_data(&private_key), CBS_len(&private_key), NULL); ret->pub_key = EC_POINT_new(group); - if (ret->priv_key == NULL || ret->pub_key == NULL) { - goto err; - } - - if (BN_cmp(ret->priv_key, EC_GROUP_get0_order(group)) >= 0) { - OPENSSL_PUT_ERROR(EC, EC_R_WRONG_ORDER); + if (priv_key == NULL || ret->pub_key == NULL || + !EC_KEY_set_private_key(ret, priv_key)) { goto err; } @@ -163,7 +159,8 @@ EC_KEY *EC_KEY_parse_private_key(CBS *cbs, const EC_GROUP *group) { (point_conversion_form_t)(CBS_data(&public_key)[0] & ~0x01); } else { // Compute the public key instead. - if (!EC_POINT_mul(group, ret->pub_key, ret->priv_key, NULL, NULL, NULL)) { + if (!ec_point_mul_scalar(group, ret->pub_key, &ret->priv_key->scalar, NULL, + NULL, NULL)) { goto err; } // Remember the original private-key-only encoding. @@ -181,11 +178,13 @@ EC_KEY *EC_KEY_parse_private_key(CBS *cbs, const EC_GROUP *group) { goto err; } + BN_free(priv_key); EC_GROUP_free(inner_group); return ret; err: EC_KEY_free(ret); + BN_free(priv_key); EC_GROUP_free(inner_group); return NULL; } @@ -203,7 +202,7 @@ int EC_KEY_marshal_private_key(CBB *cbb, const EC_KEY *key, !CBB_add_asn1(&ec_private_key, &private_key, CBS_ASN1_OCTETSTRING) || !BN_bn2cbb_padded(&private_key, BN_num_bytes(EC_GROUP_get0_order(key->group)), - key->priv_key)) { + EC_KEY_get0_private_key(key))) { OPENSSL_PUT_ERROR(EC, EC_R_ENCODE_ERROR); return 0; } diff --git a/crypto/ecdh/ecdh.c b/crypto/ecdh/ecdh.c index f38de2f1..7634ba59 100644 --- a/crypto/ecdh/ecdh.c +++ b/crypto/ecdh/ecdh.c @@ -74,6 +74,7 @@ #include #include +#include "../fipsmodule/ec/internal.h" #include "../internal.h" @@ -81,11 +82,11 @@ int ECDH_compute_key(void *out, size_t outlen, const EC_POINT *pub_key, const EC_KEY *priv_key, void *(*kdf)(const void *in, size_t inlen, void *out, size_t *outlen)) { - const BIGNUM *const priv = EC_KEY_get0_private_key(priv_key); - if (priv == NULL) { + if (priv_key->priv_key == NULL) { OPENSSL_PUT_ERROR(ECDH, ECDH_R_NO_PRIVATE_VALUE); return -1; } + const EC_SCALAR *const priv = &priv_key->priv_key->scalar; BN_CTX *ctx = BN_CTX_new(); if (ctx == NULL) { @@ -104,7 +105,7 @@ int ECDH_compute_key(void *out, size_t outlen, const EC_POINT *pub_key, goto err; } - if (!EC_POINT_mul(group, tmp, NULL, pub_key, priv, ctx)) { + if (!ec_point_mul_scalar(group, tmp, NULL, pub_key, priv, ctx)) { OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE); goto err; } diff --git a/crypto/fipsmodule/bn/random.c b/crypto/fipsmodule/bn/random.c index 733520d8..c6f9f089 100644 --- a/crypto/fipsmodule/bn/random.c +++ b/crypto/fipsmodule/bn/random.c @@ -120,8 +120,6 @@ #include "../rand/internal.h" -static const uint8_t kDefaultAdditionalData[32] = {0}; - int BN_rand(BIGNUM *rnd, int bits, int top, int bottom) { uint8_t *buf = NULL; int ret = 0, bit, bytes, mask; @@ -278,6 +276,7 @@ int bn_rand_range_words(BN_ULONG *out, BN_ULONG min_inclusive, int BN_rand_range_ex(BIGNUM *r, BN_ULONG min_inclusive, const BIGNUM *max_exclusive) { + static const uint8_t kDefaultAdditionalData[32] = {0}; if (!bn_wexpand(r, max_exclusive->width) || !bn_rand_range_words(r->d, min_inclusive, max_exclusive->d, max_exclusive->width, kDefaultAdditionalData)) { diff --git a/crypto/fipsmodule/ec/ec_key.c b/crypto/fipsmodule/ec/ec_key.c index b56139d0..a6d46976 100644 --- a/crypto/fipsmodule/ec/ec_key.c +++ b/crypto/fipsmodule/ec/ec_key.c @@ -84,6 +84,25 @@ DEFINE_STATIC_EX_DATA_CLASS(g_ec_ex_data_class); +static EC_WRAPPED_SCALAR *ec_wrapped_scalar_new(const EC_GROUP *group) { + EC_WRAPPED_SCALAR *wrapped = OPENSSL_malloc(sizeof(EC_WRAPPED_SCALAR)); + if (wrapped == NULL) { + OPENSSL_PUT_ERROR(EC, ERR_R_MALLOC_FAILURE); + return NULL; + } + + OPENSSL_memset(wrapped, 0, sizeof(EC_WRAPPED_SCALAR)); + wrapped->bignum.d = wrapped->scalar.words; + wrapped->bignum.width = group->order.width; + wrapped->bignum.dmax = group->order.width; + wrapped->bignum.flags = BN_FLG_STATIC_DATA; + return wrapped; +} + +static void ec_wrapped_scalar_free(EC_WRAPPED_SCALAR *scalar) { + OPENSSL_free(scalar); +} + EC_KEY *EC_KEY_new(void) { return EC_KEY_new_method(NULL); } EC_KEY *EC_KEY_new_method(const ENGINE *engine) { @@ -151,7 +170,7 @@ void EC_KEY_free(EC_KEY *r) { EC_GROUP_free(r->group); EC_POINT_free(r->pub_key); - BN_clear_free(r->priv_key); + ec_wrapped_scalar_free(r->priv_key); BN_free(r->fixed_k); CRYPTO_free_ex_data(g_ec_ex_data_class_bss_get(), r, &r->ex_data); @@ -175,7 +194,7 @@ EC_KEY *EC_KEY_dup(const EC_KEY *src) { (src->pub_key != NULL && !EC_KEY_set_public_key(ret, src->pub_key)) || (src->priv_key != NULL && - !EC_KEY_set_private_key(ret, src->priv_key))) { + !EC_KEY_set_private_key(ret, EC_KEY_get0_private_key(src)))) { EC_KEY_free(ret); return NULL; } @@ -215,7 +234,7 @@ int EC_KEY_set_group(EC_KEY *key, const EC_GROUP *group) { } const BIGNUM *EC_KEY_get0_private_key(const EC_KEY *key) { - return key->priv_key; + return key->priv_key != NULL ? &key->priv_key->bignum : NULL; } int EC_KEY_set_private_key(EC_KEY *key, const BIGNUM *priv_key) { @@ -224,14 +243,18 @@ int EC_KEY_set_private_key(EC_KEY *key, const BIGNUM *priv_key) { return 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); + EC_WRAPPED_SCALAR *scalar = ec_wrapped_scalar_new(key->group); + if (scalar == NULL) { return 0; } - BN_clear_free(key->priv_key); - key->priv_key = BN_dup(priv_key); - return (key->priv_key == NULL) ? 0 : 1; + if (!ec_bignum_to_scalar(key->group, &scalar->scalar, priv_key)) { + OPENSSL_PUT_ERROR(EC, EC_R_WRONG_ORDER); + ec_wrapped_scalar_free(scalar); + return 0; + } + ec_wrapped_scalar_free(key->priv_key); + key->priv_key = scalar; + return 1; } const EC_POINT *EC_KEY_get0_public_key(const EC_KEY *key) { @@ -296,15 +319,11 @@ int EC_KEY_check_key(const EC_KEY *eckey) { } // in case the priv_key is present : // check if generator * priv_key == pub_key - if (eckey->priv_key) { - if (BN_is_negative(eckey->priv_key) || - BN_cmp(eckey->priv_key, EC_GROUP_get0_order(eckey->group)) >= 0) { - OPENSSL_PUT_ERROR(EC, EC_R_WRONG_ORDER); - goto err; - } + if (eckey->priv_key != NULL) { point = EC_POINT_new(eckey->group); if (point == NULL || - !EC_POINT_mul(eckey->group, point, eckey->priv_key, NULL, NULL, ctx)) { + !ec_point_mul_scalar(eckey->group, point, &eckey->priv_key->scalar, + NULL, NULL, ctx)) { OPENSSL_PUT_ERROR(EC, ERR_R_EC_LIB); goto err; } @@ -375,65 +394,37 @@ err: return ok; } -int EC_KEY_generate_key(EC_KEY *eckey) { - int ok = 0; - BIGNUM *priv_key = NULL; - EC_POINT *pub_key = NULL; - - if (!eckey || !eckey->group) { +int EC_KEY_generate_key(EC_KEY *key) { + if (key == NULL || key->group == NULL) { OPENSSL_PUT_ERROR(EC, ERR_R_PASSED_NULL_PARAMETER); return 0; } - if (eckey->priv_key == NULL) { - priv_key = BN_new(); - if (priv_key == NULL) { - goto err; - } - } else { - priv_key = eckey->priv_key; - } - - const BIGNUM *order = EC_GROUP_get0_order(eckey->group); - - // Check that the size of the group order is FIPS compliant (FIPS 186-4 - // B.4.2). - if (BN_num_bits(order) < 160) { + // Check that the group order is FIPS compliant (FIPS 186-4 B.4.2). + if (BN_num_bits(EC_GROUP_get0_order(key->group)) < 160) { OPENSSL_PUT_ERROR(EC, EC_R_INVALID_GROUP_ORDER); - goto err; + return 0; } - // Generate the private key by testing candidates (FIPS 186-4 B.4.2). - if (!BN_rand_range_ex(priv_key, 1, order)) { - goto err; - } - - if (eckey->pub_key == NULL) { - pub_key = EC_POINT_new(eckey->group); - if (pub_key == NULL) { - goto err; - } - } else { - pub_key = eckey->pub_key; - } - - if (!EC_POINT_mul(eckey->group, pub_key, priv_key, NULL, NULL, NULL)) { - goto err; - } - - eckey->priv_key = priv_key; - eckey->pub_key = pub_key; - - ok = 1; - -err: - if (eckey->pub_key == NULL) { + static const uint8_t kDefaultAdditionalData[32] = {0}; + EC_WRAPPED_SCALAR *priv_key = ec_wrapped_scalar_new(key->group); + EC_POINT *pub_key = EC_POINT_new(key->group); + if (priv_key == NULL || pub_key == NULL || + // Generate the private key by testing candidates (FIPS 186-4 B.4.2). + !ec_random_nonzero_scalar(key->group, &priv_key->scalar, + kDefaultAdditionalData) || + !ec_point_mul_scalar(key->group, pub_key, &priv_key->scalar, NULL, NULL, + NULL)) { EC_POINT_free(pub_key); + ec_wrapped_scalar_free(priv_key); + return 0; } - if (eckey->priv_key == NULL) { - BN_free(priv_key); - } - return ok; + + ec_wrapped_scalar_free(key->priv_key); + key->priv_key = priv_key; + EC_POINT_free(key->pub_key); + key->pub_key = pub_key; + return 1; } int EC_KEY_generate_key_fips(EC_KEY *eckey) { diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc index 923ce1ee..5cce3614 100644 --- a/crypto/fipsmodule/ec/ec_test.cc +++ b/crypto/fipsmodule/ec/ec_test.cc @@ -359,6 +359,14 @@ TEST(ECTest, GroupMismatch) { EC_KEY_set_public_key(key.get(), EC_GROUP_get0_generator(p256.get()))); } +TEST(ECTest, EmptyKey) { + bssl::UniquePtr key(EC_KEY_new()); + ASSERT_TRUE(key); + EXPECT_FALSE(EC_KEY_get0_group(key.get())); + EXPECT_FALSE(EC_KEY_get0_public_key(key.get())); + EXPECT_FALSE(EC_KEY_get0_private_key(key.get())); +} + class ECCurveTest : public testing::TestWithParam {}; TEST_P(ECCurveTest, SetAffine) { diff --git a/crypto/fipsmodule/ec/internal.h b/crypto/fipsmodule/ec/internal.h index e1a3e713..75fe81c0 100644 --- a/crypto/fipsmodule/ec/internal.h +++ b/crypto/fipsmodule/ec/internal.h @@ -277,11 +277,18 @@ const EC_METHOD *EC_GFp_nistp256_method(void); // x86-64 optimized P256. See http://eprint.iacr.org/2013/816. const EC_METHOD *EC_GFp_nistz256_method(void); +// An EC_WRAPPED_SCALAR is an |EC_SCALAR| with a parallel |BIGNUM| +// representation. It exists to support the |EC_KEY_get0_private_key| API. +typedef struct { + BIGNUM bignum; + EC_SCALAR scalar; +} EC_WRAPPED_SCALAR; + struct ec_key_st { EC_GROUP *group; EC_POINT *pub_key; - BIGNUM *priv_key; + EC_WRAPPED_SCALAR *priv_key; // fixed_k may contain a specific value of 'k', to be used in ECDSA signing. // This is only for the FIPS power-on tests. diff --git a/crypto/fipsmodule/ecdsa/ecdsa.c b/crypto/fipsmodule/ecdsa/ecdsa.c index e010dbd5..1d08123b 100644 --- a/crypto/fipsmodule/ecdsa/ecdsa.c +++ b/crypto/fipsmodule/ecdsa/ecdsa.c @@ -392,17 +392,17 @@ ECDSA_SIG *ECDSA_do_sign(const uint8_t *digest, size_t digest_len, } const EC_GROUP *group = EC_KEY_get0_group(eckey); - const BIGNUM *priv_key_bn = EC_KEY_get0_private_key(eckey); - if (group == NULL || priv_key_bn == NULL) { + if (group == NULL || eckey->priv_key == NULL) { OPENSSL_PUT_ERROR(ECDSA, ERR_R_PASSED_NULL_PARAMETER); return NULL; } const BIGNUM *order = EC_GROUP_get0_order(group); + const EC_SCALAR *priv_key = &eckey->priv_key->scalar; int ok = 0; ECDSA_SIG *ret = ECDSA_SIG_new(); BN_CTX *ctx = BN_CTX_new(); - EC_SCALAR kinv_mont, priv_key, r_mont, s; + EC_SCALAR kinv_mont, r_mont, s; EC_LOOSE_SCALAR m, tmp; if (ret == NULL || ctx == NULL) { OPENSSL_PUT_ERROR(ECDSA, ERR_R_MALLOC_FAILURE); @@ -410,14 +410,9 @@ ECDSA_SIG *ECDSA_do_sign(const uint8_t *digest, size_t digest_len, } digest_to_scalar(group, &m, digest, digest_len); - // TODO(davidben): Store the private key as an |EC_SCALAR| so this is obvious - // via the type system. - if (!ec_bignum_to_scalar_unchecked(group, &priv_key, priv_key_bn)) { - goto err; - } for (;;) { if (!ecdsa_sign_setup(eckey, ctx, &kinv_mont, &ret->r, digest, digest_len, - &priv_key)) { + priv_key)) { goto err; } @@ -427,7 +422,7 @@ ECDSA_SIG *ECDSA_do_sign(const uint8_t *digest, size_t digest_len, if (!ec_bignum_to_scalar(group, &r_mont, ret->r) || !bn_to_montgomery_small(r_mont.words, order->width, r_mont.words, order->width, group->order_mont) || - !scalar_mod_mul_montgomery(group, &s, &priv_key, &r_mont)) { + !scalar_mod_mul_montgomery(group, &s, priv_key, &r_mont)) { goto err; } @@ -455,7 +450,6 @@ err: } BN_CTX_free(ctx); OPENSSL_cleanse(&kinv_mont, sizeof(kinv_mont)); - OPENSSL_cleanse(&priv_key, sizeof(priv_key)); OPENSSL_cleanse(&r_mont, sizeof(r_mont)); OPENSSL_cleanse(&s, sizeof(s)); OPENSSL_cleanse(&tmp, sizeof(tmp));