Skip to content

Commit

Permalink
Add MMA test cases, doc comments, style tweaks
Browse files Browse the repository at this point in the history
  • Loading branch information
@WillChilds-Klein
WillChilds-Klein committed Nov 18, 2024
1 parent c5feaf6 commit 86d8fc1
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 26 deletions.
62 changes: 38 additions & 24 deletions crypto/pkcs7/pkcs7.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1210,10 +1210,11 @@ static int pkcs7_decrypt_rinfo(unsigned char **ek_out, PKCS7_RECIP_INFO *ri,
}

// Do not update |ret| on decryption failure, simply null out |*pek|
int ok = EVP_PKEY_decrypt(ctx, ek, &len, ri->enc_key->data,
ri->enc_key->length);
int ok =
EVP_PKEY_decrypt(ctx, ek, &len, ri->enc_key->data, ri->enc_key->length);
if (!ok) {
*ek_out = NULL;
OPENSSL_free(ek);
ek = NULL;
}

ret = 1;
Expand All @@ -1236,8 +1237,7 @@ static int pkcs7_cmp_ri(PKCS7_RECIP_INFO *ri, X509 *pcert) {
ri->issuer_and_serial->serial);
}

static BIO *pkcs7_data_decode(PKCS7 *p7, EVP_PKEY *pkey, BIO *in_bio,
X509 *pcert) {
static BIO *pkcs7_data_decode(PKCS7 *p7, EVP_PKEY *pkey, X509 *pcert) {
GUARD_PTR(p7);
GUARD_PTR(pkey);
BIO *out = NULL, *cipher_bio = NULL, *data_bio = NULL;
Expand Down Expand Up @@ -1270,17 +1270,22 @@ static BIO *pkcs7_data_decode(PKCS7 *p7, EVP_PKEY *pkey, BIO *in_bio,
goto err;
}

// Detached content must be supplied via |in_bio|
if (data_body == NULL && in_bio == NULL) {
OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_NO_CONTENT);
goto err;
}

if ((cipher_bio = BIO_new(BIO_f_cipher())) == NULL) {
OPENSSL_PUT_ERROR(PKCS7, ERR_R_BIO_LIB);
goto err;
}

// RFC 3218 provides an overview of, and mitigations for, the "Million Message
// Attack" (MMA) on RSA encryption with PKCS-1 padding. Section 2.3 describes
// implementor countermeasures. We implement the following countermeasures, as
// does OpenSSL.
//
// 1. Do not branch on |cek| decryption failure when checking recip infos
// 2. Clear error state after |cek| decrypt is attempted
// 3. If no cek was decrypted, use same-size random bytes
// to output gibberish "plaintext"
// 4. Always pay same allocation costs, regardless of |cek| decrypt result

// If |pcert| was specified, find the matching recipient info
if (pcert) {
for (size_t ii = 0; ii < sk_PKCS7_RECIP_INFO_num(rsk); ii++) {
Expand Down Expand Up @@ -1320,7 +1325,7 @@ static BIO *pkcs7_data_decode(PKCS7 *p7, EVP_PKEY *pkey, BIO *in_bio,
}
}
}
// Clear any potential decryption errors
// Clear any decryption errors to minimize behavioral difference under MMA
ERR_clear_error();

EVP_CIPHER_CTX *evp_ctx = NULL;
Expand All @@ -1336,7 +1341,7 @@ static BIO *pkcs7_data_decode(PKCS7 *p7, EVP_PKEY *pkey, BIO *in_bio,
OPENSSL_PUT_ERROR(PKCS7, ERR_R_PKCS7_LIB);
goto err;
}
if (EVP_CipherInit_ex(evp_ctx, NULL, NULL, NULL, iv, 0) <= 0) {
if (!EVP_CipherInit_ex(evp_ctx, NULL, NULL, NULL, iv, 0)) {
goto err;
}
// Get the key length from cipher context so we don't condition on |cek_len|
Expand Down Expand Up @@ -1380,6 +1385,7 @@ static BIO *pkcs7_data_decode(PKCS7 *p7, EVP_PKEY *pkey, BIO *in_bio,

err:
OPENSSL_free(cek);
OPENSSL_free(dummy_key);
BIO_free_all(out);
BIO_free_all(cipher_bio);
BIO_free_all(data_bio);
Expand All @@ -1397,37 +1403,45 @@ PKCS7_RECIP_INFO *PKCS7_add_recipient(PKCS7 *p7, X509 *x509) {
}

int PKCS7_decrypt(PKCS7 *p7, EVP_PKEY *pkey, X509 *cert, BIO *data, int flags) {
BIO *bio;
BIO *bio = NULL;
int ret = 0;

if (p7 == NULL) {
OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_INVALID_NULL_POINTER);
return 0;
goto err;
}

if (!PKCS7_type_is_enveloped(p7) && !PKCS7_type_is_signedAndEnveloped(p7)) {
OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_WRONG_CONTENT_TYPE);
return 0;
switch (OBJ_obj2nid(p7->type)) {
case NID_pkcs7_enveloped:
case NID_pkcs7_signedAndEnveloped:
break;
default:
OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_WRONG_CONTENT_TYPE);
goto err;
}

if (cert && !X509_check_private_key(cert, pkey)) {
OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_PRIVATE_KEY_DOES_NOT_MATCH_CERTIFICATE);
return 0;
goto err;
}

if ((bio = pkcs7_data_decode(p7, pkey, NULL, cert)) == NULL ||
if ((bio = pkcs7_data_decode(p7, pkey, cert)) == NULL ||
!pkcs7_bio_copy_content(bio, data)) {
OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_DECRYPT_ERROR);
return 0;
goto err;
}

// Check whether decryption was successful
// Check whether content decryption was successful
OPENSSL_BEGIN_ALLOW_DEPRECATED
if (!BIO_get_cipher_status(bio)) {
OPENSSL_END_ALLOW_DEPRECATED
OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_DECRYPT_ERROR);
return 0;
goto err;
}

