From: "Michael Kubacki" <mikuback@linux.microsoft.com>
To: devel@edk2.groups.io, michael.d.kinney@intel.com
Cc: Erich McMillan <emcmillan@microsoft.com>,
"Jiang, Guomin" <guomin.jiang@intel.com>,
"Wang, Jian J" <jian.j.wang@intel.com>,
"Yao, Jiewen" <jiewen.yao@intel.com>,
"Lu, Xiaoyu1" <xiaoyu1.lu@intel.com>
Subject: Re: [edk2-devel] [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable
Date: Wed, 23 Nov 2022 20:47:12 -0500 [thread overview]
Message-ID: <5bb4deeb-d06b-b63d-c93d-0cd30b1bc4a2@linux.microsoft.com> (raw)
In-Reply-To: <CO1PR11MB492984F027499AFB6D49D918D20F9@CO1PR11MB4929.namprd11.prod.outlook.com>
Thanks. I'll make the suggested change in the v2 series.
On 11/23/2022 8:37 PM, Michael D Kinney wrote:
> Hi Michael,
>
> Comments below.
>
> Mike
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
>> Sent: Wednesday, November 9, 2022 9:33 AM
>> To: devel@edk2.groups.io
>> Cc: Erich McMillan <emcmillan@microsoft.com>; Jiang, Guomin <guomin.jiang@intel.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Lu, Xiaoyu1
>> <xiaoyu1.lu@intel.com>
>> Subject: [edk2-devel] [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Fixes CodeQL alerts for CWE-457:
>> https://cwe.mitre.org/data/definitions/457.html
>>
>> Checks the return value from `ASN1_get_object()` to verify values
>> set by the function are valid.
>>
>> Note that the function returns literal `0x80`:
>> `return (0x80);`
>>
>> That is used to check the return value is as the case in other areas
>> of the code.
>>
>> Cc: Erich McMillan <emcmillan@microsoft.com>
>> Cc: Guomin Jiang <guomin.jiang@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
>> Cc: Xiaoyu Lu <xiaoyu1.lu@intel.com>
>> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>> CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c | 21 +++++++++++---------
>> 1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
>> index 2333157e0d17..f867656e888c 100644
>> --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
>> +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
>> @@ -807,6 +807,7 @@ X509GetTBSCert (
>> UINT32 Asn1Tag;
>> UINT32 ObjClass;
>> UINTN Length;
>> + UINTN Inf;
>>
>> //
>> // Check input parameters.
>> @@ -836,9 +837,9 @@ X509GetTBSCert (
>> //
>> Temp = Cert;
>> Length = 0;
>> - ASN1_get_object (&Temp, (long *)&Length, (int *)&Asn1Tag, (int *)&ObjClass, (long)CertSize);
>> + Inf = ASN1_get_object (&Temp, (long *)&Length, (int *)&Asn1Tag, (int *)&ObjClass, (long)CertSize);
>>
>> - if (Asn1Tag != V_ASN1_SEQUENCE) {
>> + if (!(Inf & 0x80) && (Asn1Tag != V_ASN1_SEQUENCE)) {
>
> The result of bitwise operation (Inf & 0x80) is a value, not a BOOLEAN.
> I think the more correct way to do this check is ((Inf & 0x80) == 0x00).
>
>> return FALSE;
>> }
>>
>> @@ -848,7 +849,7 @@ X509GetTBSCert (
>> //
>> // Verify the parsed TBSCertificate is one correct SEQUENCE data.
>> //
>> - if (Asn1Tag != V_ASN1_SEQUENCE) {
>> + if (!(Inf & 0x80) && (Asn1Tag != V_ASN1_SEQUENCE)) {
>
> The result of bitwise operation (Inf & 0x80) is a value, not a BOOLEAN.
> I think the more correct way to do this check is ((Inf & 0x80) == 0x00).
>
>> return FALSE;
>> }
>>
>> @@ -1888,18 +1889,20 @@ Asn1GetTag (
>> IN UINT32 Tag
>> )
>> {
>> - UINT8 *PtrOld;
>> - INT32 ObjTag;
>> - INT32 ObjCls;
>> - long ObjLength;
>> + UINT8 *PtrOld;
>> + INT32 ObjTag;
>> + INT32 ObjCls;
>> + long ObjLength;
>> + UINT32 Inf;
>>
>> //
>> // Save Ptr position
>> //
>> PtrOld = *Ptr;
>>
>> - ASN1_get_object ((CONST UINT8 **)Ptr, &ObjLength, &ObjTag, &ObjCls, (INT32)(End - (*Ptr)));
>> - if ((ObjTag == (INT32)(Tag & CRYPTO_ASN1_TAG_VALUE_MASK)) &&
>> + Inf = ASN1_get_object ((CONST UINT8 **)Ptr, &ObjLength, &ObjTag, &ObjCls, (INT32)(End - (*Ptr)));
>> + if (!(Inf & 0x80) &&
>
> The result of bitwise operation (Inf & 0x80) is a value, not a BOOLEAN.
> I think the more correct way to do this check is ((Inf & 0x80) == 0x00).
>
>> + (ObjTag == (INT32)(Tag & CRYPTO_ASN1_TAG_VALUE_MASK)) &&
>> (ObjCls == (INT32)(Tag & CRYPTO_ASN1_TAG_CLASS_MASK)))
>> {
>> *Length = (UINTN)ObjLength;
>> --
>> 2.28.0.windows.1
>>
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#96150): https://edk2.groups.io/g/devel/message/96150
>> Mute This Topic: https://groups.io/mt/94918089/1643496
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
>> -=-=-=-=-=-=
>>
>
>
>
>
>
>
next prev parent reply other threads:[~2022-11-24 1:47 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts Michael Kubacki
2022-11-24 1:28 ` [edk2-devel] " Michael D Kinney
2022-11-24 1:46 ` Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 02/12] BaseTools/PatchCheck.py: Add PCCTS to tab exemption list Michael Kubacki
2022-11-24 1:30 ` Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 03/12] BaseTools/VfrCompile: Fix potential buffer overwrites Michael Kubacki
2022-11-24 1:32 ` [edk2-devel] " Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable Michael Kubacki
2022-11-24 1:37 ` [edk2-devel] " Michael D Kinney
2022-11-24 1:47 ` Michael Kubacki [this message]
2022-11-09 17:32 ` [PATCH v1 05/12] MdeModulePkg: Fix conditionally uninitialized variables Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 06/12] MdePkg: " Michael Kubacki
2022-11-24 1:53 ` Michael D Kinney
2022-11-24 1:59 ` Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 07/12] NetworkPkg: " Michael Kubacki
2022-11-24 1:59 ` Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 08/12] PcAtChipsetPkg: " Michael Kubacki
2022-11-24 2:00 ` Michael D Kinney
2022-11-24 5:01 ` Ni, Ray
2022-11-09 17:32 ` [PATCH v1 09/12] ShellPkg: " Michael Kubacki
2022-11-24 2:19 ` Gao, Zhichao
2022-11-24 2:36 ` [edk2-devel] " Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 10/12] UefiCpuPkg: " Michael Kubacki
2022-11-24 2:04 ` [edk2-devel] " Michael D Kinney
2022-11-24 2:14 ` Michael Kubacki
2022-11-24 2:31 ` Michael D Kinney
2022-11-24 5:12 ` Ni, Ray
2022-11-28 22:50 ` Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 11/12] .github/codeql/edk2.qls: Enable CWE 457, 676, and 758 queries Michael Kubacki
2022-11-24 2:05 ` [edk2-devel] " Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 12/12] .github/codeql/edk2.qls: Enable CWE 120, 787, and 805 queries Michael Kubacki
2022-11-24 2:06 ` Michael D Kinney
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5bb4deeb-d06b-b63d-c93d-0cd30b1bc4a2@linux.microsoft.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox