From 8970f685ed0fa542a1ca09f3d7ee4663812f7299 Mon Sep 17 00:00:00 2001 From: dkostic <25055813+dkostic@users.noreply.github.com> Date: Wed, 25 Sep 2024 15:16:14 -0700 Subject: [PATCH] Fix flaky ssl BadKemKeyShare tests (#1876) 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. --- ssl/ssl_test.cc | 45 ++++++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index e61937cc67..2c4b82483a 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -11966,30 +11966,29 @@ TEST_P(BadKemKeyShareAcceptTest, BadKemKeyShareAccept) { { bssl::UniquePtr server_key_share = bssl::SSLKeyShare::Create(t.group_id); bssl::UniquePtr client_key_share = bssl::SSLKeyShare::Create(t.group_id); + bssl::UniquePtr 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 server_secret; Array 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 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 @@ -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); } } @@ -12060,10 +12059,13 @@ TEST_P(BadKemKeyShareFinishTest, BadKemKeyShareFinish) { // Set up the client and server states for the remaining tests bssl::UniquePtr server_key_share = bssl::SSLKeyShare::Create(t.group_id); bssl::UniquePtr client_key_share = bssl::SSLKeyShare::Create(t.group_id); + bssl::UniquePtr 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 server_secret; Array client_secret; uint8_t client_alert = 0; @@ -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); @@ -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); @@ -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 {};