From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.13612.1669254435060161544 for ; Wed, 23 Nov 2022 17:47:15 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=RMN1F69b; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.201.8.94]) by linux.microsoft.com (Postfix) with ESMTPSA id D874920B6C40; Wed, 23 Nov 2022 17:47:13 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com D874920B6C40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1669254434; bh=LqtKnHQSQtlqB0yhG73LgraQ9fpsnaJA+NkBCfgO5yg=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=RMN1F69boll9lRnAbqa51w8ffwoR2t9kZLr4EazBQnZ0Gs4ZsdqebQHicOTHZGLPG 28Kg9S33HUTjlAYL9JCb1Mda2/RXNfs+vg4+hN3dQGKrnWL9bHxSP8z0xxoKqmjd64 GaTqHV0njVsYHLKhLo1UxxamUKt7bNOveRaZJKiA= Message-ID: <5bb4deeb-d06b-b63d-c93d-0cd30b1bc4a2@linux.microsoft.com> Date: Wed, 23 Nov 2022 20:47:12 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.1 Subject: Re: [edk2-devel] [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable To: devel@edk2.groups.io, michael.d.kinney@intel.com Cc: Erich McMillan , "Jiang, Guomin" , "Wang, Jian J" , "Yao, Jiewen" , "Lu, Xiaoyu1" References: <20221109173246.174-1-mikuback@linux.microsoft.com> <20221109173246.174-5-mikuback@linux.microsoft.com> From: "Michael Kubacki" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 On Behalf Of Michael Kubacki >> Sent: Wednesday, November 9, 2022 9:33 AM >> To: devel@edk2.groups.io >> Cc: Erich McMillan ; Jiang, Guomin ; Wang, Jian J >> ; Yao, Jiewen ; Michael Kubacki ; Lu, Xiaoyu1 >> >> Subject: [edk2-devel] [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable >> >> From: Michael Kubacki >> >> 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 >> Cc: Guomin Jiang >> Cc: Jian J Wang >> Cc: Jiewen Yao >> Cc: Michael Kubacki >> Cc: Xiaoyu Lu >> Co-authored-by: Erich McMillan >> Signed-off-by: Michael Kubacki >> --- >> 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] >> -=-=-=-=-=-= >> > > > > > >