Fix 32-bit big-endian p521 bug (#1240)

crypto/fipsmodule/ec/ec_test.cc

@@ -2330,3 +2330,28 @@ TEST(ECTest, HashToScalar) {
   EXPECT_FALSE(ec_hash_to_scalar_p384_xmd_sha512_draft07(
       p224.get(), &scalar, kDST, sizeof(kDST), kMessage, sizeof(kMessage)));
 }
+
+TEST(ECTest, FelemBytes) {
+  std::tuple<int,int, int>  test_cases[2] = {
+          std::make_tuple(NID_secp384r1, P384_EC_FELEM_BYTES, P384_EC_FELEM_WORDS),
+          std::make_tuple(NID_secp521r1, P521_EC_FELEM_BYTES, P521_EC_FELEM_WORDS)
+  };
+
+  for(size_t i = 0; i < sizeof(test_cases)/sizeof(std::tuple<int,int,int>); i++) {
+    int nid = std::get<0>(test_cases[i]);
+    int expected_felem_bytes = std::get<1>(test_cases[i]);
+    int expected_felem_words = std::get<2>(test_cases[i]);
+
+    ASSERT_TRUE(expected_felem_bytes <= EC_MAX_BYTES);
+    ASSERT_TRUE(expected_felem_words <= EC_MAX_WORDS);
+    if( 0 == (expected_felem_bytes % BN_BYTES)) {
+      ASSERT_EQ(expected_felem_words, expected_felem_bytes / BN_BYTES);
+    } else {
+      ASSERT_EQ(expected_felem_words, 1 + (expected_felem_bytes / BN_BYTES));
+    }
+
+    bssl::UniquePtr<EC_GROUP> test_group(EC_GROUP_new_by_curve_name(nid));
+    ASSERT_TRUE(test_group);
+    ASSERT_EQ(test_group.get()->field.width, expected_felem_words);
+  }
+}

crypto/fipsmodule/ec/internal.h

@@ -109,7 +109,7 @@ OPENSSL_STATIC_ASSERT(EC_MAX_WORDS <= BN_SMALL_MAX_WORDS,
 // An EC_SCALAR is an integer fully reduced modulo the order. Only the first
 // |order->width| words are used. An |EC_SCALAR| is specific to an |EC_GROUP|
 // and must not be mixed between groups.
-typedef union {
+typedef struct {
   // words is the representation of the scalar in little-endian order.
   BN_ULONG words[EC_MAX_WORDS];
 } EC_SCALAR;
@@ -196,6 +196,12 @@ void ec_scalar_select(const EC_GROUP *group, EC_SCALAR *out, BN_ULONG mask,
 
 // Field elements.
 
+#define P384_EC_FELEM_BYTES (48)
+#define P384_EC_FELEM_WORDS ((P384_EC_FELEM_BYTES + BN_BYTES - 1) / BN_BYTES)
+
+#define P521_EC_FELEM_BYTES (66)
+#define P521_EC_FELEM_WORDS ((P521_EC_FELEM_BYTES + BN_BYTES - 1) / BN_BYTES)
+
 // An EC_FELEM represents a field element. Only the first |field->width| words
 // are used. An |EC_FELEM| is specific to an |EC_GROUP| and must not be mixed
 // between groups. Additionally, the representation (whether or not elements are

crypto/fipsmodule/ec/p384.c

@@ -143,7 +143,6 @@ static p384_limb_t p384_felem_nz(const p384_limb_t in1[P384_NLIMBS]) {
 
 #endif // P384_USE_S2N_BIGNUM_FIELD_ARITH
 
-#define P384_FELEM_BYTES (48)
 
 static void p384_felem_copy(p384_limb_t out[P384_NLIMBS],
                            const p384_limb_t in1[P384_NLIMBS]) {
@@ -164,8 +163,8 @@ static void p384_felem_cmovznz(p384_limb_t out[P384_NLIMBS],
 
 static void p384_from_generic(p384_felem out, const EC_FELEM *in) {
 #ifdef OPENSSL_BIG_ENDIAN
-  uint8_t tmp[P384_FELEM_BYTES];
-  bn_words_to_little_endian(tmp, P384_FELEM_BYTES, in->words, P384_NLIMBS);
+  uint8_t tmp[P384_EC_FELEM_BYTES];
+  bn_words_to_little_endian(tmp, P384_EC_FELEM_BYTES, in->words, P384_EC_FELEM_WORDS);
   p384_felem_from_bytes(out, tmp);
 #else
   p384_felem_from_bytes(out, (const uint8_t *)in->words);
@@ -178,20 +177,19 @@ static void p384_to_generic(EC_FELEM *out, const p384_felem in) {
   OPENSSL_STATIC_ASSERT(
       384 / 8 == sizeof(BN_ULONG) * ((384 + BN_BITS2 - 1) / BN_BITS2),
       p384_felem_to_bytes_leaves_bytes_uninitialized);
-
 #ifdef OPENSSL_BIG_ENDIAN
-  uint8_t tmp[P384_FELEM_BYTES];
+  uint8_t tmp[P384_EC_FELEM_BYTES];
   p384_felem_to_bytes(tmp, in);
-  bn_little_endian_to_words(out->words, P384_NLIMBS, tmp, P384_FELEM_BYTES);
+  bn_little_endian_to_words(out->words, P384_EC_FELEM_WORDS, tmp, P384_EC_FELEM_BYTES);
 #else
   p384_felem_to_bytes((uint8_t *)out->words, in);
 #endif
 }
 
 static void p384_from_scalar(p384_felem out, const EC_SCALAR *in) {
 #ifdef OPENSSL_BIG_ENDIAN
-  uint8_t tmp[P384_FELEM_BYTES];
-  bn_words_to_little_endian(tmp, P384_FELEM_BYTES, in->words, P384_NLIMBS);
+  uint8_t tmp[P384_EC_FELEM_BYTES];
+  bn_words_to_little_endian(tmp, P384_EC_FELEM_BYTES, in->words, P384_EC_FELEM_WORDS);
   p384_felem_from_bytes(out, tmp);
 #else
   p384_felem_from_bytes(out, (const uint8_t *)in->words);

crypto/fipsmodule/ec/p521.c

@@ -176,8 +176,6 @@ static const p521_limb_t p521_felem_p[P521_NLIMBS] = {
 
 #endif // P521_USE_S2N_BIGNUM_FIELD_ARITH
 
-#define P521_FELEM_BYTES (66)
-
 static p521_limb_t p521_felem_nz(const p521_limb_t in1[P521_NLIMBS]) {
   p521_limb_t is_not_zero = 0;
   for (int i = 0; i < P521_NLIMBS; i++) {
@@ -219,8 +217,8 @@ static void p521_felem_cmovznz(p521_limb_t out[P521_NLIMBS],
 // NOTE: the input and output are in little-endian representation.
 static void p521_from_generic(p521_felem out, const EC_FELEM *in) {
 #ifdef OPENSSL_BIG_ENDIAN
-  uint8_t tmp[P521_FELEM_BYTES];
-  bn_words_to_little_endian(tmp, P521_FELEM_BYTES, in->words, P521_NLIMBS);
+  uint8_t tmp[P521_EC_FELEM_BYTES];
+  bn_words_to_little_endian(tmp, P521_EC_FELEM_BYTES, in->words, P521_EC_FELEM_WORDS);
   p521_felem_from_bytes(out, tmp);
 #else
   p521_felem_from_bytes(out, (const uint8_t *)in->words);
@@ -238,9 +236,9 @@ static void p521_to_generic(EC_FELEM *out, const p521_felem in) {
   // extra 6 bytes are zeroed out. To avoid confusion over 32 vs. 64 bit
   // systems and Fiat's vs. ours representation we zero out the whole element.
 #ifdef OPENSSL_BIG_ENDIAN
-  uint8_t tmp[P521_FELEM_BYTES];
+  uint8_t tmp[P521_EC_FELEM_BYTES];
   p521_felem_to_bytes(tmp, in);
-  bn_little_endian_to_words(out->words, P521_NLIMBS, tmp, P521_FELEM_BYTES);
+  bn_little_endian_to_words(out->words, P521_EC_FELEM_WORDS, tmp, P521_EC_FELEM_BYTES);
 #else
   OPENSSL_memset((uint8_t*)out->words, 0, sizeof(out->words));
   // Convert the element to bytes.