ret = 1;

err:
BIO_free_all(bio);
return 1;
return ret;
}
69 changes: 67 additions & 2 deletions crypto/pkcs7/pkcs7_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1694,18 +1694,83 @@ TEST(PKCS7Test, TestEnveloped) {
BIO_read(bio.get(), decrypted, sizeof(decrypted)));
EXPECT_EQ(Bytes(buf, sizeof(buf)), Bytes(decrypted, sizeof(decrypted)));

// no certs provided
// no certs provided for decryption
bio.reset(BIO_new_mem_buf(buf, sizeof(buf)));
p7.reset(
PKCS7_encrypt(certs.get(), bio.get(), EVP_aes_128_cbc(), /*flags*/ 0));
EXPECT_TRUE(p7);
EXPECT_TRUE(PKCS7_type_is_enveloped(p7.get()));
bio.reset(BIO_new(BIO_s_mem()));
EXPECT_TRUE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), nullptr, bio.get(),
EXPECT_TRUE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), /*certs*/ nullptr,
bio.get(),
/*flags*/ 0));
EXPECT_EQ(sizeof(decrypted), BIO_pending(bio.get()));
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)));

// empty plaintext
bio.reset(BIO_new(BIO_s_mem()));
ASSERT_TRUE(BIO_set_mem_eof_return(bio.get(), 0));
p7.reset(
PKCS7_encrypt(certs.get(), bio.get(), EVP_aes_128_cbc(), /*flags*/ 0));
EXPECT_TRUE(p7);
EXPECT_TRUE(PKCS7_type_is_enveloped(p7.get()));
bio.reset(BIO_new(BIO_s_mem()));
ASSERT_TRUE(BIO_set_mem_eof_return(bio.get(), 0));
EXPECT_TRUE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), rsa_x509.get(), bio.get(),
/*flags*/ 0));
EXPECT_EQ(0UL, BIO_pending(bio.get()));
EXPECT_EQ(0, BIO_read(bio.get(), decrypted, sizeof(decrypted)));
EXPECT_FALSE(BIO_should_retry(bio.get()));

// 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)));
p7.reset(
PKCS7_encrypt(certs.get(), bio.get(), EVP_aes_128_cbc(), /*flags*/ 0));
EXPECT_TRUE(p7);
EXPECT_TRUE(PKCS7_type_is_enveloped(p7.get()));
bio.reset(BIO_new(BIO_s_mem()));
// set newm RSA key, cert pub key and PKEY private key now mismatch
rsa.reset(RSA_new());
ASSERT_TRUE(RSA_generate_key_fips(rsa.get(), 2048, nullptr));
ASSERT_TRUE(EVP_PKEY_set1_RSA(rsa_pkey.get(), rsa.get()));
// attempt decryption with the new, mismatched keypair. content key decryption
// should "succeed" and produce random, useless content decryption key.
// The content is "decrypted" with the useless key, so nonsense gets written
// to the output |bio|. The cipher ends up in an unhealthy state due to bad
// padding (what should be the final pad block is now just random bytes), so
// the overall |PKCS7_decrypt| operation fails.
EXPECT_FALSE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), /*certs*/ nullptr,
bio.get(),
/*flags*/ 0));
EXPECT_EQ(sizeof(decrypted), BIO_pending(bio.get()));
OPENSSL_cleanse(decrypted, sizeof(decrypted));
ASSERT_EQ((int)sizeof(decrypted),
BIO_read(bio.get(), decrypted, sizeof(decrypted)));
EXPECT_NE(Bytes(buf, sizeof(buf)), Bytes(decrypted, sizeof(decrypted)));
EXPECT_EQ(CIPHER_R_BAD_DECRYPT, ERR_GET_REASON(ERR_peek_error()));

// 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)));
p7.reset(
PKCS7_encrypt(certs.get(), bio.get(), EVP_aes_128_ctr(), /*flags*/ 0));
EXPECT_TRUE(p7);
EXPECT_TRUE(PKCS7_type_is_enveloped(p7.get()));
bio.reset(BIO_new(BIO_s_mem()));
// content decryption "succeeds"...
EXPECT_TRUE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), /*certs*/ nullptr,
bio.get(),
/*flags*/ 0));
EXPECT_EQ(sizeof(decrypted), BIO_pending(bio.get()));
OPENSSL_cleanse(decrypted, sizeof(decrypted));
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_FALSE(ERR_GET_REASON(ERR_peek_error()));
}
4 changes: 4 additions & 0 deletions include/openssl/pkcs7.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,10 @@ OPENSSL_EXPORT OPENSSL_DEPRECATED PKCS7 *PKCS7_encrypt(STACK_OF(X509) *certs, BI
// If |cert| is present, it's public key is checked against |pkey| and |p7|'s
// recipient infos. 1 is returned on success and 0 on failure. |flags| is
// ignored.
//
// NOTE: If |p7| was encrypted with a stream cipher, this operation may return 1
// even on decryption failure. The reason for this is detailed in RFC 3218 and
// comments in the |PKCS7_decrypt| source.
OPENSSL_EXPORT OPENSSL_DEPRECATED int PKCS7_decrypt(PKCS7 *p7, EVP_PKEY *pkey, X509 *cert, BIO *data, int flags);

#if defined(__cplusplus)
Expand Down

0 comments on commit 86d8fc1

Please sign in to comment.