Hi Michael, Comments below.
Mike > -----Original Message----- > From: [email protected] <[email protected]> On Behalf Of Michael Kubacki > Sent: Wednesday, November 9, 2022 9:33 AM > To: [email protected] > Cc: Erich McMillan <[email protected]>; Jiang, Guomin > <[email protected]>; Wang, Jian J > <[email protected]>; Yao, Jiewen <[email protected]>; Michael Kubacki > <[email protected]>; Lu, Xiaoyu1 > <[email protected]> > Subject: [edk2-devel] [PATCH v1 04/12] CryptoPkg: Fix conditionally > uninitialized variable > > From: Michael Kubacki <[email protected]> > > 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 <[email protected]> > Cc: Guomin Jiang <[email protected]> > Cc: Jian J Wang <[email protected]> > Cc: Jiewen Yao <[email protected]> > Cc: Michael Kubacki <[email protected]> > Cc: Xiaoyu Lu <[email protected]> > Co-authored-by: Erich McMillan <[email protected]> > Signed-off-by: Michael Kubacki <[email protected]> > --- > 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: [email protected] > Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] > -=-=-=-=-=-= > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#96595): https://edk2.groups.io/g/devel/message/96595 Mute This Topic: https://groups.io/mt/94918089/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
