From c225c3cf9bd67297fb0c297768d69cbc03fbdab7 Mon Sep 17 00:00:00 2001 From: Emilia Kasper Date: Fri, 27 Feb 2015 16:52:23 +0100 Subject: [PATCH] PKCS#7: avoid NULL pointer dereferences with missing content In PKCS#7, the ASN.1 content component is optional. This typically applies to inner content (detached signatures), however we must also handle unexpected missing outer content correctly. This patch only addresses functions reachable from parsing, decryption and verification, and functions otherwise associated with reading potentially untrusted data. Correcting all low-level API calls requires further work. CVE-2015-0289 Thanks to Michal Zalewski (Google) for reporting this issue. Reviewed-by: Steve Henson --- crypto/pkcs7/pk7_doit.c | 87 ++++++++++++++++++++++++++++++++++------- crypto/pkcs7/pk7_lib.c | 3 ++ 2 files changed, 76 insertions(+), 14 deletions(-) diff --git a/crypto/pkcs7/pk7_doit.c b/crypto/pkcs7/pk7_doit.c index f61d63b39b3a2..25ac2df8379bb 100644 --- a/crypto/pkcs7/pk7_doit.c +++ b/crypto/pkcs7/pk7_doit.c @@ -261,6 +261,25 @@ BIO *PKCS7_dataInit(PKCS7 *p7, BIO *bio) PKCS7_RECIP_INFO *ri = NULL; ASN1_OCTET_STRING *os = NULL; + if (p7 == NULL) { + PKCS7err(PKCS7_F_PKCS7_DATAINIT, PKCS7_R_INVALID_NULL_POINTER); + return NULL; + } + /* + * The content field in the PKCS7 ContentInfo is optional, but that really + * only applies to inner content (precisely, detached signatures). + * + * When reading content, missing outer content is therefore treated as an + * error. + * + * When creating content, PKCS7_content_new() must be called before + * calling this method, so a NULL p7->d is always an error. + */ + if (p7->d.ptr == NULL) { + PKCS7err(PKCS7_F_PKCS7_DATAINIT, PKCS7_R_NO_CONTENT); + return NULL; + } + i = OBJ_obj2nid(p7->type); p7->state = PKCS7_S_HEADER; @@ -411,6 +430,16 @@ BIO *PKCS7_dataDecode(PKCS7 *p7, EVP_PKEY *pkey, BIO *in_bio, X509 *pcert) unsigned char *ek = NULL, *tkey = NULL; int eklen = 0, tkeylen = 0; + if (p7 == NULL) { + PKCS7err(PKCS7_F_PKCS7_DATADECODE, PKCS7_R_INVALID_NULL_POINTER); + return NULL; + } + + if (p7->d.ptr == NULL) { + PKCS7err(PKCS7_F_PKCS7_DATADECODE, PKCS7_R_NO_CONTENT); + return NULL; + } + i = OBJ_obj2nid(p7->type); p7->state = PKCS7_S_HEADER; @@ -683,6 +712,16 @@ int PKCS7_dataFinal(PKCS7 *p7, BIO *bio) STACK_OF(PKCS7_SIGNER_INFO) *si_sk = NULL; ASN1_OCTET_STRING *os = NULL; + if (p7 == NULL) { + PKCS7err(PKCS7_F_PKCS7_DATAFINAL, PKCS7_R_INVALID_NULL_POINTER); + return 0; + } + + if (p7->d.ptr == NULL) { + PKCS7err(PKCS7_F_PKCS7_DATAFINAL, PKCS7_R_NO_CONTENT); + return 0; + } + EVP_MD_CTX_init(&ctx_tmp); i = OBJ_obj2nid(p7->type); p7->state = PKCS7_S_HEADER; @@ -722,6 +761,7 @@ int PKCS7_dataFinal(PKCS7 *p7, BIO *bio) /* If detached data then the content is excluded */ if (PKCS7_type_is_data(p7->d.sign->contents) && p7->detached) { M_ASN1_OCTET_STRING_free(os); + os = NULL; p7->d.sign->contents->d.data = NULL; } break; @@ -731,6 +771,7 @@ int PKCS7_dataFinal(PKCS7 *p7, BIO *bio) /* If detached data then the content is excluded */ if (PKCS7_type_is_data(p7->d.digest->contents) && p7->detached) { M_ASN1_OCTET_STRING_free(os); + os = NULL; p7->d.digest->contents->d.data = NULL; } break; @@ -796,22 +837,30 @@ int PKCS7_dataFinal(PKCS7 *p7, BIO *bio) M_ASN1_OCTET_STRING_set(p7->d.digest->digest, md_data, md_len); } - if (!PKCS7_is_detached(p7) && !(os->flags & ASN1_STRING_FLAG_NDEF)) { - char *cont; - long contlen; - btmp = BIO_find_type(bio, BIO_TYPE_MEM); - if (btmp == NULL) { - PKCS7err(PKCS7_F_PKCS7_DATAFINAL, PKCS7_R_UNABLE_TO_FIND_MEM_BIO); - goto err; - } - contlen = BIO_get_mem_data(btmp, &cont); + if (!PKCS7_is_detached(p7)) { /* - * Mark the BIO read only then we can use its copy of the data - * instead of making an extra copy. + * NOTE(emilia): I think we only reach os == NULL here because detached + * digested data support is broken. */ - BIO_set_flags(btmp, BIO_FLAGS_MEM_RDONLY); - BIO_set_mem_eof_return(btmp, 0); - ASN1_STRING_set0(os, (unsigned char *)cont, contlen); + if (os == NULL) + goto err; + if (!(os->flags & ASN1_STRING_FLAG_NDEF)) { + char *cont; + long contlen; + btmp = BIO_find_type(bio, BIO_TYPE_MEM); + if (btmp == NULL) { + PKCS7err(PKCS7_F_PKCS7_DATAFINAL, PKCS7_R_UNABLE_TO_FIND_MEM_BIO); + goto err; + } + contlen = BIO_get_mem_data(btmp, &cont); + /* + * Mark the BIO read only then we can use its copy of the data + * instead of making an extra copy. + */ + BIO_set_flags(btmp, BIO_FLAGS_MEM_RDONLY); + BIO_set_mem_eof_return(btmp, 0); + ASN1_STRING_set0(os, (unsigned char *)cont, contlen); + } } ret = 1; err: @@ -886,6 +935,16 @@ int PKCS7_dataVerify(X509_STORE *cert_store, X509_STORE_CTX *ctx, BIO *bio, STACK_OF(X509) *cert; X509 *x509; + if (p7 == NULL) { + PKCS7err(PKCS7_F_PKCS7_DATAVERIFY, PKCS7_R_INVALID_NULL_POINTER); + return 0; + } + + if (p7->d.ptr == NULL) { + PKCS7err(PKCS7_F_PKCS7_DATAVERIFY, PKCS7_R_NO_CONTENT); + return 0; + } + if (PKCS7_type_is_signed(p7)) { cert = p7->d.sign->cert; } else if (PKCS7_type_is_signedAndEnveloped(p7)) { diff --git a/crypto/pkcs7/pk7_lib.c b/crypto/pkcs7/pk7_lib.c index c7738127564fe..0c5fcaa6aa2dd 100644 --- a/crypto/pkcs7/pk7_lib.c +++ b/crypto/pkcs7/pk7_lib.c @@ -70,6 +70,7 @@ long PKCS7_ctrl(PKCS7 *p7, int cmd, long larg, char *parg) nid = OBJ_obj2nid(p7->type); switch (cmd) { + /* NOTE(emilia): does not support detached digested data. */ case PKCS7_OP_SET_DETACHED_SIGNATURE: if (nid == NID_pkcs7_signed) { ret = p7->detached = (int)larg; @@ -444,6 +445,8 @@ int PKCS7_set_digest(PKCS7 *p7, const EVP_MD *md) STACK_OF(PKCS7_SIGNER_INFO) *PKCS7_get_signer_info(PKCS7 *p7) { + if (p7 == NULL || p7->d.ptr == NULL) + return NULL; if (PKCS7_type_is_signed(p7)) { return (p7->d.sign->signer_info); } else if (PKCS7_type_is_signedAndEnveloped(p7)) {