Skip to content

Commit

Permalink
SecurityPkg: CodeQL Fixes.
Browse files Browse the repository at this point in the history 
Makes changes to comply with alerts raised by CodeQL.

Most of the issues here fall into the following two categories:

1. Potential use of uninitialized pointer.
2. Inconsistent integer width in comparison.

Co-authored-by: Taylor Beebe <31827475+TaylorBeebe@users.noreply.github.com>
Co-authored-by: kenlautner <85201046+kenlautner@users.noreply.github.com>
Co-authored-by: Bret Barkelew <bret@corthon.com>
  • Loading branch information
@makubacki @TaylorBeebe
@kenlautner @corthon @apop5
4 people authored and apop5 committed Aug 23, 2024
1 parent 0704fc9 commit 07d1bea
Show file tree
Hide file tree
Showing 25 changed files with 478 additions and 69 deletions.
2 changes: 1 addition & 1 deletion SecurityPkg/DeviceSecurity/SpdmLib/libspdm
Submodule libspdm updated 219 files
34 changes: 30 additions & 4 deletions SecurityPkg/FvReportPei/FvReportPei.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,13 @@ InstallPreHashFvPpi (
+ HashSize;

PreHashedFvPpi = AllocatePool (PpiSize);
ASSERT (PreHashedFvPpi != NULL);
// MU_CHANGE Start - CodeQL change - unguardednullreturndereference
if (PreHashedFvPpi == NULL) {
ASSERT (PreHashedFvPpi != NULL);
return;
}

// MU_CHANGE End - CodeQL change - unguardednullreturndereference

PreHashedFvPpi->FvBase = (UINT32)(UINTN)FvBuffer;
PreHashedFvPpi->FvLength = (UINT32)FvLength;
Expand All @@ -83,7 +89,14 @@ InstallPreHashFvPpi (
CopyMem (HASH_VALUE_PTR (HashInfo), HashValue, HashSize);

FvInfoPpiDescriptor = AllocatePool (sizeof (EFI_PEI_PPI_DESCRIPTOR));
ASSERT (FvInfoPpiDescriptor != NULL);
// MU_CHANGE Start - CodeQL change - unguardednullreturndereference
if (FvInfoPpiDescriptor == NULL) {
ASSERT (FvInfoPpiDescriptor != NULL);
FreePool (PreHashedFvPpi);
return;
}

// MU_CHANGE End - CodeQL change - unguardednullreturndereference

FvInfoPpiDescriptor->Guid = &gEdkiiPeiFirmwareVolumeInfoPrehashedFvPpiGuid;
FvInfoPpiDescriptor->Flags = EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST;
Expand Down Expand Up @@ -192,8 +205,14 @@ VerifyHashedFv (
// Copy FV to permanent memory to avoid potential TOC/TOU.
//
FvBuffer = AllocatePages (EFI_SIZE_TO_PAGES ((UINTN)FvInfo[FvIndex].Length));
// MU_CHANGE Start - CodeQL change - unguardednullreturndereference
if (FvBuffer == NULL) {
ASSERT (FvBuffer != NULL);
Status = EFI_OUT_OF_RESOURCES;
goto Done;
}

ASSERT (FvBuffer != NULL);
// MU_CHANGE End - CodeQL change - unguardednullreturndereference
Status = PeiServicesLocatePpi (
&gEdkiiPeiFirmwareVolumeShadowPpiGuid,
0,
Expand Down Expand Up @@ -375,12 +394,19 @@ CheckStoredHashFv (
);
if (!EFI_ERROR (Status) && (StoredHashFvPpi != NULL) && (StoredHashFvPpi->FvNumber > 0)) {
HashInfo = GetHashInfo (StoredHashFvPpi, BootMode);
Status = VerifyHashedFv (
// MU_CHANGE Start - CodeQL change - unguardednullreturndereference
if (HashInfo != NULL) {
Status = VerifyHashedFv (
HashInfo,
StoredHashFvPpi->FvInfo,
StoredHashFvPpi->FvNumber,
BootMode
);
} else {
Status = EFI_NOT_FOUND;
}

// MU_CHANGE End - CodeQL change - unguardednullreturndereference
if (!EFI_ERROR (Status)) {
DEBUG ((DEBUG_INFO, "OBB verification passed (%r)\r\n", Status));

Expand Down
37 changes: 32 additions & 5 deletions SecurityPkg/HddPassword/HddPasswordDxe.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,13 @@ BuildHddPasswordDeviceInfo (
S3InitDevicesExist = FALSE;
} else if (Status == EFI_BUFFER_TOO_SMALL) {
S3InitDevices = AllocatePool (S3InitDevicesLength);
ASSERT (S3InitDevices != NULL);
// MU_CHANGE Start - CodeQL change - unguardednullreturndereference
if (S3InitDevices == NULL) {
ASSERT (S3InitDevices != NULL);
return;
}

// MU_CHANGE End - CodeQL change - unguardednullreturndereference

Status = RestoreLockBox (
&gS3StorageDeviceInitListGuid,
Expand Down Expand Up @@ -184,7 +190,13 @@ BuildHddPasswordDeviceInfo (
FreePool (S3InitDevicesBak);
}

ASSERT (S3InitDevices != NULL);
// MU_CHANGE Start - CodeQL change - unguardednullreturndereference
if (S3InitDevices == NULL) {
ASSERT (S3InitDevices != NULL);
return;
}

// MU_CHANGE End - CodeQL change - unguardednullreturndereference

TempDevInfo = (HDD_PASSWORD_DEVICE_INFO *)((UINTN)TempDevInfo +
sizeof (HDD_PASSWORD_DEVICE_INFO) +
Expand Down Expand Up @@ -2195,8 +2207,16 @@ HddPasswordFormExtractConfig (
// followed by "&OFFSET=0&WIDTH=WWWWWWWWWWWWWWWW" followed by a Null-terminator
//
ConfigRequestHdr = HiiConstructConfigHdr (&mHddPasswordVendorGuid, mHddPasswordVendorStorageName, Private->DriverHandle);
Size = (StrLen (ConfigRequestHdr) + 32 + 1) * sizeof (CHAR16);
ConfigRequest = AllocateZeroPool (Size);
// MU_CHANGE Start - CodeQL change - unguardednullreturndereference
if (ConfigRequestHdr == NULL) {
ASSERT (ConfigRequestHdr != NULL);
FreePool (IfrData);
return EFI_OUT_OF_RESOURCES;
}

// MU_CHANGE End - CodeQL change - unguardednullreturndereference
Size = (StrLen (ConfigRequestHdr) + 32 + 1) * sizeof (CHAR16);
ConfigRequest = AllocateZeroPool (Size);
ASSERT (ConfigRequest != NULL);
AllocatedRequest = TRUE;
UnicodeSPrint (ConfigRequest, Size, L"%s&OFFSET=0&WIDTH=%016LX", ConfigRequestHdr, (UINT64)BufferSize);
Expand Down Expand Up @@ -2386,7 +2406,14 @@ HddPasswordFormCallback (
// In case goto the device configuration form, update the device form title.
//
ConfigFormEntry = HddPasswordGetConfigFormEntryByIndex ((UINT32)(QuestionId - KEY_HDD_DEVICE_ENTRY_BASE));
ASSERT (ConfigFormEntry != NULL);
// MU_CHANGE Start - CodeQL change - unguardednullreturndereference
if (ConfigFormEntry == NULL) {
ASSERT (ConfigFormEntry != NULL);
FreePool (IfrData);
return EFI_NOT_FOUND;
}

// MU_CHANGE End - CodeQL change - unguardednullreturndereference

DeviceFormTitleToken = (EFI_STRING_ID)STR_HDD_SECURITY_HD;
HiiSetString (Private->HiiHandle, DeviceFormTitleToken, ConfigFormEntry->HddString, NULL);
Expand Down
12 changes: 9 additions & 3 deletions SecurityPkg/Library/AuthVariableLib/AuthService.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,9 @@ CheckSignatureListFormat (
// Walk through the input signature list and check the data format.
// If any signature is incorrectly formed, the whole check will fail.
//
while ((SigDataSize > 0) && (SigDataSize >= SigList->SignatureListSize)) {
// MU_CHANGE Start - CodeQL change - comparison-with-wider-type
while ((SigDataSize > 0) && (SigDataSize >= (UINTN)SigList->SignatureListSize)) {
// MU_CHANGE End - CodeQL change - comparison-with-wider-type
for (Index = 0; Index < (sizeof (mSupportSigItem) / sizeof (EFI_SIGNATURE_ITEM)); Index++ ) {
if (CompareGuid (&SigList->SignatureType, &mSupportSigItem[Index].SigType)) {
//
Expand Down Expand Up @@ -1119,7 +1121,9 @@ FilterSignatureList (
Tail = TempData;

NewCertList = (EFI_SIGNATURE_LIST *)NewData;
while ((*NewDataSize > 0) && (*NewDataSize >= NewCertList->SignatureListSize)) {
// MU_CHANGE Start - CodeQL change - comparison-with-wider-type
while ((*NewDataSize > 0) && (*NewDataSize >= (UINTN)NewCertList->SignatureListSize)) {
// MU_CHANGE End - CodeQL change - comparison-with-wider-type
NewCert = (EFI_SIGNATURE_DATA *)((UINT8 *)NewCertList + sizeof (EFI_SIGNATURE_LIST) + NewCertList->SignatureHeaderSize);
NewCertCount = (NewCertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) - NewCertList->SignatureHeaderSize) / NewCertList->SignatureSize;

Expand All @@ -1129,7 +1133,9 @@ FilterSignatureList (

Size = DataSize;
CertList = (EFI_SIGNATURE_LIST *)Data;
while ((Size > 0) && (Size >= CertList->SignatureListSize)) {
// MU_CHANGE Start - CodeQL change - comparison-with-wider-type
while ((Size > 0) && (Size >= (UINTN)CertList->SignatureListSize)) {
// MU_CHANGE End - CodeQL change - comparison-with-wider-type
if (CompareGuid (&CertList->SignatureType, &NewCertList->SignatureType) &&
(CertList->SignatureSize == NewCertList->SignatureSize))
{
Expand Down
26 changes: 20 additions & 6 deletions SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -882,7 +882,9 @@ IsCertHashFoundInDbx (
return Status;
}

while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {
// MU_CHANGE Start - CodeQL change - comparison-with-wider-type
while ((DbxSize > 0) && (SignatureListSize >= (UINTN)DbxList->SignatureListSize)) {
// MU_CHANGE End - CodeQL change - comparison-with-wider-type
//
// Determine Hash Algorithm of Certificate in the forbidden database.
//
Expand Down Expand Up @@ -1027,7 +1029,9 @@ IsSignatureFoundInDatabase (
// Enumerate all signature data in SigDB to check if signature exists for executable.
//
CertList = (EFI_SIGNATURE_LIST *)Data;
while ((DataSize > 0) && (DataSize >= CertList->SignatureListSize)) {
// MU_CHANGE Start - CodeQL change - comparison-with-wider-type
while ((DataSize > 0) && (DataSize >= (UINTN)CertList->SignatureListSize)) {
// MU_CHANGE End - CodeQL change - comparison-with-wider-type
CertCount = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) - CertList->SignatureHeaderSize) / CertList->SignatureSize;
Cert = (EFI_SIGNATURE_DATA *)((UINT8 *)CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
if ((CertList->SignatureSize == sizeof (EFI_SIGNATURE_DATA) - 1 + SignatureSize) && (CompareGuid (&CertList->SignatureType, CertType))) {
Expand Down Expand Up @@ -1192,7 +1196,9 @@ PassTimestampCheck (
}

CertList = (EFI_SIGNATURE_LIST *)DbtData;
while ((DbtDataSize > 0) && (DbtDataSize >= CertList->SignatureListSize)) {
// MU_CHANGE Start - CodeQL change - comparison-with-wider-type
while ((DbtDataSize > 0) && (DbtDataSize >= (UINTN)CertList->SignatureListSize)) {
// MU_CHANGE End - CodeQL change - comparison-with-wider-type
if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) {
Cert = (EFI_SIGNATURE_DATA *)((UINT8 *)CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
CertCount = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) - CertList->SignatureHeaderSize) / CertList->SignatureSize;
Expand Down Expand Up @@ -1318,7 +1324,9 @@ IsForbiddenByDbx (
//
CertList = (EFI_SIGNATURE_LIST *)Data;
CertListSize = DataSize;
while ((CertListSize > 0) && (CertListSize >= CertList->SignatureListSize)) {
// MU_CHANGE Start - CodeQL change - comparison-with-wider-type
while ((CertListSize > 0) && (CertListSize >= (UINTN)CertList->SignatureListSize)) {
// MU_CHANGE End - CodeQL change - comparison-with-wider-type
if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) {
CertData = (EFI_SIGNATURE_DATA *)((UINT8 *)CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
CertCount = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) - CertList->SignatureHeaderSize) / CertList->SignatureSize;
Expand Down Expand Up @@ -1523,7 +1531,9 @@ IsAllowedByDb (
// Find X509 certificate in Signature List to verify the signature in pkcs7 signed data.
//
CertList = (EFI_SIGNATURE_LIST *)Data;
while ((DataSize > 0) && (DataSize >= CertList->SignatureListSize)) {
// MU_CHANGE Start - CodeQL change - comparison-with-wider-type
while ((DataSize > 0) && (DataSize >= (UINTN)CertList->SignatureListSize)) {
// MU_CHANGE End - CodeQL change - comparison-with-wider-type
if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) {
CertData = (EFI_SIGNATURE_DATA *)((UINT8 *)CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
CertCount = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) - CertList->SignatureHeaderSize) / CertList->SignatureSize;
Expand Down Expand Up @@ -2049,12 +2059,16 @@ DxeImageVerificationHandler (
// executable information table in either case.
//
NameStr = ConvertDevicePathToText (File, FALSE, TRUE);
AddImageExeInfo (Action, NameStr, File, SignatureList, SignatureListSize);

// MU_CHANGE Start - CodeQL change - unguardednullreturndereference
if (NameStr != NULL) {
AddImageExeInfo (Action, NameStr, File, SignatureList, SignatureListSize);
DEBUG ((DEBUG_INFO, "The image doesn't pass verification: %s\n", NameStr));
FreePool (NameStr);
}

// MU_CHANGE End - CodeQL change - unguardednullreturndereference

if (SignatureList != NULL) {
FreePool (SignatureList);
}
Expand Down
Loading

0 comments on commit 07d1bea

Please sign in to comment.