Fix occasional buffer overflow in test

crypto/pkcs7/pkcs7_test.cc

@@ -1695,9 +1695,14 @@ TEST(PKCS7Test, TestEnveloped) {
   bssl::UniquePtr<BIO> bio;
   bssl::UniquePtr<STACK_OF(X509)> certs;
   bssl::UniquePtr<X509> rsa_x509;
-  uint8_t buf[64], decrypted[sizeof(buf)];
+  const size_t pt_len = 64;
+  // NOTE: we make |buf| larger than |pt_len| in case padding gets added.
+  // without the extra room, we sometimes overflow into the next variable on the
+  // stack.
+  uint8_t buf[pt_len + EVP_MAX_BLOCK_LENGTH], decrypted[pt_len];
 
-  OPENSSL_memset(buf, 'A', sizeof(buf));
+  OPENSSL_cleanse(buf, sizeof(buf));
+  OPENSSL_memset(buf, 'A', pt_len);
 
   // parse a cert for use with recipient infos
   bssl::UniquePtr<RSA> rsa(RSA_new());
@@ -1717,7 +1722,7 @@ TEST(PKCS7Test, TestEnveloped) {
   ASSERT_TRUE(X509_set_pubkey(rsa_x509.get(), rsa_pkey.get()));
   X509_up_ref(rsa_x509.get());
 
-  bio.reset(BIO_new_mem_buf(buf, sizeof(buf)));
+  bio.reset(BIO_new_mem_buf(buf, pt_len));
   p7.reset(
       PKCS7_encrypt(certs.get(), bio.get(), EVP_aes_128_cbc(), /*flags*/ 0));
   EXPECT_TRUE(p7);
@@ -1729,10 +1734,10 @@ TEST(PKCS7Test, TestEnveloped) {
   OPENSSL_cleanse(decrypted, sizeof(decrypted));
   ASSERT_EQ((int)sizeof(decrypted),
             BIO_read(bio.get(), decrypted, sizeof(decrypted)));
-  EXPECT_EQ(Bytes(buf, sizeof(buf)), Bytes(decrypted, sizeof(decrypted)));
+  EXPECT_EQ(Bytes(buf, pt_len), Bytes(decrypted, sizeof(decrypted)));
 
   // no certs provided for decryption
-  bio.reset(BIO_new_mem_buf(buf, sizeof(buf)));
+  bio.reset(BIO_new_mem_buf(buf, pt_len));
   p7.reset(
       PKCS7_encrypt(certs.get(), bio.get(), EVP_aes_128_cbc(), /*flags*/ 0));
   EXPECT_TRUE(p7);
@@ -1745,7 +1750,7 @@ TEST(PKCS7Test, TestEnveloped) {
   OPENSSL_cleanse(decrypted, sizeof(decrypted));
   ASSERT_EQ((int)sizeof(decrypted),
             BIO_read(bio.get(), decrypted, sizeof(decrypted)));
-  EXPECT_EQ(Bytes(buf, sizeof(buf)), Bytes(decrypted, sizeof(decrypted)));
+  EXPECT_EQ(Bytes(buf, pt_len), Bytes(decrypted, sizeof(decrypted)));
 
   // empty plaintext
   bio.reset(BIO_new(BIO_s_mem()));
@@ -1764,7 +1769,7 @@ TEST(PKCS7Test, TestEnveloped) {
 
   // test "MMA" decrypt with mismatched cert pub key/pkey private key and block
   // cipher used for content encryption
-  bio.reset(BIO_new_mem_buf(buf, sizeof(buf)));
+  bio.reset(BIO_new_mem_buf(buf, pt_len));
   p7.reset(
       PKCS7_encrypt(certs.get(), bio.get(), EVP_aes_128_cbc(), /*flags*/ 0));
   EXPECT_TRUE(p7);
@@ -1785,18 +1790,18 @@ TEST(PKCS7Test, TestEnveloped) {
                     /*flags*/ 0);
   EXPECT_LE(sizeof(decrypted), BIO_pending(bio.get()));
   OPENSSL_cleanse(decrypted, sizeof(decrypted));
-  // There's a fun edge case here where the MMA defence can fool the block
-  // cipher into thinking the garbled "plaintext" padding is valid thus it's
-  // successfully decrypted the content. For block ciphers using conventional
-  // PKCS#7 padding, where the last byte of the padded plaintext determines how
-  // many bytes of padding have been appended, a random MMA-defense-garbled
-  // plaintext with last byte of 0x01 will trick the cipher into thinking there
-  // is only one byte of padding to remove, leaving the other block_size-1 bytes
-  // in place.
+  // There's a fun edge case here for block ciphers using conventional PKCS#7
+  // padding. In this padding scheme, the last byte of the padded plaintext
+  // determines how many bytes of padding have been appended and must be
+  // stripped, A random MMA-defense-garbled padded plaintext with last byte of
+  // 0x01 will trick the EVP API into thinking that byte is a valid padding
+  // byte, so it (and only it) will be stripped. This leaves the other
+  // block_size-1 bytes of the padding block in place, resulting in a larger
+  // "decrypted plaintext" than anticipated.
   int max_decrypt =
       sizeof(decrypted) + EVP_CIPHER_block_size(EVP_aes_128_cbc());
   int decrypted_len = BIO_read(bio.get(), decrypted, max_decrypt);
-  if (decrypted_len > (int)sizeof(decrypted)) {
+  if (decrypted_len > (int)pt_len) {
     EXPECT_EQ(max_decrypt - 1, decrypted_len);
     EXPECT_TRUE(decrypt_ok);
     EXPECT_FALSE(ERR_GET_REASON(ERR_peek_error()));
@@ -1806,12 +1811,12 @@ TEST(PKCS7Test, TestEnveloped) {
     EXPECT_EQ(CIPHER_R_BAD_DECRYPT, ERR_GET_REASON(ERR_peek_error()));
   }
   // Of course, plaintext shouldn't equal decrypted in any case here
-  EXPECT_NE(Bytes(buf, sizeof(buf)), Bytes(decrypted, sizeof(decrypted)));
+  EXPECT_NE(Bytes(buf, pt_len), Bytes(decrypted, sizeof(decrypted)));
 
   // test "MMA" decrypt as above, but with stream cipher. stream cipher has no
   // padding, so content encryption should "succeed" but return nonsense because
   // the content decryption key is just randomly generated bytes.
-  bio.reset(BIO_new_mem_buf(buf, sizeof(buf)));
+  bio.reset(BIO_new_mem_buf(buf, pt_len));
   p7.reset(
       PKCS7_encrypt(certs.get(), bio.get(), EVP_aes_128_ctr(), /*flags*/ 0));
   EXPECT_TRUE(p7);
@@ -1826,6 +1831,6 @@ TEST(PKCS7Test, TestEnveloped) {
   ASSERT_EQ((int)sizeof(decrypted),
             BIO_read(bio.get(), decrypted, sizeof(decrypted)));
   // ...but it produces pseudo-random nonsense
-  EXPECT_NE(Bytes(buf, sizeof(buf)), Bytes(decrypted, sizeof(decrypted)));
+  EXPECT_NE(Bytes(buf, pt_len), Bytes(decrypted, sizeof(decrypted)));
   EXPECT_FALSE(ERR_GET_REASON(ERR_peek_error()));
 }