From 6d2a01cdfb56fdb8ea5d5dd417724e6906c8b8e2 Mon Sep 17 00:00:00 2001 From: Bernd Edlinger Date: Thu, 7 Sep 2023 18:05:44 +0200 Subject: [PATCH] Fix error handling in CMS_EncryptedData_encrypt That caused several memory leaks in case of error. Also when the CMS object that is created by CMS_EncryptedData_encrypt is not used in the normal way, but instead just deleted by CMS_ContentInfo_free some memory was lost. Fixes #21985 Reviewed-by: Todd Short Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/22031) --- crypto/cms/cms_asn1.c | 19 +++++++++++++++++-- crypto/cms/cms_env.c | 13 +++---------- crypto/cms/cms_lib.c | 15 +-------------- crypto/cms/cms_local.h | 2 +- crypto/cms/cms_sd.c | 20 ++++++++++++++++++-- crypto/cms/cms_smime.c | 3 ++- test/recipes/80-test_cms.t | 7 +++++++ 7 files changed, 49 insertions(+), 30 deletions(-) diff --git a/crypto/cms/cms_asn1.c b/crypto/cms/cms_asn1.c index bc6b2769f9..9ac848e448 100644 --- a/crypto/cms/cms_asn1.c +++ b/crypto/cms/cms_asn1.c @@ -51,6 +51,7 @@ static int cms_si_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, EVP_PKEY_free(si->pkey); X509_free(si->signer); EVP_MD_CTX_free(si->mctx); + EVP_PKEY_CTX_free(si->pctx); } return 1; } @@ -90,11 +91,21 @@ ASN1_SEQUENCE(CMS_OriginatorInfo) = { ASN1_IMP_SET_OF_OPT(CMS_OriginatorInfo, crls, CMS_RevocationInfoChoice, 1) } static_ASN1_SEQUENCE_END(CMS_OriginatorInfo) -ASN1_NDEF_SEQUENCE(CMS_EncryptedContentInfo) = { +static int cms_ec_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, + void *exarg) +{ + CMS_EncryptedContentInfo *ec = (CMS_EncryptedContentInfo *)*pval; + + if (operation == ASN1_OP_FREE_POST) + OPENSSL_clear_free(ec->key, ec->keylen); + return 1; +} + +ASN1_NDEF_SEQUENCE_cb(CMS_EncryptedContentInfo, cms_ec_cb) = { ASN1_SIMPLE(CMS_EncryptedContentInfo, contentType, ASN1_OBJECT), ASN1_SIMPLE(CMS_EncryptedContentInfo, contentEncryptionAlgorithm, X509_ALGOR), ASN1_IMP_OPT(CMS_EncryptedContentInfo, encryptedContent, ASN1_OCTET_STRING_NDEF, 0) -} static_ASN1_NDEF_SEQUENCE_END(CMS_EncryptedContentInfo) +} ASN1_NDEF_SEQUENCE_END_cb(CMS_EncryptedContentInfo, CMS_EncryptedContentInfo) ASN1_SEQUENCE(CMS_KeyTransRecipientInfo) = { ASN1_EMBED(CMS_KeyTransRecipientInfo, version, INT32), @@ -318,6 +329,10 @@ static int cms_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, return 0; break; + case ASN1_OP_FREE_POST: + OPENSSL_free(cms->ctx.propq); + break; + } return 1; } diff --git a/crypto/cms/cms_env.c b/crypto/cms/cms_env.c index b877e10619..37dfbe5389 100644 --- a/crypto/cms/cms_env.c +++ b/crypto/cms/cms_env.c @@ -51,15 +51,6 @@ static int cms_get_enveloped_type(const CMS_ContentInfo *cms) return ret; } -void ossl_cms_env_enc_content_free(const CMS_ContentInfo *cinf) -{ - if (cms_get_enveloped_type_simple(cinf) != 0) { - CMS_EncryptedContentInfo *ec = ossl_cms_get0_env_enc_content(cinf); - if (ec != NULL) - OPENSSL_clear_free(ec->key, ec->keylen); - } -} - CMS_EnvelopedData *ossl_cms_get0_enveloped(CMS_ContentInfo *cms) { if (OBJ_obj2nid(cms->contentType) != NID_pkcs7_enveloped) { @@ -289,8 +280,10 @@ BIO *CMS_EnvelopedData_decrypt(CMS_EnvelopedData *env, BIO *detached_data, secret == NULL ? cert : NULL, detached_data, bio, flags); end: - if (ci != NULL) + if (ci != NULL) { ci->d.envelopedData = NULL; /* do not indirectly free |env| */ + ci->contentType = NULL; + } CMS_ContentInfo_free(ci); if (!res) { BIO_free(bio); diff --git a/crypto/cms/cms_lib.c b/crypto/cms/cms_lib.c index afc210c9d0..759e1d46af 100644 --- a/crypto/cms/cms_lib.c +++ b/crypto/cms/cms_lib.c @@ -21,6 +21,7 @@ static STACK_OF(CMS_CertificateChoices) **cms_get0_certificate_choices(CMS_ContentInfo *cms); +IMPLEMENT_ASN1_ALLOC_FUNCTIONS(CMS_ContentInfo) IMPLEMENT_ASN1_PRINT_FUNCTION(CMS_ContentInfo) CMS_ContentInfo *d2i_CMS_ContentInfo(CMS_ContentInfo **a, @@ -66,20 +67,6 @@ CMS_ContentInfo *CMS_ContentInfo_new_ex(OSSL_LIB_CTX *libctx, const char *propq) return ci; } -CMS_ContentInfo *CMS_ContentInfo_new(void) -{ - return CMS_ContentInfo_new_ex(NULL, NULL); -} - -void CMS_ContentInfo_free(CMS_ContentInfo *cms) -{ - if (cms != NULL) { - ossl_cms_env_enc_content_free(cms); - OPENSSL_free(cms->ctx.propq); - ASN1_item_free((ASN1_VALUE *)cms, ASN1_ITEM_rptr(CMS_ContentInfo)); - } -} - const CMS_CTX *ossl_cms_get0_cmsctx(const CMS_ContentInfo *cms) { return cms != NULL ? &cms->ctx : NULL; diff --git a/crypto/cms/cms_local.h b/crypto/cms/cms_local.h index 7069021267..a2b72484c6 100644 --- a/crypto/cms/cms_local.h +++ b/crypto/cms/cms_local.h @@ -366,6 +366,7 @@ struct CMS_Receipt_st { DECLARE_ASN1_FUNCTIONS(CMS_ContentInfo) DECLARE_ASN1_ITEM(CMS_SignerInfo) +DECLARE_ASN1_ITEM(CMS_EncryptedContentInfo) DECLARE_ASN1_ITEM(CMS_IssuerAndSerialNumber) DECLARE_ASN1_ITEM(CMS_Attributes_Sign) DECLARE_ASN1_ITEM(CMS_Attributes_Verify) @@ -447,7 +448,6 @@ BIO *ossl_cms_EnvelopedData_init_bio(CMS_ContentInfo *cms); int ossl_cms_EnvelopedData_final(CMS_ContentInfo *cms, BIO *chain); BIO *ossl_cms_AuthEnvelopedData_init_bio(CMS_ContentInfo *cms); int ossl_cms_AuthEnvelopedData_final(CMS_ContentInfo *cms, BIO *cmsbio); -void ossl_cms_env_enc_content_free(const CMS_ContentInfo *cinf); CMS_EnvelopedData *ossl_cms_get0_enveloped(CMS_ContentInfo *cms); CMS_AuthEnvelopedData *ossl_cms_get0_auth_enveloped(CMS_ContentInfo *cms); CMS_EncryptedContentInfo *ossl_cms_get0_env_enc_content(const CMS_ContentInfo *cms); diff --git a/crypto/cms/cms_sd.c b/crypto/cms/cms_sd.c index b41e3571b2..a76e795df5 100644 --- a/crypto/cms/cms_sd.c +++ b/crypto/cms/cms_sd.c @@ -512,8 +512,12 @@ CMS_SignerInfo *CMS_add1_signer(CMS_ContentInfo *cms, ossl_cms_ctx_get0_libctx(ctx), ossl_cms_ctx_get0_propq(ctx), pk, NULL) <= 0) { + si->pctx = NULL; goto err; } + else { + EVP_MD_CTX_set_flags(si->mctx, EVP_MD_CTX_FLAG_KEEP_PKEY_CTX); + } } if (sd->signerInfos == NULL) @@ -758,6 +762,7 @@ static int cms_SignerInfo_content_sign(CMS_ContentInfo *cms, unsigned char computed_md[EVP_MAX_MD_SIZE]; pctx = si->pctx; + si->pctx = NULL; if (md == NULL) { if (!EVP_DigestFinal_ex(mctx, computed_md, &mdlen)) goto err; @@ -851,6 +856,7 @@ int CMS_SignerInfo_sign(CMS_SignerInfo *si) ossl_cms_ctx_get0_propq(ctx), si->pkey, NULL) <= 0) goto err; + EVP_MD_CTX_set_flags(mctx, EVP_MD_CTX_FLAG_KEEP_PKEY_CTX); si->pctx = pctx; } @@ -922,9 +928,16 @@ int CMS_SignerInfo_verify(CMS_SignerInfo *si) goto err; } mctx = si->mctx; + if (si->pctx != NULL) { + EVP_PKEY_CTX_free(si->pctx); + si->pctx = NULL; + } if (EVP_DigestVerifyInit_ex(mctx, &si->pctx, EVP_MD_get0_name(md), libctx, - propq, si->pkey, NULL) <= 0) + propq, si->pkey, NULL) <= 0) { + si->pctx = NULL; goto err; + } + EVP_MD_CTX_set_flags(mctx, EVP_MD_CTX_FLAG_KEEP_PKEY_CTX); if (!cms_sd_asn1_ctrl(si, 1)) goto err; @@ -1040,8 +1053,11 @@ int CMS_SignerInfo_verify_content(CMS_SignerInfo *si, BIO *chain) if (EVP_PKEY_CTX_set_signature_md(pkctx, md) <= 0) goto err; si->pctx = pkctx; - if (!cms_sd_asn1_ctrl(si, 1)) + if (!cms_sd_asn1_ctrl(si, 1)) { + si->pctx = NULL; goto err; + } + si->pctx = NULL; r = EVP_PKEY_verify(pkctx, si->signature->data, si->signature->length, mval, mlen); if (r <= 0) { diff --git a/crypto/cms/cms_smime.c b/crypto/cms/cms_smime.c index 99a72f4dff..4df4487cbc 100644 --- a/crypto/cms/cms_smime.c +++ b/crypto/cms/cms_smime.c @@ -236,7 +236,7 @@ CMS_ContentInfo *CMS_EncryptedData_encrypt_ex(BIO *in, const EVP_CIPHER *cipher, if (cms == NULL) return NULL; if (!CMS_EncryptedData_set1_key(cms, cipher, key, keylen)) - return NULL; + goto err; if (!(flags & CMS_DETACHED)) CMS_set_detached(cms, 0); @@ -245,6 +245,7 @@ CMS_ContentInfo *CMS_EncryptedData_encrypt_ex(BIO *in, const EVP_CIPHER *cipher, || CMS_final(cms, in, NULL, flags)) return cms; + err: CMS_ContentInfo_free(cms); return NULL; } diff --git a/test/recipes/80-test_cms.t b/test/recipes/80-test_cms.t index 6a9792128b..07014945a7 100644 --- a/test/recipes/80-test_cms.t +++ b/test/recipes/80-test_cms.t @@ -394,6 +394,13 @@ my @smime_cms_tests = ( "-out", "{output}.txt" ], \&final_compare ], + + [ "encrypted content test streaming PEM format -noout, 128 bit AES key", + [ "{cmd1}", @prov, "-EncryptedData_encrypt", "-in", $smcont, "-outform", "PEM", + "-aes128", "-secretkey", "000102030405060708090A0B0C0D0E0F", + "-stream", "-noout" ], + [ "{cmd2}", @prov, "-help" ] + ], ); my @smime_cms_cades_tests = (