From 95e6e00a2bc7a0896e36c8d05630a5e4ad2ab007 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Thu, 7 Nov 2024 20:30:26 +0000 Subject: [PATCH 01/13] Implement PKCS7_encrypt and PKC7_decrypt --- crypto/pkcs7/bio/bio_cipher_test.cc | 3 - crypto/pkcs7/bio/cipher.c | 5 +- crypto/pkcs7/pkcs7.c | 350 +++++++++++++++++++++++++++- crypto/pkcs7/pkcs7_test.cc | 122 ++++++++++ include/openssl/pkcs7.h | 37 ++- 5 files changed, 504 insertions(+), 13 deletions(-) diff --git a/crypto/pkcs7/bio/bio_cipher_test.cc b/crypto/pkcs7/bio/bio_cipher_test.cc index d111d27476..c05ffcf04c 100644 --- a/crypto/pkcs7/bio/bio_cipher_test.cc +++ b/crypto/pkcs7/bio/bio_cipher_test.cc @@ -13,9 +13,6 @@ // NOTE: need to keep this in sync with sizeof(ctx->buf) cipher.c #define ENC_BLOCK_SIZE 1024 * 4 -#define BIO_get_cipher_status(bio) \ - BIO_ctrl(bio, BIO_C_GET_CIPHER_STATUS, 0, NULL) - struct CipherParams { const char name[40]; const EVP_CIPHER *(*cipher)(void); diff --git a/crypto/pkcs7/bio/cipher.c b/crypto/pkcs7/bio/cipher.c index 7d822a492a..ee5c95e7eb 100644 --- a/crypto/pkcs7/bio/cipher.c +++ b/crypto/pkcs7/bio/cipher.c @@ -193,7 +193,6 @@ static int enc_write(BIO *b, const char *in, int inl) { static long enc_ctrl(BIO *b, int cmd, long num, void *ptr) { GUARD_PTR(b); long ret = 1; - BIO_ENC_CTX *ctx = BIO_get_data(b); EVP_CIPHER_CTX **cipher_ctx; BIO *next = BIO_next(b); @@ -326,3 +325,7 @@ const BIO_METHOD *BIO_f_cipher(void) { return &methods_enc; } int BIO_get_cipher_ctx(BIO *b, EVP_CIPHER_CTX **ctx) { return BIO_ctrl(b, BIO_C_GET_CIPHER_CTX, 0, ctx); } + +int BIO_get_cipher_status(BIO *b) { + return BIO_ctrl(b, BIO_C_GET_CIPHER_STATUS, 0, NULL); +} diff --git a/crypto/pkcs7/pkcs7.c b/crypto/pkcs7/pkcs7.c index 984b6b56e9..d846b06c65 100644 --- a/crypto/pkcs7/pkcs7.c +++ b/crypto/pkcs7/pkcs7.c @@ -858,7 +858,7 @@ OPENSSL_END_ALLOW_DEPRECATED } -static BIO *PKCS7_find_digest(EVP_MD_CTX **pmd, BIO *bio, int nid) { +static BIO *pkcs7_find_digest(EVP_MD_CTX **pmd, BIO *bio, int nid) { GUARD_PTR(pmd); while (bio != NULL) { bio = BIO_find_type(bio, BIO_TYPE_MD); @@ -1017,7 +1017,7 @@ OPENSSL_END_ALLOW_DEPRECATED continue; } int sign_nid = OBJ_obj2nid(si->digest_alg->algorithm); - bio_tmp = PKCS7_find_digest(&md_ctx, bio_tmp, sign_nid); + bio_tmp = pkcs7_find_digest(&md_ctx, bio_tmp, sign_nid); if (bio_tmp == NULL) { goto err; } @@ -1046,7 +1046,7 @@ OPENSSL_END_ALLOW_DEPRECATED } else if (OBJ_obj2nid(p7->type) == NID_pkcs7_digest) { unsigned char md_data[EVP_MAX_MD_SIZE]; unsigned int md_len; - if (!PKCS7_find_digest(&md_ctx, bio, EVP_MD_nid(p7->d.digest->md)) || + if (!pkcs7_find_digest(&md_ctx, bio, EVP_MD_nid(p7->d.digest->md)) || !EVP_DigestFinal_ex(md_ctx, md_data, &md_len) || !ASN1_OCTET_STRING_set(p7->d.digest->digest, md_data, md_len)) { goto err; @@ -1085,3 +1085,347 @@ OPENSSL_END_ALLOW_DEPRECATED EVP_MD_CTX_free(md_ctx_tmp); return ret; } + +// pkcs7_bio_copy_content copies the contents of |src| into |dst|. Only full +// copies are considered successful. It returns 1 on success and 0 on failure. +static int pkcs7_bio_copy_content(BIO *src, BIO *dst) { + uint8_t buf[1024]; + int bytes_processed; + while ((bytes_processed = BIO_read(src, buf, sizeof(buf))) > 0) { + if (BIO_write(dst, buf, bytes_processed) < bytes_processed) { + return 0; + } + } + if (bytes_processed < 0) { + return 0; + } + return 1; +} + +// PKCS7_final copies the contents of |data| into |p7| before finalizing |p7|. +// The number of bytes copies +static int pkcs7_final(PKCS7 *p7, BIO *data, int flags) { + BIO *p7bio; + int ret = 0; + + OPENSSL_BEGIN_ALLOW_DEPRECATED + if ((p7bio = PKCS7_dataInit(p7, NULL)) == NULL) { + OPENSSL_END_ALLOW_DEPRECATED + OPENSSL_PUT_ERROR(PKCS7, ERR_R_PKCS7_LIB); + return 0; + } + + if (!pkcs7_bio_copy_content(data, p7bio)) { + goto err; + } + + BIO_flush(p7bio); + OPENSSL_BEGIN_ALLOW_DEPRECATED + if (!PKCS7_dataFinal(p7, p7bio)) { + OPENSSL_END_ALLOW_DEPRECATED + OPENSSL_PUT_ERROR(PKCS7, ERR_R_PKCS7_LIB); + goto err; + } + ret = 1; +err: + BIO_free_all(p7bio); + + return ret; +} + +PKCS7 *PKCS7_encrypt(STACK_OF(X509) *certs, BIO *in, const EVP_CIPHER *cipher, + int flags) { + PKCS7 *p7; + BIO *p7bio = NULL; + X509 *x509; + + if ((p7 = PKCS7_new()) == NULL) { + OPENSSL_PUT_ERROR(PKCS7, ERR_R_PKCS7_LIB); + return NULL; + } + if (!PKCS7_set_type(p7, NID_pkcs7_enveloped)) { + OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_WRONG_CONTENT_TYPE); + goto err; + } + if (!PKCS7_set_cipher(p7, cipher)) { + OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_ERROR_SETTING_CIPHER); + goto err; + } + + for (size_t i = 0; i < sk_X509_num(certs); i++) { + x509 = sk_X509_value(certs, i); + OPENSSL_BEGIN_ALLOW_DEPRECATED + if (!PKCS7_add_recipient(p7, x509)) { + OPENSSL_END_ALLOW_DEPRECATED + OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_ERROR_ADDING_RECIPIENT); + goto err; + } + } + + if (pkcs7_final(p7, in, flags)) { + return p7; + } + +err: + + BIO_free_all(p7bio); + PKCS7_free(p7); + return NULL; +} + +static int pkcs7_decrypt_rinfo(unsigned char **ek_out, PKCS7_RECIP_INFO *ri, + EVP_PKEY *pkey) { + GUARD_PTR(ri); + GUARD_PTR(ek_out); + unsigned char *ek = NULL; + int ret = 0; + + EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, /*engine*/ NULL); + if (ctx == NULL || !EVP_PKEY_decrypt_init(ctx)) { + goto err; + } + size_t len; + if (!EVP_PKEY_decrypt(ctx, NULL, &len, ri->enc_key->data, + ri->enc_key->length) || + (ek = OPENSSL_malloc(len)) == NULL) { + OPENSSL_PUT_ERROR(EVP, ERR_R_EVP_LIB); + return 0; + } + + // 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); + if (!ok) { + OPENSSL_free(ek); + ek = NULL; + } + + ret = 1; + *ek_out = ek; + +err: + EVP_PKEY_CTX_free(ctx); + return ret; +} + +static int pkcs7_cmp_ri(PKCS7_RECIP_INFO *ri, X509 *pcert) { + GUARD_PTR(ri); + GUARD_PTR(pcert); + int ret = + X509_NAME_cmp(ri->issuer_and_serial->issuer, X509_get_issuer_name(pcert)); + if (ret) { + return ret; + } + return ASN1_INTEGER_cmp(X509_get0_serialNumber(pcert), + ri->issuer_and_serial->serial); +} + +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; + ASN1_OCTET_STRING *data_body = NULL; + const EVP_CIPHER *cipher = NULL; + X509_ALGOR *enc_alg = NULL; + STACK_OF(PKCS7_RECIP_INFO) *rsk = NULL; + PKCS7_RECIP_INFO *ri = NULL; + uint8_t *cek = NULL, *dummy_key = NULL; // cek means "content encryption key" + + if (p7->d.ptr == NULL) { + OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_NO_CONTENT); + return NULL; + } + + switch (OBJ_obj2nid(p7->type)) { + case NID_pkcs7_enveloped: + rsk = p7->d.enveloped->recipientinfo; + enc_alg = p7->d.enveloped->enc_data->algorithm; + // |data_body| is NULL if the optional EncryptedContent is missing. + data_body = p7->d.enveloped->enc_data->enc_data; + cipher = EVP_get_cipherbynid(OBJ_obj2nid(enc_alg->algorithm)); + if (cipher == NULL) { + OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_UNSUPPORTED_CIPHER_TYPE); + goto err; + } + break; + default: + OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_UNSUPPORTED_CONTENT_TYPE); + 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++) { + ri = sk_PKCS7_RECIP_INFO_value(rsk, ii); + // No decryption operation here, so we can return early without divulging + // information that could be used in MMA. + if (!pkcs7_cmp_ri(ri, pcert)) { + break; + } + ri = NULL; + } + if (ri == NULL) { + OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_NO_RECIPIENT_MATCHES_CERTIFICATE); + goto err; + } + // |pkcs7_decrypt_rinfo| will only return false on critical failure, not + // on decryption failure. Decryption check happens below, after we populate + // |dummy_key| with random bytes. + if (!pkcs7_decrypt_rinfo(&cek, ri, pkey)) { + goto err; + } + } else { + // Attempt to decrypt every recipient info. Don't exit early as + // countermeasure for MMA. + for (size_t ii = 0; ii < sk_PKCS7_RECIP_INFO_num(rsk); ii++) { + ri = sk_PKCS7_RECIP_INFO_value(rsk, ii); + uint8_t *tmp_cek; + // |pkcs7_decrypt_rinfo| will only return false on critical failure, not + // on decryption failure. Check whether |tmp_cek| is present after the + // call to determine if decryption succeeded. + if (!pkcs7_decrypt_rinfo(&tmp_cek, ri, pkey)) { + goto err; + } + // Set |cek| to the first successful decryption and keep going + if (tmp_cek && !cek) { + cek = tmp_cek; + } + } + } + // Clear any decryption errors to minimize behavioral difference under MMA + ERR_clear_error(); + + EVP_CIPHER_CTX *evp_ctx = NULL; + BIO_get_cipher_ctx(cipher_bio, &evp_ctx); + if (!EVP_CipherInit_ex(evp_ctx, cipher, NULL, NULL, NULL, 0)) { + goto err; + } + uint8_t iv[EVP_MAX_IV_LENGTH]; + OPENSSL_memcpy(iv, enc_alg->parameter->value.octet_string->data, + enc_alg->parameter->value.octet_string->length); + const int expected_iv_len = EVP_CIPHER_CTX_iv_length(evp_ctx); + if (enc_alg->parameter->value.octet_string->length != expected_iv_len) { + OPENSSL_PUT_ERROR(PKCS7, ERR_R_PKCS7_LIB); + goto err; + } + 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| + int len = EVP_CIPHER_CTX_key_length(evp_ctx); + if (!len) { + goto err; + } + // Always generate random bytes for the dummy key, regardless of |cek| decrypt + dummy_key = OPENSSL_malloc(len); + RAND_bytes(dummy_key, len); + // At this point, null |cek| indicates that no content encryption key was + // successfully decrypted. We don't want to return early due to MMA. So, swap + // in the dummy key and proceed. Content decryption result will be gibberish. + if (cek == NULL) { + cek = dummy_key; + dummy_key = NULL; + } + + if (!EVP_CipherInit_ex(evp_ctx, NULL, NULL, cek, NULL, 0)) { + goto err; + } + + OPENSSL_free(cek); + OPENSSL_free(dummy_key); + out = cipher_bio; + + if (data_body->length > 0) { + data_bio = BIO_new_mem_buf(data_body->data, data_body->length); + } else { + data_bio = BIO_new(BIO_s_mem()); + if (data_bio == NULL) { + goto err; + } + BIO_set_mem_eof_return(data_bio, 0); + } + if (data_bio == NULL) { + goto err; + } + BIO_push(out, data_bio); + return out; + +err: + OPENSSL_free(cek); + OPENSSL_free(dummy_key); + BIO_free_all(out); + BIO_free_all(cipher_bio); + BIO_free_all(data_bio); + return NULL; +} + +PKCS7_RECIP_INFO *PKCS7_add_recipient(PKCS7 *p7, X509 *x509) { + PKCS7_RECIP_INFO *ri; + if ((ri = PKCS7_RECIP_INFO_new()) == NULL || + !PKCS7_RECIP_INFO_set(ri, x509) || !PKCS7_add_recipient_info(p7, ri)) { + PKCS7_RECIP_INFO_free(ri); + return NULL; + } + return ri; +} + +int PKCS7_decrypt(PKCS7 *p7, EVP_PKEY *pkey, X509 *cert, BIO *data, int flags) { + BIO *bio = NULL; + int ret = 0; + + if (p7 == NULL) { + OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_INVALID_NULL_POINTER); + goto err; + } + + 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); + goto err; + } + + if ((bio = pkcs7_data_decode(p7, pkey, cert)) == NULL || + !pkcs7_bio_copy_content(bio, data)) { + OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_DECRYPT_ERROR); + goto err; + } + + // 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); + goto err; + } + + ret = 1; + +err: + BIO_free_all(bio); + return ret; +} + diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc index a8199edc1b..a001797d29 100644 --- a/crypto/pkcs7/pkcs7_test.cc +++ b/crypto/pkcs7/pkcs7_test.cc @@ -1689,3 +1689,125 @@ TEST(PKCS7Test, DataInitFinal) { EXPECT_FALSE(bio); EXPECT_FALSE(PKCS7_dataFinal(p7.get(), bio.get())); } + +TEST(PKCS7Test, TestEnveloped) { + bssl::UniquePtr p7; + bssl::UniquePtr bio; + bssl::UniquePtr certs; + bssl::UniquePtr rsa_x509; + uint8_t buf[64], decrypted[sizeof(buf)]; + + OPENSSL_memset(buf, 'A', sizeof(buf)); + + // parse a cert for use with recipient infos + bssl::UniquePtr rsa(RSA_new()); + ASSERT_TRUE(rsa); + ASSERT_TRUE(RSA_generate_key_fips(rsa.get(), 2048, nullptr)); + bssl::UniquePtr rsa_pkey(EVP_PKEY_new()); + ASSERT_TRUE(rsa_pkey); + ASSERT_TRUE(EVP_PKEY_set1_RSA(rsa_pkey.get(), rsa.get())); + certs.reset(sk_X509_new_null()); + bio.reset(BIO_new_mem_buf(kPEMCert, strlen(kPEMCert))); + ASSERT_TRUE(bio); + certs.reset(sk_X509_new_null()); + ASSERT_TRUE(certs); + ASSERT_TRUE(PKCS7_get_PEM_certificates(certs.get(), bio.get())); + ASSERT_EQ(1U, sk_X509_num(certs.get())); + rsa_x509.reset(sk_X509_value(certs.get(), 0)); + 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))); + 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(), rsa_x509.get(), 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))); + + // 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(), /*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())); +} diff --git a/include/openssl/pkcs7.h b/include/openssl/pkcs7.h index 4c0948a7cd..dbfb438598 100644 --- a/include/openssl/pkcs7.h +++ b/include/openssl/pkcs7.h @@ -344,21 +344,46 @@ OPENSSL_EXPORT OPENSSL_DEPRECATED int PKCS7_is_detached(PKCS7 *p7); // PKCS7_dataInit creates or initializes a BIO chain for reading data from or // writing data to |p7|. If |bio| is non-null, it is added to the chain. -// Otherwise, a new BIO is allocated and returned to anchor the chain. +// Otherwise, a new BIO is allocated to anchor the chain. OPENSSL_EXPORT OPENSSL_DEPRECATED BIO *PKCS7_dataInit(PKCS7 *p7, BIO *bio); // PKCS7_dataFinal serializes data written to |bio|'s chain into |p7|. It should -// only be called on BIO chains created by |PKCS7_dataInit|. +// only be called on BIO chains created by PKCS7_dataFinal. OPENSSL_EXPORT OPENSSL_DEPRECATED int PKCS7_dataFinal(PKCS7 *p7, BIO *bio); -// PKCS7_set_digest sets |p7|'s digest to |md|. It returns 1 on success and 0 if -// |p7| is of the wrong content type. +// PKCS7_set_digest sets |p7|'s digest to |md|. It returns 1 on sucess and 0 if +// |p7| is of the wrong type. OPENSSL_EXPORT OPENSSL_DEPRECATED int PKCS7_set_digest(PKCS7 *p7, const EVP_MD *md); -// PKCS7_get_recipient_info returns a pointer to a stack containing |p7|'s -// |PKCS7_RECIP_INFO| or NULL if none are present. +// PKCS7_get_recipient_info returns a point to a stack containing |p7|'s or NULL +// if none are present. OPENSSL_EXPORT OPENSSL_DEPRECATED STACK_OF(PKCS7_RECIP_INFO) *PKCS7_get_recipient_info(PKCS7 *p7); +// BIO_get_cipher_status returns 1 if the cipher is in a healthy state or 0 +// otherwise. Unhealthy state could indicate decryption failure or other +// abnormalities. Data read from an unhealthy cipher should not be considered +// authentic. +OPENSSL_EXPORT OPENSSL_DEPRECATED int BIO_get_cipher_status(BIO *b); + +// PKCS7_add_recipient allocates a new |PCKS7_RECEPIENT_INFO|, adds |x509| to it +// and returns that |PCKS7_RECEPIENT_INFO|. +OPENSSL_EXPORT OPENSSL_DEPRECATED PKCS7_RECIP_INFO *PKCS7_add_recipient(PKCS7 *p7, X509 *x509); + +// PKCS7_encrypt encrypts the contents of |in| with |cipher| and adds |certs| as +// recipient infos and returns an encrypted |PKCS7| or NULL on failed +// encryption. |flags| is ignored. +OPENSSL_EXPORT OPENSSL_DEPRECATED PKCS7 *PKCS7_encrypt(STACK_OF(X509) *certs, BIO *in, const EVP_CIPHER *cipher, int flags); + +// PKCS7_decrypt decrypts |p7| with |pkey| and writes the plaintext to |data|. +// 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) } // extern C From bec48227c296dfed785ca494aca7992eedb133fb Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Mon, 18 Nov 2024 19:25:53 +0000 Subject: [PATCH 02/13] Remove clang-format directives and re-run formatter --- crypto/pkcs7/pkcs7.c | 39 +++++++-------------------------------- 1 file changed, 7 insertions(+), 32 deletions(-) diff --git a/crypto/pkcs7/pkcs7.c b/crypto/pkcs7/pkcs7.c index d846b06c65..9a8d992b2a 100644 --- a/crypto/pkcs7/pkcs7.c +++ b/crypto/pkcs7/pkcs7.c @@ -816,13 +816,9 @@ BIO *PKCS7_dataInit(PKCS7 *p7, BIO *bio) { goto err; } BIO_set_mem_eof_return(bio, /*eof_value*/ 0); - // clang-format off -OPENSSL_BEGIN_ALLOW_DEPRECATED - // clang-format on + OPENSSL_BEGIN_ALLOW_DEPRECATED if (!PKCS7_is_detached(p7) && content && content->length > 0) { - // clang-format off -OPENSSL_END_ALLOW_DEPRECATED - // clang-format on + OPENSSL_END_ALLOW_DEPRECATED // |bio |needs a copy of |os->data| instead of a pointer because the data // will be used after |os |has been freed if (BIO_write(bio, content->data, content->length) != content->length) { @@ -843,13 +839,9 @@ OPENSSL_END_ALLOW_DEPRECATED return NULL; } -// clang-format off OPENSSL_BEGIN_ALLOW_DEPRECATED -// clang-format on int PKCS7_is_detached(PKCS7 *p7) { - // clang-format off -OPENSSL_END_ALLOW_DEPRECATED - // clang-format on + OPENSSL_END_ALLOW_DEPRECATED GUARD_PTR(p7); if (PKCS7_type_is_signed(p7)) { return (p7->d.sign == NULL || p7->d.sign->contents->d.ptr == NULL); @@ -970,17 +962,9 @@ int PKCS7_dataFinal(PKCS7 *p7, BIO *bio) { si_sk = p7->d.sign->signer_info; // clang-format off OPENSSL_BEGIN_ALLOW_DEPRECATED - // clang-format on content = PKCS7_get_octet_string(p7->d.sign->contents); - // clang-format off -OPENSSL_END_ALLOW_DEPRECATED - // clang-format on // If detached data then the content is excluded - // clang-format off -OPENSSL_BEGIN_ALLOW_DEPRECATED - // clang-format on if (PKCS7_type_is_data(p7->d.sign->contents) && PKCS7_is_detached(p7)) { - // clang-format off OPENSSL_END_ALLOW_DEPRECATED // clang-format on ASN1_OCTET_STRING_free(content); @@ -992,13 +976,9 @@ OPENSSL_END_ALLOW_DEPRECATED case NID_pkcs7_digest: content = PKCS7_get_octet_string(p7->d.digest->contents); // If detached data, then the content is excluded - // clang-format off -OPENSSL_BEGIN_ALLOW_DEPRECATED - // clang-format on + OPENSSL_BEGIN_ALLOW_DEPRECATED if (PKCS7_type_is_data(p7->d.digest->contents) && PKCS7_is_detached(p7)) { - // clang-format off -OPENSSL_END_ALLOW_DEPRECATED - // clang-format on + OPENSSL_END_ALLOW_DEPRECATED ASN1_OCTET_STRING_free(content); content = NULL; p7->d.digest->contents->d.data = NULL; @@ -1053,13 +1033,9 @@ OPENSSL_END_ALLOW_DEPRECATED } } - // clang-format off -OPENSSL_BEGIN_ALLOW_DEPRECATED - // clang-format on + OPENSSL_BEGIN_ALLOW_DEPRECATED if (!PKCS7_is_detached(p7)) { - // clang-format off -OPENSSL_END_ALLOW_DEPRECATED - // clang-format on + OPENSSL_END_ALLOW_DEPRECATED if (content == NULL) { goto err; } @@ -1428,4 +1404,3 @@ int PKCS7_decrypt(PKCS7 *p7, EVP_PKEY *pkey, X509 *cert, BIO *data, int flags) { BIO_free_all(bio); return ret; } - From 499f1967b074089d88c3d1edeaff8a2f5b77b951 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Mon, 18 Nov 2024 20:46:21 +0000 Subject: [PATCH 03/13] Account for padding-related edge case in testing --- crypto/pkcs7/pkcs7_test.cc | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc index a001797d29..47ff60da15 100644 --- a/crypto/pkcs7/pkcs7_test.cc +++ b/crypto/pkcs7/pkcs7_test.cc @@ -1780,15 +1780,33 @@ TEST(PKCS7Test, TestEnveloped) { // 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())); + int decrypt_ok = + PKCS7_decrypt(p7.get(), rsa_pkey.get(), /*certs*/ nullptr, bio.get(), + /*flags*/ 0); + EXPECT_LE(sizeof(decrypted), BIO_pending(bio.get())); OPENSSL_cleanse(decrypted, sizeof(decrypted)); - ASSERT_EQ((int)sizeof(decrypted), - BIO_read(bio.get(), 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. + 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)) { + EXPECT_EQ(max_decrypt - 1, decrypted_len); + EXPECT_TRUE(decrypt_ok); + EXPECT_FALSE(ERR_GET_REASON(ERR_peek_error())); + } else { + EXPECT_EQ((int)sizeof(decrypted), decrypted_len); + EXPECT_FALSE(decrypt_ok); + 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_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 From 58e91347db90d8e5ce8758167ae700380baf2e42 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Mon, 18 Nov 2024 22:23:37 +0000 Subject: [PATCH 04/13] Fix occasional buffer overflow in test --- crypto/pkcs7/pkcs7_test.cc | 43 +++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 19 deletions(-) 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())); } From 704e9fe9cc7ca595831f62c96a1ecd08071fbbcf Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Tue, 19 Nov 2024 14:36:50 +0000 Subject: [PATCH 05/13] Increase coverage, fix doc comments --- crypto/pkcs7/pkcs7.c | 13 +++++++------ crypto/pkcs7/pkcs7_test.cc | 19 +++++++++++++++++++ include/openssl/pkcs7.h | 12 ++++++------ 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/crypto/pkcs7/pkcs7.c b/crypto/pkcs7/pkcs7.c index 9a8d992b2a..1cda923ff9 100644 --- a/crypto/pkcs7/pkcs7.c +++ b/crypto/pkcs7/pkcs7.c @@ -1111,6 +1111,9 @@ static int pkcs7_final(PKCS7 *p7, BIO *data, int flags) { PKCS7 *PKCS7_encrypt(STACK_OF(X509) *certs, BIO *in, const EVP_CIPHER *cipher, int flags) { + GUARD_PTR(certs); + GUARD_PTR(in); + GUARD_PTR(cipher); PKCS7 *p7; BIO *p7bio = NULL; X509 *x509; @@ -1327,7 +1330,7 @@ static BIO *pkcs7_data_decode(PKCS7 *p7, EVP_PKEY *pkey, X509 *pcert) { OPENSSL_free(dummy_key); out = cipher_bio; - if (data_body->length > 0) { + if (data_body && data_body->length > 0) { data_bio = BIO_new_mem_buf(data_body->data, data_body->length); } else { data_bio = BIO_new(BIO_s_mem()); @@ -1362,14 +1365,12 @@ PKCS7_RECIP_INFO *PKCS7_add_recipient(PKCS7 *p7, X509 *x509) { } int PKCS7_decrypt(PKCS7 *p7, EVP_PKEY *pkey, X509 *cert, BIO *data, int flags) { + GUARD_PTR(p7); + GUARD_PTR(pkey); + GUARD_PTR(data); BIO *bio = NULL; int ret = 0; - if (p7 == NULL) { - OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_INVALID_NULL_POINTER); - goto err; - } - switch (OBJ_obj2nid(p7->type)) { case NID_pkcs7_enveloped: case NID_pkcs7_signedAndEnveloped: diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc index 5ed3d5906c..5bc1038a44 100644 --- a/crypto/pkcs7/pkcs7_test.cc +++ b/crypto/pkcs7/pkcs7_test.cc @@ -1767,6 +1767,14 @@ TEST(PKCS7Test, TestEnveloped) { EXPECT_EQ(0, BIO_read(bio.get(), decrypted, sizeof(decrypted))); EXPECT_FALSE(BIO_should_retry(bio.get())); + // unsupported content type, with and without content + p7.reset(PKCS7_new()); + ASSERT_TRUE(p7); + ASSERT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_signed)); + EXPECT_FALSE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), nullptr, bio.get(), 0)); + ASSERT_TRUE(PKCS7_content_new(p7.get(), NID_pkcs7_data)); + EXPECT_FALSE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), nullptr, bio.get(), 0)); + // 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, pt_len)); @@ -1833,4 +1841,15 @@ TEST(PKCS7Test, TestEnveloped) { // ...but it produces pseudo-random nonsense EXPECT_NE(Bytes(buf, pt_len), Bytes(decrypted, sizeof(decrypted))); EXPECT_FALSE(ERR_GET_REASON(ERR_peek_error())); + + // mismatched cert + pkey on decrypt + bio.reset(BIO_new_mem_buf(buf, pt_len)); + p7.reset( + PKCS7_encrypt(certs.get(), bio.get(), EVP_aes_128_cbc(), /*flags*/ 0)); + bio.reset(BIO_new(BIO_s_mem())); + 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())); + EXPECT_FALSE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), rsa_x509.get(), bio.get(), + /*flags*/ 0)); } diff --git a/include/openssl/pkcs7.h b/include/openssl/pkcs7.h index dbfb438598..aa95fc3dff 100644 --- a/include/openssl/pkcs7.h +++ b/include/openssl/pkcs7.h @@ -344,19 +344,19 @@ OPENSSL_EXPORT OPENSSL_DEPRECATED int PKCS7_is_detached(PKCS7 *p7); // PKCS7_dataInit creates or initializes a BIO chain for reading data from or // writing data to |p7|. If |bio| is non-null, it is added to the chain. -// Otherwise, a new BIO is allocated to anchor the chain. +// Otherwise, a new BIO is allocated and returned to anchor the chain. OPENSSL_EXPORT OPENSSL_DEPRECATED BIO *PKCS7_dataInit(PKCS7 *p7, BIO *bio); // PKCS7_dataFinal serializes data written to |bio|'s chain into |p7|. It should -// only be called on BIO chains created by PKCS7_dataFinal. +// only be called on BIO chains created by |PKCS7_dataInit|. OPENSSL_EXPORT OPENSSL_DEPRECATED int PKCS7_dataFinal(PKCS7 *p7, BIO *bio); -// PKCS7_set_digest sets |p7|'s digest to |md|. It returns 1 on sucess and 0 if -// |p7| is of the wrong type. +// PKCS7_set_digest sets |p7|'s digest to |md|. It returns 1 on success and 0 if +// |p7| is of the wrong content type. OPENSSL_EXPORT OPENSSL_DEPRECATED int PKCS7_set_digest(PKCS7 *p7, const EVP_MD *md); -// PKCS7_get_recipient_info returns a point to a stack containing |p7|'s or NULL -// if none are present. +// PKCS7_get_recipient_info returns a pointer to a stack containing |p7|'s +// |PKCS7_RECIP_INFO| or NULL if none are present. OPENSSL_EXPORT OPENSSL_DEPRECATED STACK_OF(PKCS7_RECIP_INFO) *PKCS7_get_recipient_info(PKCS7 *p7); // BIO_get_cipher_status returns 1 if the cipher is in a healthy state or 0 From a0abcdf9917496231df50cf21187a66585a1b96b Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Wed, 20 Nov 2024 16:59:50 +0000 Subject: [PATCH 06/13] Address PR feedback --- crypto/pkcs7/internal.h | 6 ++++++ crypto/pkcs7/pkcs7.c | 35 +++++++++++++++++------------------ crypto/pkcs7/pkcs7_test.cc | 6 ++++-- include/openssl/pkcs7.h | 12 ++++-------- 4 files changed, 31 insertions(+), 28 deletions(-) diff --git a/crypto/pkcs7/internal.h b/crypto/pkcs7/internal.h index a59f01f3c2..a3b5bc433e 100644 --- a/crypto/pkcs7/internal.h +++ b/crypto/pkcs7/internal.h @@ -223,6 +223,12 @@ OPENSSL_EXPORT int BIO_set_cipher(BIO *b, const EVP_CIPHER *cipher, const unsigned char *key, const unsigned char *iv, int enc); +// BIO_get_cipher_status returns 1 if the cipher is in a healthy state or 0 +// otherwise. Unhealthy state could indicate decryption failure or other +// abnormalities. Data read from an unhealthy cipher should not be considered +// authentic. +OPENSSL_EXPORT int BIO_get_cipher_status(BIO *b); + #if defined(__cplusplus) } // extern C #endif diff --git a/crypto/pkcs7/pkcs7.c b/crypto/pkcs7/pkcs7.c index 1cda923ff9..dd2b2d391d 100644 --- a/crypto/pkcs7/pkcs7.c +++ b/crypto/pkcs7/pkcs7.c @@ -662,8 +662,7 @@ static int pkcs7_encode_rinfo(PKCS7_RECIP_INFO *ri, unsigned char *key, int ret = 0; size_t eklen; - pkey = X509_get0_pubkey(ri->cert); - if (pkey == NULL) { + if ((pkey = X509_get0_pubkey(ri->cert)) == NULL) { goto err; } @@ -1068,7 +1067,7 @@ static int pkcs7_bio_copy_content(BIO *src, BIO *dst) { uint8_t buf[1024]; int bytes_processed; while ((bytes_processed = BIO_read(src, buf, sizeof(buf))) > 0) { - if (BIO_write(dst, buf, bytes_processed) < bytes_processed) { + if (!BIO_write_all(dst, buf, bytes_processed)) { return 0; } } @@ -1079,8 +1078,7 @@ static int pkcs7_bio_copy_content(BIO *src, BIO *dst) { } // PKCS7_final copies the contents of |data| into |p7| before finalizing |p7|. -// The number of bytes copies -static int pkcs7_final(PKCS7 *p7, BIO *data, int flags) { +static int pkcs7_final(PKCS7 *p7, BIO *data) { BIO *p7bio; int ret = 0; @@ -1115,7 +1113,6 @@ PKCS7 *PKCS7_encrypt(STACK_OF(X509) *certs, BIO *in, const EVP_CIPHER *cipher, GUARD_PTR(in); GUARD_PTR(cipher); PKCS7 *p7; - BIO *p7bio = NULL; X509 *x509; if ((p7 = PKCS7_new()) == NULL) { @@ -1141,13 +1138,11 @@ PKCS7 *PKCS7_encrypt(STACK_OF(X509) *certs, BIO *in, const EVP_CIPHER *cipher, } } - if (pkcs7_final(p7, in, flags)) { + if (pkcs7_final(p7, in)) { return p7; } err: - - BIO_free_all(p7bio); PKCS7_free(p7); return NULL; } @@ -1171,9 +1166,11 @@ static int pkcs7_decrypt_rinfo(unsigned char **ek_out, PKCS7_RECIP_INFO *ri, return 0; } - // 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); + // We only return 0 on critical failure, not decryption failure. However, we + // still need to set |ek| to NULL to signal decryption failure to callers so + // they can use random bytes as content encryption key for MMA defense. if (!ok) { OPENSSL_free(ek); ek = NULL; @@ -1187,9 +1184,12 @@ static int pkcs7_decrypt_rinfo(unsigned char **ek_out, PKCS7_RECIP_INFO *ri, return ret; } +// pkcs7_cmp_ri is a comparison function, so it returns 0 if |ri| and |pcert| +// match and 1 if they do not. static int pkcs7_cmp_ri(PKCS7_RECIP_INFO *ri, X509 *pcert) { - GUARD_PTR(ri); - GUARD_PTR(pcert); + if (ri == NULL || ri->issuer_and_serial == NULL || pcert == NULL) { + return 1; + } int ret = X509_NAME_cmp(ri->issuer_and_serial->issuer, X509_get_issuer_name(pcert)); if (ret) { @@ -1291,8 +1291,8 @@ static BIO *pkcs7_data_decode(PKCS7 *p7, EVP_PKEY *pkey, X509 *pcert) { ERR_clear_error(); EVP_CIPHER_CTX *evp_ctx = NULL; - BIO_get_cipher_ctx(cipher_bio, &evp_ctx); - if (!EVP_CipherInit_ex(evp_ctx, cipher, NULL, NULL, NULL, 0)) { + if (!BIO_get_cipher_ctx(cipher_bio, &evp_ctx) || + !EVP_CipherInit_ex(evp_ctx, cipher, NULL, NULL, NULL, 0)) { goto err; } uint8_t iv[EVP_MAX_IV_LENGTH]; @@ -1334,10 +1334,9 @@ static BIO *pkcs7_data_decode(PKCS7 *p7, EVP_PKEY *pkey, X509 *pcert) { data_bio = BIO_new_mem_buf(data_body->data, data_body->length); } else { data_bio = BIO_new(BIO_s_mem()); - if (data_bio == NULL) { + if (data_bio == NULL || !BIO_set_mem_eof_return(data_bio, 0)) { goto err; } - BIO_set_mem_eof_return(data_bio, 0); } if (data_bio == NULL) { goto err; @@ -1355,6 +1354,8 @@ static BIO *pkcs7_data_decode(PKCS7 *p7, EVP_PKEY *pkey, X509 *pcert) { } PKCS7_RECIP_INFO *PKCS7_add_recipient(PKCS7 *p7, X509 *x509) { + GUARD_PTR(p7); + GUARD_PTR(x509); PKCS7_RECIP_INFO *ri; if ((ri = PKCS7_RECIP_INFO_new()) == NULL || !PKCS7_RECIP_INFO_set(ri, x509) || !PKCS7_add_recipient_info(p7, ri)) { @@ -1392,9 +1393,7 @@ int PKCS7_decrypt(PKCS7 *p7, EVP_PKEY *pkey, X509 *cert, BIO *data, int flags) { } // 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); goto err; } diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc index 5bc1038a44..92917f942d 100644 --- a/crypto/pkcs7/pkcs7_test.cc +++ b/crypto/pkcs7/pkcs7_test.cc @@ -1810,6 +1810,7 @@ TEST(PKCS7Test, TestEnveloped) { sizeof(decrypted) + EVP_CIPHER_block_size(EVP_aes_128_cbc()); int decrypted_len = BIO_read(bio.get(), decrypted, max_decrypt); if (decrypted_len > (int)pt_len) { + // TODO [childw] how to get padded plaintext from BIO? need to walk pad... EXPECT_EQ(max_decrypt - 1, decrypted_len); EXPECT_TRUE(decrypt_ok); EXPECT_FALSE(ERR_GET_REASON(ERR_peek_error())); @@ -1850,6 +1851,7 @@ TEST(PKCS7Test, TestEnveloped) { 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())); - EXPECT_FALSE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), rsa_x509.get(), bio.get(), - /*flags*/ 0)); + EXPECT_FALSE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), rsa_x509.get(), + bio.get(), + /*flags*/ 0)); } diff --git a/include/openssl/pkcs7.h b/include/openssl/pkcs7.h index aa95fc3dff..3562c22820 100644 --- a/include/openssl/pkcs7.h +++ b/include/openssl/pkcs7.h @@ -359,25 +359,20 @@ OPENSSL_EXPORT OPENSSL_DEPRECATED int PKCS7_set_digest(PKCS7 *p7, const EVP_MD * // |PKCS7_RECIP_INFO| or NULL if none are present. OPENSSL_EXPORT OPENSSL_DEPRECATED STACK_OF(PKCS7_RECIP_INFO) *PKCS7_get_recipient_info(PKCS7 *p7); -// BIO_get_cipher_status returns 1 if the cipher is in a healthy state or 0 -// otherwise. Unhealthy state could indicate decryption failure or other -// abnormalities. Data read from an unhealthy cipher should not be considered -// authentic. -OPENSSL_EXPORT OPENSSL_DEPRECATED int BIO_get_cipher_status(BIO *b); - // PKCS7_add_recipient allocates a new |PCKS7_RECEPIENT_INFO|, adds |x509| to it // and returns that |PCKS7_RECEPIENT_INFO|. OPENSSL_EXPORT OPENSSL_DEPRECATED PKCS7_RECIP_INFO *PKCS7_add_recipient(PKCS7 *p7, X509 *x509); // PKCS7_encrypt encrypts the contents of |in| with |cipher| and adds |certs| as // recipient infos and returns an encrypted |PKCS7| or NULL on failed -// encryption. |flags| is ignored. +// encryption. |flags| is ignored. We only perform key encryption using RSA, so +// |certs| must use RSA public keys. OPENSSL_EXPORT OPENSSL_DEPRECATED PKCS7 *PKCS7_encrypt(STACK_OF(X509) *certs, BIO *in, const EVP_CIPHER *cipher, int flags); // PKCS7_decrypt decrypts |p7| with |pkey| and writes the plaintext to |data|. // 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. +// ignored. |pkey| must be an |EVP_PKEY_RSA|. // // 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 @@ -430,5 +425,6 @@ BSSL_NAMESPACE_END #define PKCS7_R_PKCS7_ADD_SIGNER_ERROR 131 #define PKCS7_R_PKCS7_ADD_SIGNATURE_ERROR 132 #define PKCS7_R_NO_DEFAULT_DIGEST 133 +#define PKCS7_R_CERT_MUST_BE_RSA 134 #endif // OPENSSL_HEADER_PKCS7_H From 6fd4a0607b3a6ca154578506164203980d692dbe Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Wed, 20 Nov 2024 17:09:35 +0000 Subject: [PATCH 07/13] Additional randomized test edge case mitigation (bleh) --- crypto/pkcs7/pkcs7_test.cc | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc index 92917f942d..dbf2a8b845 100644 --- a/crypto/pkcs7/pkcs7_test.cc +++ b/crypto/pkcs7/pkcs7_test.cc @@ -1805,13 +1805,18 @@ TEST(PKCS7Test, TestEnveloped) { // 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. + // "decrypted plaintext" than anticipated. However, this doesn't only apply to + // one byte of padding. With probability 16^-2, it applies to pad 0x02 0x02 + // and so on with increasingly small probabilities. So, we give slack up to + // 16^-4 which means this test will erroneously fail 0.001526% of the time in + // expectation. Ideally we'd find a way to access the padded plaintext and + // account for this deterministically by checking the random "padding" and + // adusting accordingly. 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)pt_len) { - // TODO [childw] how to get padded plaintext from BIO? need to walk pad... - EXPECT_EQ(max_decrypt - 1, decrypted_len); + EXPECT_GT(max_decrypt - 4, decrypted_len); EXPECT_TRUE(decrypt_ok); EXPECT_FALSE(ERR_GET_REASON(ERR_peek_error())); } else { From 5d8bd7caed7567a75c1681b79bf77f23ced53080 Mon Sep 17 00:00:00 2001 From: Will Childs-Klein Date: Thu, 21 Nov 2024 18:17:15 -0500 Subject: [PATCH 08/13] PR feedback --- crypto/pkcs7/pkcs7.c | 14 ++++++++------ crypto/pkcs7/pkcs7_test.cc | 1 + 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/crypto/pkcs7/pkcs7.c b/crypto/pkcs7/pkcs7.c index dd2b2d391d..ffb7b9308c 100644 --- a/crypto/pkcs7/pkcs7.c +++ b/crypto/pkcs7/pkcs7.c @@ -1163,14 +1163,14 @@ static int pkcs7_decrypt_rinfo(unsigned char **ek_out, PKCS7_RECIP_INFO *ri, ri->enc_key->length) || (ek = OPENSSL_malloc(len)) == NULL) { OPENSSL_PUT_ERROR(EVP, ERR_R_EVP_LIB); - return 0; + goto err; } int ok = EVP_PKEY_decrypt(ctx, ek, &len, ri->enc_key->data, ri->enc_key->length); - // We only return 0 on critical failure, not decryption failure. However, we - // still need to set |ek| to NULL to signal decryption failure to callers so - // they can use random bytes as content encryption key for MMA defense. + // We return 0 any failure except for decryption failure. On decrypt failure, + // we still need to set |ek| to NULL to signal decryption failure to callers + // so they can use random bytes as content encryption key for MMA defense. if (!ok) { OPENSSL_free(ek); ek = NULL; @@ -1281,8 +1281,10 @@ static BIO *pkcs7_data_decode(PKCS7 *p7, EVP_PKEY *pkey, X509 *pcert) { if (!pkcs7_decrypt_rinfo(&tmp_cek, ri, pkey)) { goto err; } - // Set |cek| to the first successful decryption and keep going - if (tmp_cek && !cek) { + // OpenSSL sets encryption key to last successfully decrypted key. Copy + // that behavior, but free previously allocated key memory. + if (tmp_cek) { + OPENSSL_free(cek); cek = tmp_cek; } } diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc index dbf2a8b845..090f28a75e 100644 --- a/crypto/pkcs7/pkcs7_test.cc +++ b/crypto/pkcs7/pkcs7_test.cc @@ -1722,6 +1722,7 @@ TEST(PKCS7Test, TestEnveloped) { ASSERT_TRUE(X509_set_pubkey(rsa_x509.get(), rsa_pkey.get())); X509_up_ref(rsa_x509.get()); + // standard success case bio.reset(BIO_new_mem_buf(buf, pt_len)); p7.reset( PKCS7_encrypt(certs.get(), bio.get(), EVP_aes_128_cbc(), /*flags*/ 0)); From f5d92c91ecb579032da6b02fb5dd494f2b3e9869 Mon Sep 17 00:00:00 2001 From: Will Childs-Klein Date: Thu, 21 Nov 2024 18:17:15 -0500 Subject: [PATCH 09/13] PR feedback --- crypto/pkcs7/pkcs7_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc index 090f28a75e..d6f81bd6de 100644 --- a/crypto/pkcs7/pkcs7_test.cc +++ b/crypto/pkcs7/pkcs7_test.cc @@ -1817,7 +1817,7 @@ TEST(PKCS7Test, TestEnveloped) { sizeof(decrypted) + EVP_CIPHER_block_size(EVP_aes_128_cbc()); int decrypted_len = BIO_read(bio.get(), decrypted, max_decrypt); if (decrypted_len > (int)pt_len) { - EXPECT_GT(max_decrypt - 4, decrypted_len); + EXPECT_LT(max_decrypt - 4, decrypted_len); EXPECT_TRUE(decrypt_ok); EXPECT_FALSE(ERR_GET_REASON(ERR_peek_error())); } else { From ed61efd07de067073e45144f2868915a4a7ceaf2 Mon Sep 17 00:00:00 2001 From: Will Childs-Klein Date: Fri, 22 Nov 2024 11:11:07 -0500 Subject: [PATCH 10/13] Add multi-recipient test case --- crypto/pkcs7/pkcs7_test.cc | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc index d6f81bd6de..ead46ff3d9 100644 --- a/crypto/pkcs7/pkcs7_test.cc +++ b/crypto/pkcs7/pkcs7_test.cc @@ -1776,6 +1776,24 @@ TEST(PKCS7Test, TestEnveloped) { ASSERT_TRUE(PKCS7_content_new(p7.get(), NID_pkcs7_data)); EXPECT_FALSE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), nullptr, bio.get(), 0)); + // test multiple recipients using the same recipient twice. elide |cert| to + // exercise iterative decryption attempt behavior with multiple (2) successful + // decryptions. + X509_up_ref(rsa_x509.get()); + sk_X509_push(certs.get(), rsa_x509.get()); + bio.reset(BIO_new_mem_buf(buf, pt_len)); + p7.reset( + PKCS7_encrypt(certs.get(), bio.get(), EVP_aes_128_cbc(), /*flags*/ 0)); + ASSERT_TRUE(p7); + bio.reset(BIO_new(BIO_s_mem())); + // set |rsa_pkey| back to original RSA key + ASSERT_TRUE(EVP_PKEY_set1_RSA(rsa_pkey.get(), rsa.get())); + EXPECT_TRUE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), /*cert*/nullptr, + bio.get(), + /*flags*/ 0)); + ASSERT_TRUE(sk_X509_pop(certs.get())); + ASSERT_EQ(1LU, sk_X509_num(certs.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, pt_len)); @@ -1784,7 +1802,7 @@ TEST(PKCS7Test, TestEnveloped) { 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 + // set new 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())); @@ -1854,10 +1872,11 @@ TEST(PKCS7Test, TestEnveloped) { p7.reset( PKCS7_encrypt(certs.get(), bio.get(), EVP_aes_128_cbc(), /*flags*/ 0)); bio.reset(BIO_new(BIO_s_mem())); - 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())); + bssl::UniquePtr rsa2(RSA_new()); + ASSERT_TRUE(RSA_generate_key_fips(rsa2.get(), 2048, nullptr)); + ASSERT_TRUE(EVP_PKEY_set1_RSA(rsa_pkey.get(), rsa2.get())); EXPECT_FALSE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), rsa_x509.get(), bio.get(), /*flags*/ 0)); + EXPECT_EQ(X509_R_KEY_VALUES_MISMATCH, ERR_GET_REASON(ERR_peek_error())); } From cae369843a11198064bffae4825a109c20d7422e Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Fri, 22 Nov 2024 16:20:27 +0000 Subject: [PATCH 11/13] Remove unnecessary upref in test --- crypto/pkcs7/pkcs7_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc index ead46ff3d9..d5eb4747dd 100644 --- a/crypto/pkcs7/pkcs7_test.cc +++ b/crypto/pkcs7/pkcs7_test.cc @@ -1779,7 +1779,6 @@ TEST(PKCS7Test, TestEnveloped) { // test multiple recipients using the same recipient twice. elide |cert| to // exercise iterative decryption attempt behavior with multiple (2) successful // decryptions. - X509_up_ref(rsa_x509.get()); sk_X509_push(certs.get(), rsa_x509.get()); bio.reset(BIO_new_mem_buf(buf, pt_len)); p7.reset( From 1c3d6ad3a31d17aa4b8433d37724c0bb3cf1334d Mon Sep 17 00:00:00 2001 From: Will Childs-Klein Date: Fri, 22 Nov 2024 17:56:54 -0500 Subject: [PATCH 12/13] PR feedback --- crypto/pkcs7/pkcs7.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crypto/pkcs7/pkcs7.c b/crypto/pkcs7/pkcs7.c index ffb7b9308c..2da1992abe 100644 --- a/crypto/pkcs7/pkcs7.c +++ b/crypto/pkcs7/pkcs7.c @@ -1395,7 +1395,7 @@ int PKCS7_decrypt(PKCS7 *p7, EVP_PKEY *pkey, X509 *cert, BIO *data, int flags) { } // Check whether content decryption was successful - if (!BIO_get_cipher_status(bio)) { + if (1 != BIO_get_cipher_status(bio)) { OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_DECRYPT_ERROR); goto err; } From 8b49c3f9dd477736918f9a3e2d16ce53c9ce06aa Mon Sep 17 00:00:00 2001 From: Will Childs-Klein Date: Fri, 22 Nov 2024 22:04:35 -0500 Subject: [PATCH 13/13] Cleanse stack copy buffer in pkcs7_bio_copy_content --- crypto/pkcs7/pkcs7.c | 11 +++++++---- crypto/pkcs7/pkcs7_test.cc | 6 +++--- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/crypto/pkcs7/pkcs7.c b/crypto/pkcs7/pkcs7.c index 2da1992abe..4ff90e8280 100644 --- a/crypto/pkcs7/pkcs7.c +++ b/crypto/pkcs7/pkcs7.c @@ -1065,16 +1065,19 @@ OPENSSL_END_ALLOW_DEPRECATED // copies are considered successful. It returns 1 on success and 0 on failure. static int pkcs7_bio_copy_content(BIO *src, BIO *dst) { uint8_t buf[1024]; - int bytes_processed; + int bytes_processed, ret = 0; while ((bytes_processed = BIO_read(src, buf, sizeof(buf))) > 0) { if (!BIO_write_all(dst, buf, bytes_processed)) { - return 0; + goto err; } } if (bytes_processed < 0) { - return 0; + goto err; } - return 1; + ret = 1; +err: + OPENSSL_cleanse(buf, sizeof(buf)); + return ret; } // PKCS7_final copies the contents of |data| into |p7| before finalizing |p7|. diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc index d5eb4747dd..b75408a1a3 100644 --- a/crypto/pkcs7/pkcs7_test.cc +++ b/crypto/pkcs7/pkcs7_test.cc @@ -1787,9 +1787,9 @@ TEST(PKCS7Test, TestEnveloped) { bio.reset(BIO_new(BIO_s_mem())); // set |rsa_pkey| back to original RSA key ASSERT_TRUE(EVP_PKEY_set1_RSA(rsa_pkey.get(), rsa.get())); - EXPECT_TRUE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), /*cert*/nullptr, - bio.get(), - /*flags*/ 0)); + EXPECT_TRUE(PKCS7_decrypt(p7.get(), rsa_pkey.get(), /*cert*/ nullptr, + bio.get(), + /*flags*/ 0)); ASSERT_TRUE(sk_X509_pop(certs.get())); ASSERT_EQ(1LU, sk_X509_num(certs.get()));