Fix various NULL dereferences in PKCS #12

The PKCS #7 ContentInfo has a mandatory contentType, but the content itself
is OPTIONAL. Various unpacking API assumed presence of the content type is
enough to access members of the content, resulting in crashes.

Reported by Bahaa Naamneh on libressl-security, many thanks

ok jsing

diff --git a/lib/libcrypto/pkcs12/p12_add.c b/lib/libcrypto/pkcs12/p12_add.c
index 93c7c72..8ce1fed 100644 (file)
--- a/lib/libcrypto/pkcs12/p12_add.c
+++ b/lib/libcrypto/pkcs12/p12_add.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: p12_add.c,v 1.22 2023/02/16 08:38:17 tb Exp $ */
+/* $OpenBSD: p12_add.c,v 1.23 2024/01/25 13:44:08 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 1999.
  */
@@ -124,11 +124,15 @@ LCRYPTO_ALIAS(PKCS12_pack_p7data);
 STACK_OF(PKCS12_SAFEBAG) *
 PKCS12_unpack_p7data(PKCS7 *p7)
 {
+       ASN1_OCTET_STRING *aos;
+
        if (!PKCS7_type_is_data(p7)) {
                PKCS12error(PKCS12_R_CONTENT_TYPE_NOT_DATA);
                return NULL;
        }
-       return ASN1_item_unpack(p7->d.data, &PKCS12_SAFEBAGS_it);
+       if ((aos = PKCS7_get_octet_string(p7)) == NULL)
+               return NULL;
+       return ASN1_item_unpack(aos, &PKCS12_SAFEBAGS_it);
 }
 LCRYPTO_ALIAS(PKCS12_unpack_p7data);
 
@@ -182,11 +186,16 @@ LCRYPTO_ALIAS(PKCS12_pack_p7encdata);
 STACK_OF(PKCS12_SAFEBAG) *
 PKCS12_unpack_p7encdata(PKCS7 *p7, const char *pass, int passlen)
 {
+       PKCS7_ENC_CONTENT *content;
+
        if (!PKCS7_type_is_encrypted(p7))
                return NULL;
-       return PKCS12_item_decrypt_d2i(p7->d.encrypted->enc_data->algorithm,
-           &PKCS12_SAFEBAGS_it, pass, passlen,
-           p7->d.encrypted->enc_data->enc_data, 1);
+       if (p7->d.encrypted == NULL)
+               return NULL;
+       if ((content = p7->d.encrypted->enc_data) == NULL)
+               return NULL;
+       return PKCS12_item_decrypt_d2i(content->algorithm, &PKCS12_SAFEBAGS_it,
+           pass, passlen, content->enc_data, 1);
 }
 LCRYPTO_ALIAS(PKCS12_unpack_p7encdata);
 
@@ -210,11 +219,14 @@ LCRYPTO_ALIAS(PKCS12_pack_authsafes);
 STACK_OF(PKCS7) *
 PKCS12_unpack_authsafes(const PKCS12 *p12)
 {
+       ASN1_OCTET_STRING *aos;
+
        if (!PKCS7_type_is_data(p12->authsafes)) {
                PKCS12error(PKCS12_R_CONTENT_TYPE_NOT_DATA);
                return NULL;
        }
-       return ASN1_item_unpack(p12->authsafes->d.data,
-           &PKCS12_AUTHSAFES_it);
+       if ((aos = PKCS7_get_octet_string(p12->authsafes)) == NULL)
+               return NULL;
+       return ASN1_item_unpack(aos, &PKCS12_AUTHSAFES_it);
 }
 LCRYPTO_ALIAS(PKCS12_unpack_authsafes);

diff --git a/lib/libcrypto/pkcs12/p12_mutl.c b/lib/libcrypto/pkcs12/p12_mutl.c
index f0e6df9..c71ed73 100644 (file)
--- a/lib/libcrypto/pkcs12/p12_mutl.c
+++ b/lib/libcrypto/pkcs12/p12_mutl.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: p12_mutl.c,v 1.35 2023/02/16 08:38:17 tb Exp $ */
+/* $OpenBSD: p12_mutl.c,v 1.36 2024/01/25 13:44:08 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 1999.
  */
