CryptoPkg: CodeQL Fixes

Resolves CodeQL issues in the package. Co-authored-by: Aaron Pop <[email protected]> Co-authored-by: Kun Qin <[email protected]> Co-authored-by: Taylor Beebe <[email protected]> Co-authored-by: Doug Flick <[email protected]> Co-authored-by: Ken Lautner <[email protected]> Signed-off-by: Michael Kubacki <[email protected]>
microsoft · Jun 25, 2024 · 55e2ded · 55e2ded
1 parent a30086a
commit 55e2ded
Show file tree

Hide file tree

Showing 12 changed files with 170 additions and 35 deletions.
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c
@@ -110,6 +110,12 @@ AuthenticodeVerify (
   //       PKCS#7 ContentInfo here.
   //
   SpcIndirectDataOid = OBJ_get0_data (Pkcs7->d.sign->contents->type);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (SpcIndirectDataOid == NULL) {
+    goto _Exit;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   if ((OBJ_length (Pkcs7->d.sign->contents->type) != sizeof (mSpcIndirectOidValue)) ||
       (CompareMem (
          SpcIndirectDataOid,

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptDh.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptDh.c
@@ -163,8 +163,16 @@ DhSetParameter (
   return TRUE;
 
 Error:
-  BN_free (BnP);
-  BN_free (BnG);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (BnP != NULL) {
+    BN_free (BnP);
+  }
+
+  if (BnG != NULL) {
+    BN_free (BnG);
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   return FALSE;
 }

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptEc.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptEc.c
@@ -585,6 +585,11 @@ EcGetPubKey (
     return FALSE;
   }
 
+  // MU_CHANGE [BEGIN] - CodeQL change
+  BnX = NULL;
+  BnY = NULL;
+  // MU_CHANGE [END] - CodeQL change
+
   EcKey    = (EC_KEY *)EcContext;
   Group    = EC_KEY_get0_group (EcKey);
   HalfSize = (EC_GROUP_get_degree (Group) + 7) / 8;
@@ -628,8 +633,16 @@ EcGetPubKey (
   RetVal = TRUE;
 
 fail:
-  BN_free (BnX);
-  BN_free (BnY);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (BnX != NULL) {
+    BN_free (BnX);
+  }
+
+  if (BnY != NULL) {
+    BN_free (BnY);
+  }
+
+  // MU_CHANGE [END] - CodeQL change
   return RetVal;
 }
 

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c
@@ -288,6 +288,12 @@ CheckTSTInfo (
   //
   Imprint  = TstInfo->MessageImprint;
   HashAlgo = X509_ALGOR_dup (Imprint->HashAlgorithm);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if ((HashAlgo == NULL) || (HashAlgo->algorithm == NULL)) {
+    goto _Exit;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   Md = EVP_get_digestbyobj (HashAlgo->algorithm);
   if (Md == NULL) {

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
@@ -448,6 +448,17 @@ InternalX509GetNIDName (
   }
 
   EntryData = X509_NAME_ENTRY_get_data (Entry);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (EntryData == NULL) {
+    //
+    // Fail to retrieve name entry data
+    //
+    *CommonNameSize = 0;
+    ReturnStatus    = RETURN_NOT_FOUND;
+    goto _Exit;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   Length = ASN1_STRING_to_UTF8 (&UTF8Name, EntryData);
   if (Length < 0) {
@@ -809,6 +820,10 @@ X509GetTBSCert (
   UINTN        Length;
   UINTN        Inf;
 
+  // MU_CHANGE [BEGIN] - CodeQL change
+  Asn1Tag = (UINT32)V_ASN1_UNDEF;
+  // MU_CHANGE [END] - CodeQL change
+
   //
   // Check input parameters.
   //
@@ -1900,9 +1915,16 @@ Asn1GetTag (
   //
   PtrOld = *Ptr;
 
+  // MU_CHANGE [BEGIN] - CodeQL change
+  // ASN1_get_object ((CONST UINT8 **)Ptr, &ObjLength, &ObjTag, &ObjCls, (INT32)(End - (*Ptr)));
   Inf = ASN1_get_object ((CONST UINT8 **)Ptr, &ObjLength, &ObjTag, &ObjCls, (INT32)(End - (*Ptr)));
-  if (((Inf & 0x80) == 0x00) &&
-      (ObjTag == (INT32)(Tag & CRYPTO_ASN1_TAG_VALUE_MASK)) &&
+  if (Inf & 0x80) {
+    return FALSE;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
+
+  if ((ObjTag == (INT32)(Tag & CRYPTO_ASN1_TAG_VALUE_MASK)) &&
       (ObjCls == (INT32)(Tag & CRYPTO_ASN1_TAG_CLASS_MASK)))
   {
     *Length = (UINTN)ObjLength;

diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
@@ -421,7 +421,10 @@ qsort (
   // Use CRT-style malloc to cover BS and RT memory allocation.
   //
   Buffer = malloc (width);
-  ASSERT (Buffer != NULL);
+  if (Buffer == NULL) {
+    ASSERT (Buffer != NULL);
+    return;
+  }
 
   //
   // Re-use PerformQuickSort() function Implementation in EDKII BaseSortLib.

diff --git a/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/Pkcs5Pbkdf2Tests.c b/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/Pkcs5Pbkdf2Tests.c
@@ -33,6 +33,10 @@ TestVerifyPkcs5Pbkdf2 (
   UINT8    *OutKey;
 
   OutKey = AllocatePool (KeyLen);
+  if (OutKey == NULL) {
+    UT_LOG_ERROR ("Failed to allocate memory for OutKey.\n");
+    return UNIT_TEST_ERROR_TEST_FAILED;
+  }
 
   //
   // Verify PKCS#5 PBKDF2 Key Derivation Function

diff --git a/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/RsaPkcs7Tests.c b/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/RsaPkcs7Tests.c
@@ -246,18 +246,26 @@ TestVerifyRsaCertPkcs1SignVerify (
   IN UNIT_TEST_CONTEXT  Context
   )
 {
-  BOOLEAN        Status;
-  VOID           *RsaPrivKey;
-  VOID           *RsaPubKey;
-  UINT8          *Signature;
-  UINTN          SigSize;
-  UINT8          *Subject;
-  UINTN          SubjectSize;
-  RETURN_STATUS  ReturnStatus;
-  CHAR8          CommonName[64];
-  UINTN          CommonNameSize;
-  CHAR8          OrgName[64];
-  UINTN          OrgNameSize;
+  BOOLEAN           Status;
+  VOID              *RsaPrivKey;
+  VOID              *RsaPubKey;
+  UINT8             *Signature;
+  UINTN             SigSize;
+  UINT8             *Subject;
+  UINTN             SubjectSize;
+  RETURN_STATUS     ReturnStatus;
+  CHAR8             CommonName[64];
+  UINTN             CommonNameSize;
+  CHAR8             OrgName[64];
+  UINTN             OrgNameSize;
+  UNIT_TEST_STATUS  TestStatus;
+
+  RsaPrivKey = NULL;
+  RsaPubKey  = NULL;
+  Signature  = NULL;
+  Subject    = NULL;
+
+  TestStatus = UNIT_TEST_ERROR_TEST_FAILED;
 
   //
   // Retrieve RSA private key from encrypted PEM data.
@@ -281,7 +289,12 @@ TestVerifyRsaCertPkcs1SignVerify (
   UT_ASSERT_NOT_EQUAL (SigSize, 0);
 
   Signature = AllocatePool (SigSize);
-  Status    = RsaPkcs1Sign (RsaPrivKey, MsgHash, SHA1_DIGEST_SIZE, Signature, &SigSize);
+  if (Signature == NULL) {
+    UT_LOG_ERROR ("Failed to allocate memory for Signature.\n");
+    goto Exit;
+  }
+
+  Status = RsaPkcs1Sign (RsaPrivKey, MsgHash, SHA1_DIGEST_SIZE, Signature, &SigSize);
   UT_ASSERT_TRUE (Status);
 
   //
@@ -296,7 +309,12 @@ TestVerifyRsaCertPkcs1SignVerify (
   SubjectSize = 0;
   Status      = X509GetSubjectName (TestCert, sizeof (TestCert), NULL, &SubjectSize);
   Subject     = (UINT8 *)AllocatePool (SubjectSize);
-  Status      = X509GetSubjectName (TestCert, sizeof (TestCert), Subject, &SubjectSize);
+  if (Subject == NULL) {
+    UT_LOG_ERROR ("Failed to allocate memory for Subject.\n");
+    goto Exit;
+  }
+
+  Status = X509GetSubjectName (TestCert, sizeof (TestCert), Subject, &SubjectSize);
   UT_ASSERT_TRUE (Status);
 
   //
@@ -324,15 +342,28 @@ TestVerifyRsaCertPkcs1SignVerify (
   Status = X509VerifyCert (TestCert, sizeof (TestCert), TestCACert, sizeof (TestCACert));
   UT_ASSERT_TRUE (Status);
 
+  TestStatus = UNIT_TEST_PASSED;
+Exit:
   //
   // Release Resources.
   //
-  RsaFree (RsaPubKey);
-  RsaFree (RsaPrivKey);
-  FreePool (Signature);
-  FreePool (Subject);
+  if (Subject != NULL) {
+    FreePool (Subject);
+  }
 
-  return UNIT_TEST_PASSED;
+  if (Signature != NULL) {
+    FreePool (Signature);
+  }
+
+  if (RsaPubKey != NULL) {
+    RsaFree (RsaPubKey);
+  }
+
+  if (RsaPrivKey != NULL) {
+    RsaFree (RsaPrivKey);
+  }
+
+  return TestStatus;
 }
 
 UNIT_TEST_STATUS
@@ -369,8 +400,8 @@ TestVerifyPkcs7SignVerify (
              (CONST UINT8 *)PemPass,
              (UINT8 *)Payload,
              AsciiStrLen (Payload),
-             TestCert,            // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
-             sizeof (TestCert),   // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
+             TestCert,                   // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
+             sizeof (TestCert),          // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
              NULL,
              &P7SignedData,
              &P7SignedDataSize

diff --git a/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/RsaPssTests.c b/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/RsaPssTests.c
@@ -159,7 +159,12 @@ TestVerifyRsaPssSignVerify (
   UT_ASSERT_NOT_EQUAL (SigSize, 0);
 
   Signature = AllocatePool (SigSize);
-  Status    = RsaPssSign (mRsa, PssMessage, sizeof (PssMessage), SHA256_DIGEST_SIZE, SHA256_DIGEST_SIZE, Signature, &SigSize);
+  if (Signature == NULL) {
+    UT_LOG_ERROR ("Failed to allocate memory for Signature");
+    return UNIT_TEST_ERROR_TEST_FAILED;
+  }
+
+  Status = RsaPssSign (mRsa, PssMessage, sizeof (PssMessage), SHA256_DIGEST_SIZE, SHA256_DIGEST_SIZE, Signature, &SigSize);
   UT_ASSERT_TRUE (Status);
 
   //

diff --git a/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/RsaTests.c b/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/RsaTests.c
@@ -128,7 +128,12 @@ TestVerifyRsaSetGetKeyComponents (
   UT_ASSERT_EQUAL (KeySize, sizeof (RsaN));
 
   KeyBuffer = AllocatePool (KeySize);
-  Status    = RsaGetKey (mRsa, RsaKeyN, KeyBuffer, &KeySize);
+  if (KeyBuffer == NULL) {
+    UT_LOG_ERROR ("Failed to allocate memory for KeyBuffer");
+    return UNIT_TEST_ERROR_TEST_FAILED;
+  }
+
+  Status = RsaGetKey (mRsa, RsaKeyN, KeyBuffer, &KeySize);
   UT_ASSERT_TRUE (Status);
   UT_ASSERT_EQUAL (KeySize, sizeof (RsaN));
 
@@ -148,7 +153,12 @@ TestVerifyRsaSetGetKeyComponents (
   UT_ASSERT_EQUAL (KeySize, sizeof (RsaE));
 
   KeyBuffer = AllocatePool (KeySize);
-  Status    = RsaGetKey (mRsa, RsaKeyE, KeyBuffer, &KeySize);
+  if (KeyBuffer == NULL) {
+    UT_LOG_ERROR ("Failed to allocate memory for KeyBuffer");
+    return UNIT_TEST_ERROR_TEST_FAILED;
+  }
+
+  Status = RsaGetKey (mRsa, RsaKeyE, KeyBuffer, &KeySize);
   UT_ASSERT_TRUE (Status);
   UT_ASSERT_EQUAL (KeySize, sizeof (RsaE));
 
@@ -214,7 +224,12 @@ TestVerifyRsaGenerateKeyComponents (
 
   KeySize   = RSA_MODULUS_LENGTH / 8;
   KeyBuffer = AllocatePool (KeySize);
-  Status    = RsaGetKey (mRsa, RsaKeyE, KeyBuffer, &KeySize);
+  if (KeyBuffer == NULL) {
+    UT_LOG_ERROR ("Failed to allocate memory for KeyBuffer");
+    return UNIT_TEST_ERROR_TEST_FAILED;
+  }
+
+  Status = RsaGetKey (mRsa, RsaKeyE, KeyBuffer, &KeySize);
   UT_ASSERT_TRUE (Status);
   UT_ASSERT_EQUAL (KeySize, 3);
   UT_ASSERT_MEM_EQUAL (KeyBuffer, DefaultPublicKey, 3);
@@ -293,7 +308,12 @@ TestVerifyRsaPkcs1SignVerify (
   UT_ASSERT_NOT_EQUAL (SigSize, 0);
 
   Signature = AllocatePool (SigSize);
-  Status    = RsaPkcs1Sign (mRsa, HashValue, HashSize, Signature, &SigSize);
+  if (Signature == NULL) {
+    UT_LOG_ERROR ("Failed to allocate memory for Signature");
+    return UNIT_TEST_ERROR_TEST_FAILED;
+  }
+
+  Status = RsaPkcs1Sign (mRsa, HashValue, HashSize, Signature, &SigSize);
   UT_ASSERT_TRUE (Status);
   UT_ASSERT_EQUAL (SigSize, sizeof (RsaPkcs1Signature));
 

diff --git a/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/UnitTestMain.c b/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/UnitTestMain.c
@@ -26,13 +26,23 @@ UefiTestMain (
   UNIT_TEST_FRAMEWORK_HANDLE  Framework;
 
   DEBUG ((DEBUG_INFO, "%a v%a\n", UNIT_TEST_NAME, UNIT_TEST_VERSION));
-  CreateUnitTest (UNIT_TEST_NAME, UNIT_TEST_VERSION, &Framework);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  Status = CreateUnitTest (UNIT_TEST_NAME, UNIT_TEST_VERSION, &Framework);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "Failed in CreateUnitTestsfor BaseCryptLib Tests!  Status = %r\n", Status));
+    goto Done;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
 
   //
   // Execute the tests.
   //
   Status = RunAllTestSuites (Framework);
 
+  // MU_CHANGE [BEGIN] - CodeQL change
+Done:
+  // MU_CHANGE [END] - CodeQL change
   if (Framework) {
     FreeUnitTestFramework (Framework);
   }

diff --git a/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/X509Tests.c b/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/X509Tests.c
@@ -573,7 +573,14 @@ TestVerifyX509 (
   Status      = X509GetIssuerName (mTestCert, sizeof (mTestCert), NULL, &SubjectSize);
   UT_ASSERT_TRUE (!Status);
   Subject = AllocatePool (SubjectSize);
-  Status  = X509GetIssuerName (mTestCert, sizeof (mTestCert), Subject, &SubjectSize);
+  // MU_CHANGE [BEGIN] - CodeQL change
+  if (Subject == NULL) {
+    ASSERT (Subject != NULL);
+    return UNIT_TEST_ERROR_TEST_FAILED;
+  }
+
+  // MU_CHANGE [END] - CodeQL change
+  Status = X509GetIssuerName (mTestCert, sizeof (mTestCert), Subject, &SubjectSize);
   UT_ASSERT_TRUE (Status);
   FreePool (Subject);