From bce94dcca00ee84968f846106e64ab201aea3dd7 Mon Sep 17 00:00:00 2001 From: Justin W Smith <103147162+justsmth@users.noreply.github.com> Date: Wed, 18 Oct 2023 14:26:19 -0400 Subject: [PATCH] Fix 32-bit big-endian p521 bug (#1240) --- crypto/fipsmodule/ec/ec_test.cc | 25 +++++++++++++++++++++++++ crypto/fipsmodule/ec/internal.h | 8 +++++++- crypto/fipsmodule/ec/p384.c | 14 ++++++-------- crypto/fipsmodule/ec/p521.c | 10 ++++------ 4 files changed, 42 insertions(+), 15 deletions(-) diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc index 617488eec1..d797c733c5 100644 --- a/crypto/fipsmodule/ec/ec_test.cc +++ b/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 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); 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 test_group(EC_GROUP_new_by_curve_name(nid)); + ASSERT_TRUE(test_group); + ASSERT_EQ(test_group.get()->field.width, expected_felem_words); + } +} diff --git a/crypto/fipsmodule/ec/internal.h b/crypto/fipsmodule/ec/internal.h index 4d693f678c..2f02080ab1 100644 --- a/crypto/fipsmodule/ec/internal.h +++ b/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 diff --git a/crypto/fipsmodule/ec/p384.c b/crypto/fipsmodule/ec/p384.c index 42f1ee4399..966729f458 100644 --- a/crypto/fipsmodule/ec/p384.c +++ b/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,11 +177,10 @@ 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 @@ -190,8 +188,8 @@ static void p384_to_generic(EC_FELEM *out, const p384_felem in) { 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); diff --git a/crypto/fipsmodule/ec/p521.c b/crypto/fipsmodule/ec/p521.c index ea5982c26e..95ee79acdd 100644 --- a/crypto/fipsmodule/ec/p521.c +++ b/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.