@@ -115,6 +115,7 @@ PKCS12_gen_mac(PKCS12 *p12, const char *pass, int passlen,
 {
        const EVP_MD *md_type;
        HMAC_CTX *hmac = NULL;
+       ASN1_OCTET_STRING *aos;
        unsigned char key[EVP_MAX_MD_SIZE], *salt;
        int saltlen, iter;
        int md_size;
@@ -124,6 +125,10 @@ PKCS12_gen_mac(PKCS12 *p12, const char *pass, int passlen,
                PKCS12error(PKCS12_R_CONTENT_TYPE_NOT_DATA);
                goto err;
        }
+       if ((aos = PKCS7_get_octet_string(p12->authsafes)) == NULL) {
+               PKCS12error(PKCS12_R_DECODE_ERROR);
+               goto err;
+       }
 
        salt = p12->mac->salt->data;
        saltlen = p12->mac->salt->length;
@@ -155,8 +160,7 @@ PKCS12_gen_mac(PKCS12 *p12, const char *pass, int passlen,
                goto err;
        if (!HMAC_Init_ex(hmac, key, md_size, md_type, NULL))
                goto err;
-       if (!HMAC_Update(hmac, p12->authsafes->d.data->data,
-           p12->authsafes->d.data->length))
+       if (!HMAC_Update(hmac, aos->data, aos->length))
                goto err;
        if (!HMAC_Final(hmac, mac, maclen))
                goto err;

diff --git a/lib/libcrypto/pkcs12/pkcs12_local.h b/lib/libcrypto/pkcs12/pkcs12_local.h
index 1d6f055..8d82d2f 100644 (file)
--- a/lib/libcrypto/pkcs12/pkcs12_local.h
+++ b/lib/libcrypto/pkcs12/pkcs12_local.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: pkcs12_local.h,v 1.3 2022/11/26 17:23:18 tb Exp $ */
+/* $OpenBSD: pkcs12_local.h,v 1.4 2024/01/25 13:44:08 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 1999.
  */
@@ -96,6 +96,9 @@ struct pkcs12_bag_st {
        } value;
 };
 
+/* XXX - should go into pkcs7_local.h. */
+ASN1_OCTET_STRING *PKCS7_get_octet_string(PKCS7 *p7);
+
 __END_HIDDEN_DECLS
 
 #endif /* !HEADER_PKCS12_LOCAL_H */

diff --git a/lib/libcrypto/pkcs7/pk7_doit.c b/lib/libcrypto/pkcs7/pk7_doit.c
index 759d9dd..ce0e99e 100644 (file)
--- a/lib/libcrypto/pkcs7/pk7_doit.c
+++ b/lib/libcrypto/pkcs7/pk7_doit.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: pk7_doit.c,v 1.54 2023/11/15 00:55:43 tb Exp $ */
+/* $OpenBSD: pk7_doit.c,v 1.55 2024/01/25 13:44:08 tb Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -92,7 +92,7 @@ PKCS7_type_is_other(PKCS7* p7)
 
 }
 
-static ASN1_OCTET_STRING *
+ASN1_OCTET_STRING *
 PKCS7_get_octet_string(PKCS7 *p7)
 {
        if (PKCS7_type_is_data(p7))

diff --git a/lib/libcrypto/pkcs7/pk7_mime.c b/lib/libcrypto/pkcs7/pk7_mime.c
index f00e18c..3813355 100644 (file)
--- a/lib/libcrypto/pkcs7/pk7_mime.c
+++ b/lib/libcrypto/pkcs7/pk7_mime.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: pk7_mime.c,v 1.19 2023/05/02 09:56:12 tb Exp $ */
+/* $OpenBSD: pk7_mime.c,v 1.20 2024/01/25 13:44:08 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project.
  */
@@ -89,8 +89,11 @@ SMIME_write_PKCS7(BIO *bio, PKCS7 *p7, BIO *data, int flags)
        STACK_OF(X509_ALGOR) *mdalgs = NULL;
        int ctype_nid;
 
-       if ((ctype_nid = OBJ_obj2nid(p7->type)) == NID_pkcs7_signed)
+       if ((ctype_nid = OBJ_obj2nid(p7->type)) == NID_pkcs7_signed) {
+               if (p7->d.sign == NULL)
+                       return 0;
                mdalgs = p7->d.sign->md_algs;
+       }
 
        flags ^= SMIME_OLDMIME;