REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1914
AuthenticodeVerify() calls OpenSSLs d2i_PKCS7() API to parse asn encoded
signed authenticode pkcs#7 data. when this successfully returns, a type
check is done by calling PKCS7_type_is_signed() and then
Pkcs7->d.sign->contents->type is used. It is possible to construct an asn1
blob that successfully decodes and have d2i_PKCS7() return a valid pointer
and have PKCS7_type_is_signed() also return success but have Pkcs7->d.sign
be a NULL pointer.
Looking at how PKCS7_verify() [inside of OpenSSL] implements checking for
pkcs7 structs it does the following:
- call PKCS7_type_is_signed()
- call PKCS7_get_detached()
Looking into how PKCS7_get_detatched() is implemented, it checks to see if
p7->d.sign is NULL or if p7->d.sign->contents->d.ptr is NULL.
As such, the fix is to do the same as OpenSSL after calling d2i_PKCS7().
- Add call to PKS7_get_detached() to existing error handling
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c
index 2772b1e2be..ae0ee61fb6 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c
@@ -9,7 +9,7 @@
AuthenticodeVerify() will get PE/COFF Authenticode and will do basic check for
data structure.
-Copyright (c) 2011 - 2015, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2011 - 2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -100,7 +100,7 @@ AuthenticodeVerify (
//
// Check if it's PKCS#7 Signed Data (for Authenticode Scenario)
//
- if (!PKCS7_type_is_signed (Pkcs7)) {
+ if (!PKCS7_type_is_signed (Pkcs7) || PKCS7_get_detached (Pkcs7)) {
goto _Exit;
}
--
2.19.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66309): https://edk2.groups.io/g/devel/message/66309
Mute This Topic: https://groups.io/mt/77544856/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 10/16/20 07:14, Wang, Jian J wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1914 > > AuthenticodeVerify() calls OpenSSLs d2i_PKCS7() API to parse asn encoded > signed authenticode pkcs#7 data. when this successfully returns, a type > check is done by calling PKCS7_type_is_signed() and then > Pkcs7->d.sign->contents->type is used. It is possible to construct an asn1 > blob that successfully decodes and have d2i_PKCS7() return a valid pointer > and have PKCS7_type_is_signed() also return success but have Pkcs7->d.sign > be a NULL pointer. > > Looking at how PKCS7_verify() [inside of OpenSSL] implements checking for > pkcs7 structs it does the following: > - call PKCS7_type_is_signed() > - call PKCS7_get_detached() > Looking into how PKCS7_get_detatched() is implemented, it checks to see if > p7->d.sign is NULL or if p7->d.sign->contents->d.ptr is NULL. > > As such, the fix is to do the same as OpenSSL after calling d2i_PKCS7(). > - Add call to PKS7_get_detached() to existing error handling > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> > Cc: Guomin Jiang <guomin.jiang@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com> > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > --- > CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c > index 2772b1e2be..ae0ee61fb6 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c > @@ -9,7 +9,7 @@ > AuthenticodeVerify() will get PE/COFF Authenticode and will do basic check for > data structure. > > -Copyright (c) 2011 - 2015, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2011 - 2019, Intel Corporation. All rights reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ It's probably best to bump the (C) date to 2020; if you must update the (C) line in the first place. Otherwise, this patch seems identical to what I reviewed in <https://bugzilla.tianocore.org/show_bug.cgi?id=1914#c6>, so my R-b stands. Thanks Laszlo > @@ -100,7 +100,7 @@ AuthenticodeVerify ( > // > // Check if it's PKCS#7 Signed Data (for Authenticode Scenario) > // > - if (!PKCS7_type_is_signed (Pkcs7)) { > + if (!PKCS7_type_is_signed (Pkcs7) || PKCS7_get_detached (Pkcs7)) { > goto _Exit; > } > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66399): https://edk2.groups.io/g/devel/message/66399 Mute This Topic: https://groups.io/mt/77544856/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Laszlo, > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > Sent: Tuesday, October 20, 2020 3:13 AM > To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com> > Cc: Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin > <guomin.jiang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com> > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/BaseCryptLib: fix NULL > dereference (CVE-2019-14584) > > On 10/16/20 07:14, Wang, Jian J wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1914 > > > > AuthenticodeVerify() calls OpenSSLs d2i_PKCS7() API to parse asn encoded > > signed authenticode pkcs#7 data. when this successfully returns, a type > > check is done by calling PKCS7_type_is_signed() and then > > Pkcs7->d.sign->contents->type is used. It is possible to construct an asn1 > > blob that successfully decodes and have d2i_PKCS7() return a valid pointer > > and have PKCS7_type_is_signed() also return success but have Pkcs7->d.sign > > be a NULL pointer. > > > > Looking at how PKCS7_verify() [inside of OpenSSL] implements checking for > > pkcs7 structs it does the following: > > - call PKCS7_type_is_signed() > > - call PKCS7_get_detached() > > Looking into how PKCS7_get_detatched() is implemented, it checks to see if > > p7->d.sign is NULL or if p7->d.sign->contents->d.ptr is NULL. > > > > As such, the fix is to do the same as OpenSSL after calling d2i_PKCS7(). > > - Add call to PKS7_get_detached() to existing error handling > > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> > > Cc: Guomin Jiang <guomin.jiang@intel.com> > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Signed-off-by: Jian J Wang <jian.j.wang@intel.com> > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > --- > > CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c > > index 2772b1e2be..ae0ee61fb6 100644 > > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c > > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c > > @@ -9,7 +9,7 @@ > > AuthenticodeVerify() will get PE/COFF Authenticode and will do basic check > for > > data structure. > > > > -Copyright (c) 2011 - 2015, Intel Corporation. All rights reserved.<BR> > > +Copyright (c) 2011 - 2019, Intel Corporation. All rights reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > It's probably best to bump the (C) date to 2020; if you must update the > (C) line in the first place. > > Otherwise, this patch seems identical to what I reviewed in > <https://bugzilla.tianocore.org/show_bug.cgi?id=1914#c6>, so my R-b stands. > Thanks for catching this. I'll update it before pushing. Regards, Jian > Thanks > Laszlo > > > @@ -100,7 +100,7 @@ AuthenticodeVerify ( > > // > > // Check if it's PKCS#7 Signed Data (for Authenticode Scenario) > > // > > - if (!PKCS7_type_is_signed (Pkcs7)) { > > + if (!PKCS7_type_is_signed (Pkcs7) || PKCS7_get_detached (Pkcs7)) { > > goto _Exit; > > } > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66439): https://edk2.groups.io/g/devel/message/66439 Mute This Topic: https://groups.io/mt/77544856/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com> > -----Original Message----- > From: Wang, Jian J <jian.j.wang@intel.com> > Sent: Friday, October 16, 2020 1:15 PM > To: devel@edk2.groups.io > Cc: Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin > <guomin.jiang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Laszlo > Ersek <lersek@redhat.com> > Subject: [PATCH] CryptoPkg/BaseCryptLib: fix NULL dereference (CVE-2019- > 14584) > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1914 > > > > AuthenticodeVerify() calls OpenSSLs d2i_PKCS7() API to parse asn encoded > > signed authenticode pkcs#7 data. when this successfully returns, a type > > check is done by calling PKCS7_type_is_signed() and then > > Pkcs7->d.sign->contents->type is used. It is possible to construct an asn1 > > blob that successfully decodes and have d2i_PKCS7() return a valid pointer > > and have PKCS7_type_is_signed() also return success but have Pkcs7->d.sign > > be a NULL pointer. > > > > Looking at how PKCS7_verify() [inside of OpenSSL] implements checking for > > pkcs7 structs it does the following: > > - call PKCS7_type_is_signed() > > - call PKCS7_get_detached() > > Looking into how PKCS7_get_detatched() is implemented, it checks to see if > > p7->d.sign is NULL or if p7->d.sign->contents->d.ptr is NULL. > > > > As such, the fix is to do the same as OpenSSL after calling d2i_PKCS7(). > > - Add call to PKS7_get_detached() to existing error handling > > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com> > > Cc: Guomin Jiang <guomin.jiang@intel.com> > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com> > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > --- > CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c > index 2772b1e2be..ae0ee61fb6 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c > @@ -9,7 +9,7 @@ > AuthenticodeVerify() will get PE/COFF Authenticode and will do basic check > for > > data structure. > > > > -Copyright (c) 2011 - 2015, Intel Corporation. All rights reserved.<BR> > > +Copyright (c) 2011 - 2019, Intel Corporation. All rights reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > @@ -100,7 +100,7 @@ AuthenticodeVerify ( > // > > // Check if it's PKCS#7 Signed Data (for Authenticode Scenario) > > // > > - if (!PKCS7_type_is_signed (Pkcs7)) { > > + if (!PKCS7_type_is_signed (Pkcs7) || PKCS7_get_detached (Pkcs7)) { > > goto _Exit; > > } > > > > -- > 2.19.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66353): https://edk2.groups.io/g/devel/message/66353 Mute This Topic: https://groups.io/mt/77544856/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.