Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 32-bit big-endian p521 bug #1240

Merged
merged 7 commits into from
Oct 18, 2023
Merged

Fix 32-bit big-endian p521 bug #1240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b93e55b
Fix 32-bit big-endian p521 bug
justsmth Oct 10, 2023
94de9ea
Define PXXX_FELEM_WORDS macros
justsmth Oct 11, 2023
d0fed29
EC_SCALAR should be a struct
justsmth Oct 11, 2023
a37035e
Per PR comments
justsmth Oct 17, 2023
c089d64
Improve test
justsmth Oct 17, 2023
e0d8b00
Add another check
justsmth Oct 17, 2023
89d0058
Scale back the test
justsmth Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions crypto/fipsmodule/ec/ec_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
8 changes: 7 additions & 1 deletion crypto/fipsmodule/ec/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been done as part of the upstream merge of #1177.

// words is the representation of the scalar in little-endian order.
BN_ULONG words[EC_MAX_WORDS];
} EC_SCALAR;
Expand Down Expand Up @@ -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
Expand Down
14 changes: 6 additions & 8 deletions crypto/fipsmodule/ec/p384.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]) {
Expand All @@ -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);
Expand All @@ -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);
Expand Down
10 changes: 4 additions & 6 deletions crypto/fipsmodule/ec/p521.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Expand Down Expand Up @@ -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);
Expand All @@ -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.
Expand Down
Loading