diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc index 47ff60da15..5ed3d5906c 100644 --- a/crypto/pkcs7/pkcs7_test.cc +++ b/crypto/pkcs7/pkcs7_test.cc @@ -1695,9 +1695,14 @@ TEST(PKCS7Test, TestEnveloped) { bssl::UniquePtr bio; bssl::UniquePtr certs; bssl::UniquePtr 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_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())); }