From 9f152adfcfef4e6d4d4a680460bb75f2e0754a08 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 9 Nov 2018 17:06:51 -0600 Subject: [PATCH] Use EC_RAW_POINT in ECDSA. Now the only allocations in ECDSA are the ECDSA_SIG input and output. Change-Id: If1fcde6dc2ee2c53f5adc16a7f692e22e9c238de Reviewed-on: https://boringssl-review.googlesource.com/c/33069 Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org Reviewed-by: Adam Langley --- crypto/ec_extra/ec_asn1.c | 4 ++-- crypto/ecdh_extra/ecdh_extra.c | 8 +++++-- crypto/fipsmodule/ec/ec.c | 38 ++++++++++++++------------------- crypto/fipsmodule/ec/ec_key.c | 6 +++--- crypto/fipsmodule/ec/ec_test.cc | 5 +++-- crypto/fipsmodule/ec/internal.h | 16 +++++++++----- crypto/fipsmodule/ecdh/ecdh.c | 9 ++++++-- crypto/fipsmodule/ecdsa/ecdsa.c | 33 ++++++++-------------------- 8 files changed, 57 insertions(+), 62 deletions(-) diff --git a/crypto/ec_extra/ec_asn1.c b/crypto/ec_extra/ec_asn1.c index 9d9a2001..6e212751 100644 --- a/crypto/ec_extra/ec_asn1.c +++ b/crypto/ec_extra/ec_asn1.c @@ -159,8 +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_scalar(group, ret->pub_key, &ret->priv_key->scalar, NULL, - NULL)) { + if (!ec_point_mul_scalar(group, &ret->pub_key->raw, &ret->priv_key->scalar, + NULL, NULL)) { goto err; } // Remember the original private-key-only encoding. diff --git a/crypto/ecdh_extra/ecdh_extra.c b/crypto/ecdh_extra/ecdh_extra.c index 80dcfb06..eb321f7d 100644 --- a/crypto/ecdh_extra/ecdh_extra.c +++ b/crypto/ecdh_extra/ecdh_extra.c @@ -87,6 +87,11 @@ int ECDH_compute_key(void *out, size_t outlen, const EC_POINT *pub_key, return -1; } const EC_SCALAR *const priv = &priv_key->priv_key->scalar; + const EC_GROUP *const group = EC_KEY_get0_group(priv_key); + if (EC_GROUP_cmp(group, pub_key->group, NULL) != 0) { + OPENSSL_PUT_ERROR(EC, EC_R_INCOMPATIBLE_OBJECTS); + return -1; + } BN_CTX *ctx = BN_CTX_new(); if (ctx == NULL) { @@ -98,14 +103,13 @@ int ECDH_compute_key(void *out, size_t outlen, const EC_POINT *pub_key, size_t buflen = 0; uint8_t *buf = NULL; - const EC_GROUP *const group = EC_KEY_get0_group(priv_key); EC_POINT *tmp = EC_POINT_new(group); if (tmp == NULL) { OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE); goto err; } - if (!ec_point_mul_scalar(group, tmp, NULL, pub_key, priv)) { + if (!ec_point_mul_scalar(group, &tmp->raw, NULL, &pub_key->raw, priv)) { OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE); goto err; } diff --git a/crypto/fipsmodule/ec/ec.c b/crypto/fipsmodule/ec/ec.c index 4420c089..34383e8e 100644 --- a/crypto/fipsmodule/ec/ec.c +++ b/crypto/fipsmodule/ec/ec.c @@ -865,6 +865,12 @@ int EC_POINT_mul(const EC_GROUP *group, EC_POINT *r, const BIGNUM *g_scalar, return 0; } + if (EC_GROUP_cmp(group, r->group, NULL) != 0 || + (p != NULL && EC_GROUP_cmp(group, p->group, NULL) != 0)) { + OPENSSL_PUT_ERROR(EC, EC_R_INCOMPATIBLE_OBJECTS); + return 0; + } + int ret = 0; EC_SCALAR g_scalar_storage, p_scalar_storage; EC_SCALAR *g_scalar_arg = NULL, *p_scalar_arg = NULL; @@ -891,7 +897,8 @@ int EC_POINT_mul(const EC_GROUP *group, EC_POINT *r, const BIGNUM *g_scalar, p_scalar_arg = &p_scalar_storage; } - ret = ec_point_mul_scalar(group, r, g_scalar_arg, p, p_scalar_arg); + ret = ec_point_mul_scalar(group, &r->raw, g_scalar_arg, + p == NULL ? NULL : &p->raw, p_scalar_arg); err: BN_CTX_free(new_ctx); @@ -900,42 +907,29 @@ err: return ret; } -int ec_point_mul_scalar_public(const EC_GROUP *group, EC_POINT *r, - const EC_SCALAR *g_scalar, const EC_POINT *p, +int ec_point_mul_scalar_public(const EC_GROUP *group, EC_RAW_POINT *r, + const EC_SCALAR *g_scalar, const EC_RAW_POINT *p, const EC_SCALAR *p_scalar) { if ((g_scalar == NULL && p_scalar == NULL) || - (p == NULL) != (p_scalar == NULL)) { + (p == NULL) != (p_scalar == NULL)) { OPENSSL_PUT_ERROR(EC, ERR_R_PASSED_NULL_PARAMETER); return 0; } - if (EC_GROUP_cmp(group, r->group, NULL) != 0 || - (p != NULL && EC_GROUP_cmp(group, p->group, NULL) != 0)) { - OPENSSL_PUT_ERROR(EC, EC_R_INCOMPATIBLE_OBJECTS); - return 0; - } - - group->meth->mul_public(group, &r->raw, g_scalar, &p->raw, p_scalar); + group->meth->mul_public(group, r, g_scalar, p, p_scalar); return 1; } -int ec_point_mul_scalar(const EC_GROUP *group, EC_POINT *r, - const EC_SCALAR *g_scalar, const EC_POINT *p, +int ec_point_mul_scalar(const EC_GROUP *group, EC_RAW_POINT *r, + const EC_SCALAR *g_scalar, const EC_RAW_POINT *p, const EC_SCALAR *p_scalar) { if ((g_scalar == NULL && p_scalar == NULL) || - (p == NULL) != (p_scalar == NULL)) { + (p == NULL) != (p_scalar == NULL)) { OPENSSL_PUT_ERROR(EC, ERR_R_PASSED_NULL_PARAMETER); return 0; } - if (EC_GROUP_cmp(group, r->group, NULL) != 0 || - (p != NULL && EC_GROUP_cmp(group, p->group, NULL) != 0)) { - OPENSSL_PUT_ERROR(EC, EC_R_INCOMPATIBLE_OBJECTS); - return 0; - } - - group->meth->mul(group, &r->raw, g_scalar, (p == NULL) ? NULL : &p->raw, - p_scalar); + group->meth->mul(group, r, g_scalar, p, p_scalar); return 1; } diff --git a/crypto/fipsmodule/ec/ec_key.c b/crypto/fipsmodule/ec/ec_key.c index defd77c2..632dc9b2 100644 --- a/crypto/fipsmodule/ec/ec_key.c +++ b/crypto/fipsmodule/ec/ec_key.c @@ -322,8 +322,8 @@ int EC_KEY_check_key(const EC_KEY *eckey) { if (eckey->priv_key != NULL) { point = EC_POINT_new(eckey->group); if (point == NULL || - !ec_point_mul_scalar(eckey->group, point, &eckey->priv_key->scalar, - NULL, NULL)) { + !ec_point_mul_scalar(eckey->group, &point->raw, + &eckey->priv_key->scalar, NULL, NULL)) { OPENSSL_PUT_ERROR(EC, ERR_R_EC_LIB); goto err; } @@ -413,7 +413,7 @@ int EC_KEY_generate_key(EC_KEY *key) { // 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, + !ec_point_mul_scalar(key->group, &pub_key->raw, &priv_key->scalar, NULL, NULL)) { EC_POINT_free(pub_key); ec_wrapped_scalar_free(priv_key); diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc index d45a52fe..87146ad0 100644 --- a/crypto/fipsmodule/ec/ec_test.cc +++ b/crypto/fipsmodule/ec/ec_test.cc @@ -726,7 +726,8 @@ TEST_P(ECCurveTest, DoubleSpecialCase) { EC_SCALAR one; ASSERT_TRUE(ec_bignum_to_scalar(group(), &one, BN_value_one())); - ASSERT_TRUE(ec_point_mul_scalar_public(group(), p.get(), &one, g, &one)); + ASSERT_TRUE( + ec_point_mul_scalar_public(group(), &p->raw, &one, &g->raw, &one)); EXPECT_EQ(0, EC_POINT_cmp(group(), p.get(), two_g.get(), nullptr)); } @@ -871,7 +872,7 @@ TEST(ECTest, ScalarBaseMultVectors) { EC_SCALAR a_scalar, b_scalar; ASSERT_TRUE(ec_bignum_to_scalar(group.get(), &a_scalar, a.get())); ASSERT_TRUE(ec_bignum_to_scalar(group.get(), &b_scalar, b.get())); - ASSERT_TRUE(ec_point_mul_scalar_public(group.get(), p.get(), &a_scalar, g, + ASSERT_TRUE(ec_point_mul_scalar_public(group.get(), &p->raw, &a_scalar, &g->raw, &b_scalar)); check_point(p.get()); } diff --git a/crypto/fipsmodule/ec/internal.h b/crypto/fipsmodule/ec/internal.h index be548a32..7c7937b9 100644 --- a/crypto/fipsmodule/ec/internal.h +++ b/crypto/fipsmodule/ec/internal.h @@ -239,6 +239,10 @@ struct ec_point_st { // group is an owning reference to |group|, unless this is // |group->generator|. EC_GROUP *group; + // raw is the group-specific point data. Functions that take |EC_POINT| + // typically check consistency with |EC_GROUP| while functions that take + // |EC_RAW_POINT| do not. Thus accesses to this field should be externally + // checked for consistency. EC_RAW_POINT raw; } /* EC_POINT */; @@ -325,16 +329,18 @@ int ec_scalar_inv_montgomery_vartime(const EC_GROUP *group, EC_SCALAR *r, // |p_scalar|. Unlike other functions which take |EC_SCALAR|, |g_scalar| and // |p_scalar| need not be fully reduced. They need only contain as many bits as // the order. -int ec_point_mul_scalar(const EC_GROUP *group, EC_POINT *r, - const EC_SCALAR *g_scalar, const EC_POINT *p, +int ec_point_mul_scalar(const EC_GROUP *group, EC_RAW_POINT *r, + const EC_SCALAR *g_scalar, const EC_RAW_POINT *p, const EC_SCALAR *p_scalar); // ec_point_mul_scalar_public performs the same computation as // ec_point_mul_scalar. It further assumes that the inputs are public so // there is no concern about leaking their values through timing. -OPENSSL_EXPORT int ec_point_mul_scalar_public( - const EC_GROUP *group, EC_POINT *r, const EC_SCALAR *g_scalar, - const EC_POINT *p, const EC_SCALAR *p_scalar); +OPENSSL_EXPORT int ec_point_mul_scalar_public(const EC_GROUP *group, + EC_RAW_POINT *r, + const EC_SCALAR *g_scalar, + const EC_RAW_POINT *p, + const EC_SCALAR *p_scalar); // ec_cmp_x_coordinate compares the x (affine) coordinate of |p|, mod the group // order, with |r|. It returns one if the values match and zero if |p| is the diff --git a/crypto/fipsmodule/ecdh/ecdh.c b/crypto/fipsmodule/ecdh/ecdh.c index 726fa6d7..19d12c98 100644 --- a/crypto/fipsmodule/ecdh/ecdh.c +++ b/crypto/fipsmodule/ecdh/ecdh.c @@ -86,6 +86,11 @@ int ECDH_compute_key_fips(uint8_t *out, size_t out_len, const EC_POINT *pub_key, return 0; } const EC_SCALAR *const priv = &priv_key->priv_key->scalar; + const EC_GROUP *const group = EC_KEY_get0_group(priv_key); + if (EC_GROUP_cmp(group, pub_key->group, NULL) != 0) { + OPENSSL_PUT_ERROR(EC, EC_R_INCOMPATIBLE_OBJECTS); + return 0; + } BN_CTX *ctx = BN_CTX_new(); if (ctx == NULL) { @@ -97,14 +102,14 @@ int ECDH_compute_key_fips(uint8_t *out, size_t out_len, const EC_POINT *pub_key, size_t buflen = 0; uint8_t *buf = NULL; - const EC_GROUP *const group = EC_KEY_get0_group(priv_key); EC_POINT *shared_point = EC_POINT_new(group); if (shared_point == NULL) { OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE); goto err; } - if (!ec_point_mul_scalar(group, shared_point, NULL, pub_key, priv)) { + if (!ec_point_mul_scalar(group, &shared_point->raw, NULL, &pub_key->raw, + priv)) { OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE); goto err; } diff --git a/crypto/fipsmodule/ecdsa/ecdsa.c b/crypto/fipsmodule/ecdsa/ecdsa.c index 6d5d3882..0e89b431 100644 --- a/crypto/fipsmodule/ecdsa/ecdsa.c +++ b/crypto/fipsmodule/ecdsa/ecdsa.c @@ -173,27 +173,18 @@ int ECDSA_do_verify(const uint8_t *digest, size_t digest_len, ec_scalar_mul_montgomery(group, &u1, &m, &s_inv_mont); ec_scalar_mul_montgomery(group, &u2, &r, &s_inv_mont); - int ret = 0; - EC_POINT *point = EC_POINT_new(group); - if (point == NULL) { - OPENSSL_PUT_ERROR(ECDSA, ERR_R_MALLOC_FAILURE); - goto err; - } - if (!ec_point_mul_scalar_public(group, point, &u1, pub_key, &u2)) { + EC_RAW_POINT point; + if (!ec_point_mul_scalar_public(group, &point, &u1, &pub_key->raw, &u2)) { OPENSSL_PUT_ERROR(ECDSA, ERR_R_EC_LIB); - goto err; + return 0; } - if (!ec_cmp_x_coordinate(group, &point->raw, &r)) { + if (!ec_cmp_x_coordinate(group, &point, &r)) { OPENSSL_PUT_ERROR(ECDSA, ECDSA_R_BAD_SIGNATURE); - goto err; + return 0; } - ret = 1; - -err: - EC_POINT_free(point); - return ret; + return 1; } static int ecdsa_sign_setup(const EC_KEY *eckey, EC_SCALAR *out_kinv_mont, @@ -210,12 +201,7 @@ static int ecdsa_sign_setup(const EC_KEY *eckey, EC_SCALAR *out_kinv_mont, int ret = 0; EC_SCALAR k; - EC_POINT *tmp_point = EC_POINT_new(group); - if (tmp_point == NULL) { - OPENSSL_PUT_ERROR(ECDSA, ERR_R_EC_LIB); - goto err; - } - + EC_RAW_POINT tmp_point; do { // Include the private key and message digest in the k generation. if (eckey->fixed_k != NULL) { @@ -246,8 +232,8 @@ static int ecdsa_sign_setup(const EC_KEY *eckey, EC_SCALAR *out_kinv_mont, ec_scalar_from_montgomery(group, out_kinv_mont, out_kinv_mont); // Compute r, the x-coordinate of generator * k. - if (!ec_point_mul_scalar(group, tmp_point, &k, NULL, NULL) || - !ec_get_x_coordinate_as_scalar(group, out_r, &tmp_point->raw)) { + if (!ec_point_mul_scalar(group, &tmp_point, &k, NULL, NULL) || + !ec_get_x_coordinate_as_scalar(group, out_r, &tmp_point)) { goto err; } } while (ec_scalar_is_zero(group, out_r)); @@ -256,7 +242,6 @@ static int ecdsa_sign_setup(const EC_KEY *eckey, EC_SCALAR *out_kinv_mont, err: OPENSSL_cleanse(&k, sizeof(k)); - EC_POINT_free(tmp_point); return ret; }