Skip to content

Commit

Permalink
Fix flaky ssl BadKemKeyShare tests (#1876)
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
dkostic committed Sep 25, 2024
1 parent fbeb5e8 commit 8970f68
Showing 1 changed file with 22 additions and 23 deletions.
45 changes: 22 additions & 23 deletions ssl/ssl_test.cc
View file
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 random 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 @@ -12115,17 +12118,15 @@ TEST_P(BadKemKeyShareFinishTest, BadKemKeyShareFinish) {
// would fail because the client secret does not match the server secret.
{
// 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;
// a call to Accept(). Here we modify it by replacing it with a randomly
// generated public key that is incompatible with the secret key
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 +12136,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 8970f68

Please sign in to comment.