Skip to content

Commit

Permalink
Fix flaky ssl BadKemKeyShare tests
Browse files Browse the repository at this point in the history
The way the test was generating a public key that is not
consistent with a secret key is by xor-ing the first byte
of the key with 1. Such key modifications can inadvertently
make the key invalid and thus fail the test.

For example, before performing encapsulation ML-KEM decodes
the public key bytes to an array of 12-bit coefficients and
checks that all coefficients are in the range [0, 3328].
If the first two bytes of the key encode the coefficient 3328
then xor-ing the first byte with 1 will make the coefficient
equal to 3329. The call to encapsulate will then fail because
3329 is an invalid coefficient.
  • Loading branch information
dkostic committed Sep 25, 2024
1 parent c5d3f3d commit 4754546
Showing 1 changed file with 20 additions and 22 deletions.
42 changes: 20 additions & 22 deletions ssl/ssl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11966,30 +11966,29 @@ TEST_P(BadKemKeyShareAcceptTest, BadKemKeyShareAccept) {
{
bssl::UniquePtr<SSLKeyShare> server_key_share = bssl::SSLKeyShare::Create(t.group_id);
bssl::UniquePtr<SSLKeyShare> client_key_share = bssl::SSLKeyShare::Create(t.group_id);
bssl::UniquePtr<SSLKeyShare> random_key_share = bssl::SSLKeyShare::Create(t.group_id);
ASSERT_TRUE(server_key_share);
ASSERT_TRUE(client_key_share);
ASSERT_TRUE(random_key_share);
uint8_t server_alert = 0;
uint8_t client_alert = 0;
Array<uint8_t> server_secret;
Array<uint8_t> client_secret;
CBB server_out_public_key;
CBB client_out_public_key;
CBB random_out_public_key;

// Start by having the client Offer() its public key
EXPECT_TRUE(CBB_init(&client_out_public_key, t.offer_key_share_size));
EXPECT_TRUE(client_key_share->Offer(&client_out_public_key));

// Modify the public key making it incompatible with the secret key
uint8_t *modified_client_public_key_buf =
(uint8_t *)OPENSSL_malloc(t.offer_key_share_size);
ASSERT_TRUE(modified_client_public_key_buf);
const uint8_t *client_out_public_key_data = CBB_data(&client_out_public_key);
ASSERT_TRUE(client_out_public_key_data);
OPENSSL_memcpy(modified_client_public_key_buf, client_out_public_key_data,
t.offer_key_share_size);
modified_client_public_key_buf[0] ^= 1;
// Generate a radnom public key that is incompatible with client's secret key
EXPECT_TRUE(CBB_init(&random_out_public_key, t.offer_key_share_size));
EXPECT_TRUE(random_key_share->Offer(&random_out_public_key));
const uint8_t *random_out_public_key_data = CBB_data(&random_out_public_key);
ASSERT_TRUE(random_out_public_key_data);
Span<const uint8_t> client_public_key =
MakeConstSpan(modified_client_public_key_buf, t.offer_key_share_size);
MakeConstSpan(random_out_public_key_data, t.offer_key_share_size);

// When the server calls Accept() with the modified public key, it will
// return success
Expand All @@ -12015,9 +12014,9 @@ TEST_P(BadKemKeyShareAcceptTest, BadKemKeyShareAccept) {

EXPECT_EQ(server_alert, 0);
EXPECT_EQ(client_alert, 0);
OPENSSL_free(modified_client_public_key_buf);
CBB_cleanup(&server_out_public_key);
CBB_cleanup(&client_out_public_key);
CBB_cleanup(&random_out_public_key);
}
}

Expand Down Expand Up @@ -12060,10 +12059,13 @@ TEST_P(BadKemKeyShareFinishTest, BadKemKeyShareFinish) {
// Set up the client and server states for the remaining tests
bssl::UniquePtr<SSLKeyShare> server_key_share = bssl::SSLKeyShare::Create(t.group_id);
bssl::UniquePtr<SSLKeyShare> client_key_share = bssl::SSLKeyShare::Create(t.group_id);
bssl::UniquePtr<SSLKeyShare> random_key_share = bssl::SSLKeyShare::Create(t.group_id);
ASSERT_TRUE(server_key_share);
ASSERT_TRUE(client_key_share);
ASSERT_TRUE(random_key_share);
CBB client_out_public_key;
CBB server_out_public_key;
CBB random_out_public_key;
Array<uint8_t> server_secret;
Array<uint8_t> client_secret;
uint8_t client_alert = 0;
Expand All @@ -12073,6 +12075,7 @@ TEST_P(BadKemKeyShareFinishTest, BadKemKeyShareFinish) {

EXPECT_TRUE(CBB_init(&client_out_public_key, t.offer_key_share_size));
EXPECT_TRUE(CBB_init(&server_out_public_key, t.accept_key_share_size));
EXPECT_TRUE(CBB_init(&random_out_public_key, t.accept_key_share_size));
EXPECT_TRUE(client_key_share->Offer(&client_out_public_key));
const uint8_t *client_out_public_key_data = CBB_data(&client_out_public_key);
ASSERT_TRUE(client_out_public_key_data);
Expand Down Expand Up @@ -12116,16 +12119,13 @@ TEST_P(BadKemKeyShareFinishTest, BadKemKeyShareFinish) {
{
// The server's public key was already correctly generated previously in
// a call to Accept(). Here we modify it.
uint8_t *invalid_server_public_key_buf = (uint8_t *) OPENSSL_malloc(t.accept_key_share_size);
ASSERT_TRUE(invalid_server_public_key_buf);
const uint8_t *server_out_public_key_data = CBB_data(&server_out_public_key);
ASSERT_TRUE(server_out_public_key_data);
OPENSSL_memcpy(invalid_server_public_key_buf, server_out_public_key_data, t.accept_key_share_size);
invalid_server_public_key_buf[0] ^= 1;
EXPECT_TRUE(random_key_share->Offer(&random_out_public_key));
const uint8_t *random_out_public_key_data = CBB_data(&random_out_public_key);
ASSERT_TRUE(random_out_public_key_data);
server_public_key =
MakeConstSpan(random_out_public_key_data, t.accept_key_share_size);

// The call to Finish() will return success
server_public_key =
MakeConstSpan(invalid_server_public_key_buf, t.accept_key_share_size);
EXPECT_TRUE(client_key_share->Finish(&client_secret, &client_alert, server_public_key));
EXPECT_EQ(client_alert, 0);

Expand All @@ -12135,13 +12135,11 @@ TEST_P(BadKemKeyShareFinishTest, BadKemKeyShareFinish) {

// ... but they are not equal
EXPECT_NE(Bytes(client_secret), Bytes(server_secret));


OPENSSL_free(invalid_server_public_key_buf);
}

CBB_cleanup(&server_out_public_key);
CBB_cleanup(&client_out_public_key);
CBB_cleanup(&random_out_public_key);
}

class HybridKeyShareTest : public testing::TestWithParam<HybridGroupTest> {};
Expand Down

0 comments on commit 4754546

Please sign in to comment.