Skip to content

Commit

Permalink
Merge pull request #361 from nebeid/fips-cherry-pick-pub-key-validati…
Browse files Browse the repository at this point in the history 
…on-ECDH

Cherry pick to fips-2021-10-20 public key validation in ECDH
  • Loading branch information
@darylmartin100
darylmartin100 authored Dec 22, 2021
2 parents f247b1d + fbfb838 commit 794872f
Show file tree
Hide file tree
Showing 5 changed files with 196 additions and 34 deletions.
121 changes: 120 additions & 1 deletion crypto/ecdh_extra/ecdh_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,12 @@
#include <openssl/nid.h>
#include <openssl/sha.h>

#include "../fipsmodule/ec/internal.h"

#include "../test/file_test.h"
#include "../test/test_util.h"
#include "../test/wycheproof_util.h"


static bssl::UniquePtr<EC_GROUP> GetCurve(FileTest *t, const char *key) {
std::string curve_name;
if (!t->GetAttribute(&curve_name, key)) {
Expand Down Expand Up @@ -126,6 +127,124 @@ TEST(ECDHTest, TestVectors) {
});
}

static int has_uint128_and_not_small() {
#if defined(BORINGSSL_HAS_UINT128) && !defined(OPENSSL_SMALL)
return 1;
#else
return 0;
#endif
}

static int awslc_fips() {
#if defined(AWSLC_FIPS)
return 1;
#else
return 0;
#endif
}

// The following test is adapted from ECTest.LargeXCoordinateVectors
TEST(ECDHTest, InvalidPubKeyLargeCoord) {
bssl::UniquePtr<BN_CTX> ctx(BN_CTX_new());
ASSERT_TRUE(ctx);

FileTestGTest("crypto/fipsmodule/ec/large_x_coordinate_points.txt",
[&](FileTest *t) {
int ret;
bssl::UniquePtr<EC_GROUP> group = GetCurve(t, "Curve");
ASSERT_TRUE(group);
bssl::UniquePtr<BIGNUM> x = GetBIGNUM(t, "X");
ASSERT_TRUE(x);
bssl::UniquePtr<BIGNUM> xpp = GetBIGNUM(t, "XplusP");
ASSERT_TRUE(xpp);
bssl::UniquePtr<BIGNUM> y = GetBIGNUM(t, "Y");
ASSERT_TRUE(y);
bssl::UniquePtr<EC_KEY> peer_key(EC_KEY_new());
ASSERT_TRUE(peer_key);
bssl::UniquePtr<EC_POINT> pub_key(EC_POINT_new(group.get()));
ASSERT_TRUE(pub_key);
bssl::UniquePtr<EC_KEY> priv_key(EC_KEY_new());
// Own private key
ASSERT_TRUE(priv_key);
ASSERT_TRUE(EC_KEY_set_group(priv_key.get(), group.get()));
// Generate a generic ec key.
EC_KEY_generate_key(priv_key.get());

size_t len = BN_num_bytes(&group.get()->field); // Modulus byte-length
std::vector<uint8_t> shared_key((group.get()->curve_name == NID_secp521r1)?
SHA512_DIGEST_LENGTH : len);

ASSERT_TRUE(EC_KEY_set_group(peer_key.get(), group.get()));
// The following call converts the point to Montgomery form for P-256, 384 and 521.
// For P-224, when the functions from simple.c are used, i.e. when
// group->meth = EC_GFp_nistp224_method, the coordinate representation is not changed.
// This is determined based on compile flags in ec.c that are also used below
// in has_uint128_and_not_small().
ASSERT_TRUE(EC_POINT_set_affine_coordinates_GFp(
group.get(), pub_key.get(), x.get(), y.get(), nullptr));
ASSERT_TRUE(EC_KEY_set_public_key(peer_key.get(), pub_key.get()));
ASSERT_TRUE(ECDH_compute_key_fips(shared_key.data(), shared_key.size(),
EC_KEY_get0_public_key(peer_key.get()), priv_key.get()));
// Ensure the pointers were not affected.
ASSERT_TRUE(peer_key.get());
ASSERT_TRUE(pub_key.get());

// Set the raw point directly with the BIGNUM coordinates.
// Note that both are in little-endian byte order.
OPENSSL_memcpy(peer_key.get()->pub_key->raw.X.bytes, (const uint8_t *)x.get()->d, len);
OPENSSL_memcpy(peer_key.get()->pub_key->raw.Y.bytes, (const uint8_t *)y.get()->d, len);
OPENSSL_memset(peer_key.get()->pub_key->raw.Z.bytes, 0, len);
peer_key.get()->pub_key->raw.Z.bytes[0] = 1;
// As mentioned, for P-224, setting the raw point directly with the coordinates
// still passes |EC_KEY_check_fips| and the rest of the computation.
// For P-256, 384 and 521, the failure is due to that the coordinates are
// not in Montgomery representation, and, hence, fail |EC_KEY_check_fips|, if in FIPS build;
// or the shared secret computation, otherwise.
ret = ECDH_compute_key_fips(shared_key.data(), shared_key.size(),
EC_KEY_get0_public_key(peer_key.get()), priv_key.get());
if (has_uint128_and_not_small() && (group.get()->curve_name == NID_secp224r1))
{
ASSERT_TRUE(ret);
} else {
ASSERT_FALSE(ret);
if (awslc_fips()) {
// Fails in |EC_KEY_check_fips|.
EXPECT_EQ(EC_R_PUBLIC_KEY_VALIDATION_FAILED,
ERR_GET_REASON(ERR_peek_last_error()));
} else {
// Fails in the actual shared secret computation.
EXPECT_EQ(ECDH_R_POINT_ARITHMETIC_FAILURE,
ERR_GET_REASON(ERR_peek_last_error()));
}
}
ASSERT_TRUE(peer_key.get());
ASSERT_TRUE(pub_key.get());

// Now replace the x-coordinate with the larger one, x+p;
// ECDH fails |EC_KEY_check_fips| or in the actual shared secret computation
// in all curves (except for P-224 in non-FIPS build).
// TODO: Do we want to widen the check the non-FIPS builds?.
OPENSSL_memcpy(peer_key.get()->pub_key->raw.X.bytes, (const uint8_t *)xpp.get()->d, len);
ret = ECDH_compute_key_fips(shared_key.data(), shared_key.size(),
EC_KEY_get0_public_key(peer_key.get()), priv_key.get());
if (!awslc_fips() && has_uint128_and_not_small() && (group.get()->curve_name == NID_secp224r1)) {
ASSERT_TRUE(ret);
} else {
ASSERT_FALSE(ret);
if (awslc_fips()) {
// Fails in |EC_KEY_check_fips|.
EXPECT_EQ(EC_R_PUBLIC_KEY_VALIDATION_FAILED,
ERR_GET_REASON(ERR_peek_last_error()));
} else {
// Fails in the actual shared secret computation.
EXPECT_EQ(ECDH_R_POINT_ARITHMETIC_FAILURE,
ERR_GET_REASON(ERR_peek_last_error()));
}
ASSERT_TRUE(peer_key.get());
ASSERT_TRUE(pub_key.get());
}
});
}

