From e16b9ec8ee165efeb4a00e975f9c50751d958e27 Mon Sep 17 00:00:00 2001 From: Doug Flick Date: Thu, 23 Feb 2023 13:56:49 -0500 Subject: [PATCH] CryptoPkg: Additional CodeQL fixes (#279) # Preface Please ensure you have read the [contribution docs](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md) prior to submitting the pull request. In particular, [pull request guidelines](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md#pull-request-best-practices). ## Description <_Please include a description of the change and why this change was made._> For each item, place an "x" in between `[` and `]` if true. Example: `[x]`. _(you can also check items in the GitHub UI)_ - [ ] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested <_Please describe the test(s) that were run to verify the changes._> ## Integration Instructions <_Describe how these changes should be integrated. Use N/A if nothing is required._> --------- Co-authored-by: Michael Kubacki --- .../Library/BaseCryptLib/Pkcs5Pbkdf2Tests.c | 4 + .../Library/BaseCryptLib/RsaPkcs7Tests.c | 73 +++++++++++++------ .../Library/BaseCryptLib/RsaPssTests.c | 7 +- .../UnitTest/Library/BaseCryptLib/RsaTests.c | 28 ++++++- 4 files changed, 86 insertions(+), 26 deletions(-) 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 3f06e89b3cc..3e0ae23098d 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)); @@ -204,7 +214,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); @@ -283,7 +298,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));