devel@edk2.groups.io | [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable

Home Messages Hashtags Subgroups Calendar

Date Date 1 - 3 of 3

[PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable

Michael Kubacki

From: Michael Kubacki <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 <emcmillan@...>
Cc: Guomin Jiang <guomin.jiang@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Michael Kubacki <mikuback@...>
Cc: Xiaoyu Lu <xiaoyu1.lu@...>
Co-authored-by: Erich McMillan <emcmillan@...>
Signed-off-by: Michael Kubacki <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/Li=
brary/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;
=20
   //
   // Check input parameters.
@@ -836,9 +837,9 @@ X509GetTBSCert (
   //
   Temp   =3D Cert;
   Length =3D 0;
-  ASN1_get_object (&Temp, (long *)&Length, (int *)&Asn1Tag, (int *)&ObjC=
lass, (long)CertSize);
+  Inf    =3D ASN1_get_object (&Temp, (long *)&Length, (int *)&Asn1Tag, (=
int *)&ObjClass, (long)CertSize);
=20
-  if (Asn1Tag !=3D V_ASN1_SEQUENCE) {
+  if (!(Inf & 0x80) && (Asn1Tag !=3D V_ASN1_SEQUENCE)) {
     return FALSE;
   }
=20
@@ -848,7 +849,7 @@ X509GetTBSCert (
   //
   // Verify the parsed TBSCertificate is one correct SEQUENCE data.
   //
-  if (Asn1Tag !=3D V_ASN1_SEQUENCE) {
+  if (!(Inf & 0x80) && (Asn1Tag !=3D V_ASN1_SEQUENCE)) {
     return FALSE;
   }
=20
@@ -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;
=20
   //
   // Save Ptr position
   //
   PtrOld =3D *Ptr;
=20
-  ASN1_get_object ((CONST UINT8 **)Ptr, &ObjLength, &ObjTag, &ObjCls, (I=
NT32)(End - (*Ptr)));
-  if ((ObjTag =3D=3D (INT32)(Tag & CRYPTO_ASN1_TAG_VALUE_MASK)) &&
+  Inf =3D ASN1_get_object ((CONST UINT8 **)Ptr, &ObjLength, &ObjTag, &Ob=
jCls, (INT32)(End - (*Ptr)));
+  if (!(Inf & 0x80) &&
+      (ObjTag =3D=3D (INT32)(Tag & CRYPTO_ASN1_TAG_VALUE_MASK)) &&
       (ObjCls =3D=3D (INT32)(Tag & CRYPTO_ASN1_TAG_CLASS_MASK)))
   {
     *Length =3D (UINTN)ObjLength;
--=20
2.28.0.windows.1

Michael D Kinney

#96595

Hi Michael,

Comments below.

Mike

Show quoted text

-----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@...>; Jiang, Guomin <guomin.jiang@...>; Wang, Jian J
<jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Michael Kubacki <mikuback@...>; Lu, Xiaoyu1
<xiaoyu1.lu@...>
Subject: [edk2-devel] [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable

From: Michael Kubacki <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 <emcmillan@...>
Cc: Guomin Jiang <guomin.jiang@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Michael Kubacki <mikuback@...>
Cc: Xiaoyu Lu <xiaoyu1.lu@...>
Co-authored-by: Erich McMillan <emcmillan@...>
Signed-off-by: Michael Kubacki <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@...]
-=-=-=-=-=-=

Michael Kubacki

#96597

Thanks. I'll make the suggested change in the v2 series.

Show quoted text

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@...>; Jiang, Guomin <guomin.jiang@...>; Wang, Jian J
<jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Michael Kubacki <mikuback@...>; Lu, Xiaoyu1
<xiaoyu1.lu@...>
Subject: [edk2-devel] [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable

From: Michael Kubacki <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 <emcmillan@...>
Cc: Guomin Jiang <guomin.jiang@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Michael Kubacki <mikuback@...>
Cc: Xiaoyu Lu <xiaoyu1.lu@...>
Co-authored-by: Erich McMillan <emcmillan@...>
Signed-off-by: Michael Kubacki <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@...]
-=-=-=-=-=-=

1 - 3 of 3

Previous Topic Next Topic

Home Hashtags Subgroups Calendar About Features Pricing Updates Terms Help Twitter