static void RunWycheproofTest(FileTest *t) {
t->IgnoreInstruction("encoding");
Expand Down
41 changes: 23 additions & 18 deletions crypto/fipsmodule/ec/ec_key.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -327,10 +327,6 @@ int EC_KEY_check_key(const EC_KEY *eckey) {
}

static int EVP_EC_KEY_check_fips(EC_KEY *key) {
// We have to avoid the underlying |EVP_DigestSign| and |EVP_DigestVerify|
// services updating the indicator state, so we lock the state here.
FIPS_service_indicator_lock_state();

uint8_t msg[16] = {0};
size_t msg_len = 16;
int ret = 0;
Expand Down Expand Up @@ -363,22 +359,24 @@ static int EVP_EC_KEY_check_fips(EC_KEY *key) {
EVP_PKEY_free(evp_pkey);
OPENSSL_free(sig_der);
EVP_MD_CTX_free(ctx);
FIPS_service_indicator_unlock_state();
if(ret){
FIPS_service_indicator_update_state();
}
return ret;
}

int EC_KEY_check_fips(const EC_KEY *key) {
// We have to avoid the underlying |EVP_DigestSign| and |EVP_DigestVerify|
// services in |EVP_EC_KEY_check_fips| updating the indicator state, so we
// lock the state here.
FIPS_service_indicator_lock_state();
int ret = 0;

if (EC_KEY_is_opaque(key)) {
// Opaque keys can't be checked.
OPENSSL_PUT_ERROR(EC, EC_R_PUBLIC_KEY_VALIDATION_FAILED);
return 0;
goto end;
}

if (!EC_KEY_check_key(key)) {
return 0;
goto end;
}

// Check that the coordinates are within the range [0,p-1], when the (raw)
Expand All @@ -389,34 +387,41 @@ int EC_KEY_check_fips(const EC_KEY *key) {
if(ec_felem_equal(key->pub_key->group, &key->pub_key->group->one, &key->pub_key->raw.Z)) {
BIGNUM *x = BN_new();
BIGNUM *y = BN_new();
int ret = 1;
int check_ret = 1;
if (key->pub_key->group->meth->felem_to_bytes == NULL) {
OPENSSL_PUT_ERROR(EC, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
ret = 0;
check_ret = 0;
} else if (!ec_felem_to_bignum(key->pub_key->group, x, &key->pub_key->raw.X) ||
!ec_felem_to_bignum(key->pub_key->group, y, &key->pub_key->raw.Y)) {
// Error already written to error queue by |bn_wexpand|.
ret = 0;
check_ret = 0;
} else if (BN_is_negative(x) || BN_is_negative(y) ||
BN_cmp(x, &key->pub_key->group->field) >= 0 ||
BN_cmp(y, &key->pub_key->group->field) >= 0) {
OPENSSL_PUT_ERROR(EC, EC_R_COORDINATES_OUT_OF_RANGE);
ret = 0;
check_ret = 0;
}
BN_free(x);
BN_free(y);
if (ret == 0) {
return ret;
if (check_ret == 0) {
goto end;
}
}

if (key->priv_key) {
if (!EVP_EC_KEY_check_fips((EC_KEY*)key)) {
OPENSSL_PUT_ERROR(EC, EC_R_PUBLIC_KEY_VALIDATION_FAILED);
return 0;
goto end;
}
}
return 1;

ret = 1;
end:
FIPS_service_indicator_unlock_state();
if(ret){
FIPS_service_indicator_update_state();
}
return ret;
}

int EC_KEY_set_public_key_affine_coordinates(EC_KEY *key, const BIGNUM *x,
Expand Down
26 changes: 14 additions & 12 deletions crypto/fipsmodule/ec/ec_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1691,6 +1691,14 @@ static bool HasSuffix(const char *str, const char *suffix) {
return strcmp(str + str_len - suffix_len, suffix) == 0;
}

static int has_uint128_and_not_small() {
#if defined(BORINGSSL_HAS_UINT128) && !defined(OPENSSL_SMALL)
return 1;
#else
return 0;
#endif
}

// Test for out-of-range coordinates in public-key validation in
// |EC_KEY_check_fips|, which can only be exercised for P-224 when the
// coordinates in the raw point are not in Montgomery representation.
Expand Down Expand Up @@ -1721,14 +1729,15 @@ TEST(ECTest, LargeXCoordinateVectors) {
// The following call converts the point to Montgomery form for P-256, 384 and 521.
// For P-224, when the functions from simple.c are used, i.e. when
// group->meth = EC_GFp_nistp224_method, the coordinate representation is not changed.
// This is determined based on compile flags in ec.c that are also used below.
// This is determined based on compile flags in ec.c that are also used below
// in has_uint128_and_not_small().
ASSERT_TRUE(EC_POINT_set_affine_coordinates_GFp(
group.get(), pub_key.get(), x.get(), y.get(), nullptr));
ASSERT_TRUE(EC_KEY_set_public_key(key.get(), pub_key.get()));
ASSERT_TRUE(EC_KEY_check_fips(key.get()));

// Set the raw point directly with the BIGNUM coordinates.
// Note that both are in big-endian byte order.
// Note that both are in little-endian byte order.
OPENSSL_memcpy(key.get()->pub_key->raw.X.bytes, (const uint8_t *)x.get()->d, len);
OPENSSL_memcpy(key.get()->pub_key->raw.Y.bytes, (const uint8_t *)y.get()->d, len);
OPENSSL_memset(key.get()->pub_key->raw.Z.bytes, 0, len);
Expand All @@ -1739,39 +1748,32 @@ TEST(ECTest, LargeXCoordinateVectors) {
// not in Montgomery representation, hence the checks fail earlier in
// |EC_KEY_check_key| in the point-on-the-curve calculations, which use
// Montgomery arithmetic.
#if defined(BORINGSSL_HAS_UINT128) && !defined(OPENSSL_SMALL)
if (group.get()->curve_name == NID_secp224r1)
if (has_uint128_and_not_small() && (group.get()->curve_name == NID_secp224r1))
{
ASSERT_TRUE(EC_KEY_check_fips(key.get()));
} else {
#endif
ASSERT_FALSE(EC_KEY_check_fips(key.get()));
EXPECT_EQ(EC_R_POINT_IS_NOT_ON_CURVE,
ERR_GET_REASON(ERR_peek_last_error_line(&file, &line)));
EXPECT_PRED2(HasSuffix, file, "ec_key.c"); // within EC_KEY_check_key
#if defined(BORINGSSL_HAS_UINT128) && !defined(OPENSSL_SMALL)
}
#endif

// Now replace the x-coordinate with the larger one, x+p.
OPENSSL_memcpy(key.get()->pub_key->raw.X.bytes, (const uint8_t *)xpp.get()->d, len);
ASSERT_FALSE(EC_KEY_check_fips(key.get()));

// |EC_KEY_check_fips| check on coordinate range can only be exercised for P-224
// when the coordinates in the raw point are not in Montgomery representation.
// For the other curves, they fail for the same reason as above.
#if defined(BORINGSSL_HAS_UINT128) && !defined(OPENSSL_SMALL)
if (group.get()->curve_name == NID_secp224r1) {
if (has_uint128_and_not_small() && (group.get()->curve_name == NID_secp224r1)) {
EXPECT_EQ(EC_R_COORDINATES_OUT_OF_RANGE,
ERR_GET_REASON(ERR_peek_last_error_line(&file, &line)));
EXPECT_PRED2(HasSuffix, file, "ec_key.c"); // within EC_KEY_check_fips
} else {
#endif
EXPECT_EQ(EC_R_POINT_IS_NOT_ON_CURVE,
ERR_GET_REASON(ERR_peek_last_error_line(&file, &line)));
EXPECT_PRED2(HasSuffix, file, "ec_key.c"); // within EC_KEY_check_key
#if defined(BORINGSSL_HAS_UINT128) && !defined(OPENSSL_SMALL)
}
#endif
});
}

Expand Down
34 changes: 32 additions & 2 deletions crypto/fipsmodule/ecdh/ecdh.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,45 @@ int ECDH_compute_shared_secret(uint8_t *buf, size_t *buflen, const EC_POINT *pub
return 0;
}

// Lock state here to avoid the underlying |EC_KEY_check_fips| function
// updating the service indicator state unintentionally.
FIPS_service_indicator_lock_state();
int ret = 0;

#if defined(AWSLC_FIPS)
// |EC_KEY_check_fips| is not an expensive operation on an external
// public key.
EC_KEY *key_pub_key = NULL;
key_pub_key = EC_KEY_new();
if (key_pub_key == NULL) {
goto end;
}

if (!EC_KEY_set_group(key_pub_key, group) ||
!EC_KEY_set_public_key(key_pub_key, pub_key) || // Creates a copy of pub_key within key_pub_key
!EC_KEY_check_fips(key_pub_key)) {
OPENSSL_PUT_ERROR(EC, EC_R_PUBLIC_KEY_VALIDATION_FAILED);
goto end;
}
#endif

EC_RAW_POINT shared_point;
if (!ec_point_mul_scalar(group, &shared_point, &pub_key->raw, priv) ||
!ec_get_x_coordinate_as_bytes(group, buf, buflen, *buflen,
&shared_point)) {
OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE);
return 0;
goto end;
}

return 1;
ret = 1;
end:
FIPS_service_indicator_unlock_state();
#if defined(AWSLC_FIPS)
if (key_pub_key != NULL) {
EC_KEY_free(key_pub_key);
}
#endif
return ret;
}

int ECDH_compute_key_fips(uint8_t *out, size_t out_len, const EC_POINT *pub_key,
Expand Down
8 changes: 7 additions & 1 deletion crypto/fipsmodule/service_indicator/service_indicator_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1271,7 +1271,13 @@ TEST_P(ECDSA_ServiceIndicatorTest, ECDSAKeyCheck) {
ASSERT_EQ(approved, ecdsaTestVector.key_check_expect_approved);
CALL_SERVICE_AND_CHECK_APPROVED(approved, ASSERT_TRUE(EC_KEY_check_fips(key.get())));
ASSERT_EQ(approved, ecdsaTestVector.key_check_expect_approved);

// Remove reference to private key in generated key and see if
// |EC_KEY_check_fips| still returns approval for keys with only public keys
// available.
bssl::UniquePtr<EC_KEY> key_only_public(EC_KEY_new_by_curve_name(ecdsaTestVector.nid));
ASSERT_TRUE(EC_KEY_set_public_key(key_only_public.get(), EC_KEY_get0_public_key(key.get())));
CALL_SERVICE_AND_CHECK_APPROVED(approved, ASSERT_TRUE(EC_KEY_check_fips(key_only_public.get())));
ASSERT_EQ(approved, ecdsaTestVector.key_check_expect_approved);

// Test running the EVP_PKEY_keygen interfaces one by one directly, and check
// |EVP_PKEY_keygen| for approval at the end. |EVP_PKEY_keygen_init| should
Expand Down

0 comments on commit 794872f

Please sign in to comment.