Fix flaky ssl BadKemKeyShare tests (aws#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

@@ -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
@@ -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<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;
@@ -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<HybridGroupTest> {};