P-521 implementation with Fiat-crypto field arithmetic

crypto/ecdh_extra/ecdh_test.cc

@@ -171,39 +171,49 @@ TEST(ECDHTest, InvalidPubKeyLargeCoord) {
     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)?
+    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.
+    // The following call converts the point to Montgomery form for P-256/384.
     // 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().
+    // 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().
+    // For P-521, the plain non-Motgomery representation is always used.
     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()));
+    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_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.
+
+    // As mentioned, for P-224 and P-521, setting the raw point directly
+    // with the coordinates still passes |EC_KEY_check_fips|.
+    // For P-256 and 384, the failure is due to that the coordinates are
+    // 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.
     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))
-    {
+                                EC_KEY_get0_public_key(peer_key.get()),
+                                priv_key.get());
+
+    int curve_nid = group.get()->curve_name;
+    if ((has_uint128_and_not_small() && (curve_nid == NID_secp224r1)) ||
+        (curve_nid == NID_secp521r1)) {
       ASSERT_TRUE(ret);
     } else {
       ASSERT_FALSE(ret);
@@ -222,12 +232,18 @@ TEST(ECDHTest, InvalidPubKeyLargeCoord) {
 
     // 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);
+    // in all curves (except for P-224 and P-521 in non-FIPS build).
+    // TODO: Do we want to change the code to apply the FIPS check to non-FIPS
+    // builds, or we can allow the coordinates to be larger than the modulus
+    // as long as they're correct?
+    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)) {
+                                EC_KEY_get0_public_key(peer_key.get()),
+                                priv_key.get());
+    if (!awslc_fips() &&
+        ((has_uint128_and_not_small() && (curve_nid == NID_secp224r1)) ||
+        (curve_nid == NID_secp521r1))) {
       ASSERT_TRUE(ret);
     } else {
       ASSERT_FALSE(ret);

crypto/fipsmodule/bcm.c

@@ -86,6 +86,7 @@
 #include "ec/p256.c"
 #include "ec/p256-nistz.c"
 #include "ec/p384.c"
+#include "ec/p521.c"
 #include "ec/scalar.c"
 #include "ec/simple.c"
 #include "ec/simple_mul.c"

crypto/fipsmodule/ec/ec.c

@@ -224,7 +224,7 @@ DEFINE_METHOD_FUNCTION(struct built_in_curves, OPENSSL_built_in_curves) {
   out->curves[0].comment = "NIST P-521";
   out->curves[0].param_len = 66;
   out->curves[0].params = kP521Params;
-  out->curves[0].method = EC_GFp_mont_method();
+  out->curves[0].method = EC_GFp_nistp521_method();
 
   // 1.3.132.0.34
   static const uint8_t kOIDP384[] = {0x2b, 0x81, 0x04, 0x00, 0x22};
@@ -839,6 +839,7 @@ int ec_point_set_affine_coordinates(const EC_GROUP *group, EC_AFFINE *out,
   ec_felem_add(group, &rhs, &rhs, &group->a);  // rhs = x^2 + a
   felem_mul(group, &rhs, &rhs, x);             // rhs = x^3 + ax
   ec_felem_add(group, &rhs, &rhs, &group->b);  // rhs = x^3 + ax + b
+
   if (!ec_felem_equal(group, &lhs, &rhs)) {
     OPENSSL_PUT_ERROR(EC, EC_R_POINT_IS_NOT_ON_CURVE);
     // In the event of an error, defend against the caller not checking the

crypto/fipsmodule/ec/ec_key.c

@@ -384,20 +384,22 @@ int EC_KEY_check_fips(const EC_KEY *key) {
   // This is the case when validating a received public key.
   // Note: The check for x and y being negative seems superfluous since
   // ec_felem_to_bignum() calls BN_bin2bn() which sets the `neg` flag to 0.
-  if(ec_felem_equal(key->pub_key->group, &key->pub_key->group->one, &key->pub_key->raw.Z)) {
+  EC_POINT *pub_key = key->pub_key;
+  EC_GROUP *group = key->pub_key->group;
+  if(ec_felem_equal(group, &group->one, &pub_key->raw.Z)) {
     BIGNUM *x = BN_new();
     BIGNUM *y = BN_new();
     int check_ret = 1;
-    if (key->pub_key->group->meth->felem_to_bytes == NULL) {
+    if (group->meth->felem_to_bytes == NULL) {
       OPENSSL_PUT_ERROR(EC, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
       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)) {
+    } else if (!ec_felem_to_bignum(group, x, &pub_key->raw.X) ||
+               !ec_felem_to_bignum(group, y, &pub_key->raw.Y)) {
       // Error already written to error queue by |bn_wexpand|.
       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) {
+               BN_cmp(x, &group->field) >= 0 ||
+               BN_cmp(y, &group->field) >= 0) {
       OPENSSL_PUT_ERROR(EC, EC_R_COORDINATES_OUT_OF_RANGE);
       check_ret = 0;
     }

crypto/fipsmodule/ec/ec_test.cc

@@ -1700,8 +1700,9 @@ static int has_uint128_and_not_small() {
 }
 
 // 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.
+// |EC_KEY_check_fips|. This test can only be exercised when the coordinates 
+// in the raw point are not in Montgomery representation, which is the case 
+// for P-224 in some builds (see below) and for P-521.
 TEST(ECTest, LargeXCoordinateVectors) {
   int line;
   const char *file;
@@ -1726,30 +1727,35 @@ TEST(ECTest, LargeXCoordinateVectors) {
 
     size_t len = BN_num_bytes(&group.get()->field); // Modulus byte-length
     ASSERT_TRUE(EC_KEY_set_group(key.get(), group.get()));
-    // The following call converts the point to Montgomery form for P-256, 384 and 521.
+    // The following call converts the point to Montgomery form for P-256/384.
     // 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().
+    // 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().
+    // For P-521, the plain non-Motgomery representation is always used.
     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 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_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);
     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|.
-    // For P-256, 384 and 521, the failure is due to that the coordinates are
+
+    // As mentioned, for P-224 and P-521, setting the raw point directly
+    // with the coordinates still passes |EC_KEY_check_fips|.
+    // For P-256 and 384, the failure is due to that the coordinates are
     // 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 (has_uint128_and_not_small() && (group.get()->curve_name == NID_secp224r1))
-    {
+    int curve_nid = group.get()->curve_name;
+    if ((has_uint128_and_not_small() && (curve_nid == NID_secp224r1)) ||
+        (curve_nid == NID_secp521r1)) {
       ASSERT_TRUE(EC_KEY_check_fips(key.get()));
     } else {
       ASSERT_FALSE(EC_KEY_check_fips(key.get()));
@@ -1759,13 +1765,16 @@ TEST(ECTest, LargeXCoordinateVectors) {
     }
 
     // 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);
+    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 (has_uint128_and_not_small() && (group.get()->curve_name == NID_secp224r1)) {
+    // |EC_KEY_check_fips| check on coordinate range can only be exercised
+    // for P-224 and P-521 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 ((has_uint128_and_not_small() && (curve_nid == NID_secp224r1)) ||
+        (curve_nid == NID_secp521r1)) {
       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

crypto/fipsmodule/ec/internal.h

@@ -720,6 +720,7 @@ void ec_GFp_nistp_recode_scalar_bits(crypto_word_t *sign, crypto_word_t *digit,
 const EC_METHOD *EC_GFp_nistp224_method(void);
 const EC_METHOD *EC_GFp_nistp256_method(void);
 const EC_METHOD *EC_GFp_nistp384_method(void);
+const EC_METHOD *EC_GFp_nistp521_method(void);
 
 // EC_GFp_nistz256_method is a GFp method using montgomery multiplication, with
 // x86-64 optimized P256. See http://eprint.iacr.org/2013/816.