From 55e2dedc92d7dd42770ea62e42d1438bd8201604 Mon Sep 17 00:00:00 2001 From: Michael Kubacki Date: Thu, 15 Sep 2022 01:41:24 +0000 Subject: [PATCH] CryptoPkg: CodeQL Fixes Resolves CodeQL issues in the package. Co-authored-by: Aaron Pop Co-authored-by: Kun Qin Co-authored-by: Taylor Beebe Co-authored-by: Doug Flick Co-authored-by: Ken Lautner Signed-off-by: Michael Kubacki --- .../BaseCryptLib/Pk/CryptAuthenticode.c | 6 ++ CryptoPkg/Library/BaseCryptLib/Pk/CryptDh.c | 12 ++- CryptoPkg/Library/BaseCryptLib/Pk/CryptEc.c | 17 ++++- CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c | 6 ++ CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c | 26 ++++++- .../Library/BaseCryptLib/SysCall/CrtWrapper.c | 5 +- .../Library/BaseCryptLib/Pkcs5Pbkdf2Tests.c | 4 + .../Library/BaseCryptLib/RsaPkcs7Tests.c | 73 +++++++++++++------ .../Library/BaseCryptLib/RsaPssTests.c | 7 +- .../UnitTest/Library/BaseCryptLib/RsaTests.c | 28 ++++++- .../Library/BaseCryptLib/UnitTestMain.c | 12 ++- .../UnitTest/Library/BaseCryptLib/X509Tests.c | 9 ++- 12 files changed, 170 insertions(+), 35 deletions(-) diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c index 6b0dddd4af5..ee940eb5e0a 100644 --- 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 index 27322888934..6224aa1842a 100644 --- 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 index d8cc9ba0e8f..fc95075dff9 100644 --- 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 index 944bcf8d38f..be389fa097d 100644 --- 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 index 1182323b63e..7f536249c80 100644 --- 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 index 37cdecc9bd1..491721e9aa1 100644 --- 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 index 376188f9a3b..d090ddc6039 100644 --- 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 index fc7960a2cf2..2705cf36cfd 100644 --- 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 index 42baf88524c..48c1b7a4508 100644 --- 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 index c61f2da73df..f80a9b07df8 100644 --- 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 index d0c1c7a4f7e..a0ddf07b593 100644 --- 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 index b3d11ea3303..376298a093b 100644 --- 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);