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 <emcmil...@microsoft.com>; Jiang, Guomin > <guomin.ji...@intel.com>; Wang, Jian J > <jian.j.w...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Michael Kubacki > <mikub...@linux.microsoft.com>; Lu, Xiaoyu1 > <xiaoyu1...@intel.com> > Subject: [edk2-devel] [PATCH v1 04/12] CryptoPkg: Fix conditionally > uninitialized variable > > From: Michael Kubacki <michael.kuba...@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 <emcmil...@microsoft.com> > Cc: Guomin Jiang <guomin.ji...@intel.com> > Cc: Jian J Wang <jian.j.w...@intel.com> > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Michael Kubacki <mikub...@linux.microsoft.com> > Cc: Xiaoyu Lu <xiaoyu1...@intel.com> > Co-authored-by: Erich McMillan <emcmil...@microsoft.com> > Signed-off-by: Michael Kubacki <michael.kuba...@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+ow...@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kin...@intel.com] > -=-=-=-=-=-= > -=-=-=-=-=-=-=-=-=-=-=- 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: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
