REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2215
There is an integer overflow vulnerability in DxeImageVerificationHandler function when parsing the PE files attribute certificate table. In cases where WinCertificate->dwLength is sufficiently large, it's possible to overflow Offset back to 0 causing an endless loop. Check offset inbetween VirtualAddress and VirtualAddress + Size. Using SafeintLib to do offset addition with result check. Cc: Jiewen Yao <jiewen....@intel.com> Cc: Jian J Wang <jian.j.w...@intel.com> Cc: Laszlo Ersek <ler...@redhat.com> Signed-off-by: Wenyi Xie <xiewen...@huawei.com> --- SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf | 1 + SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h | 1 + SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 111 +++++++++++--------- 3 files changed, 63 insertions(+), 50 deletions(-) diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf index 1e1a639857e0..a7ac4830b3d4 100644 --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf @@ -53,6 +53,7 @@ [LibraryClasses] SecurityManagementLib PeCoffLib TpmMeasurementLib + SafeIntLib [Protocols] gEfiFirmwareVolume2ProtocolGuid ## SOMETIMES_CONSUMES diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h index 17955ff9774c..060273917d5d 100644 --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include <Library/DevicePathLib.h> #include <Library/SecurityManagementLib.h> #include <Library/PeCoffLib.h> +#include <Library/SafeIntLib.h> #include <Protocol/FirmwareVolume2.h> #include <Protocol/DevicePath.h> #include <Protocol/BlockIo.h> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c index 36b87e16d53d..dbc03e28c05b 100644 --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c @@ -1658,6 +1658,10 @@ DxeImageVerificationHandler ( EFI_STATUS HashStatus; EFI_STATUS DbStatus; BOOLEAN IsFound; + UINT32 AlignedLength; + UINT32 Result; + EFI_STATUS AddStatus; + BOOLEAN IsAuthDataAssigned; SignatureList = NULL; SignatureListSize = 0; @@ -1667,6 +1671,7 @@ DxeImageVerificationHandler ( Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED; IsVerified = FALSE; IsFound = FALSE; + Result = 0; // // Check the image type and get policy setting. @@ -1850,9 +1855,10 @@ DxeImageVerificationHandler ( // The first certificate starts at offset (SecDataDir->VirtualAddress) from the start of the file. // for (OffSet = SecDataDir->VirtualAddress; - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size); - OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) { + (OffSet >= SecDataDir->VirtualAddress) && (OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size));) { + IsAuthDataAssigned = FALSE; WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); + AlignedLength = WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength); if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof (WIN_CERTIFICATE) || (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertificate->dwLength) { break; @@ -1872,6 +1878,8 @@ DxeImageVerificationHandler ( } AuthData = PkcsCertData->CertData; AuthDataSize = PkcsCertData->Hdr.dwLength - sizeof(PkcsCertData->Hdr); + IsAuthDataAssigned = TRUE; + HashStatus = HashPeImageByType (AuthData, AuthDataSize); } else if (WinCertificate->wCertificateType == WIN_CERT_TYPE_EFI_GUID) { // // The certificate is formatted as WIN_CERTIFICATE_UEFI_GUID which is described in UEFI Spec. @@ -1880,72 +1888,75 @@ DxeImageVerificationHandler ( if (WinCertUefiGuid->Hdr.dwLength <= OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData)) { break; } - if (!CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Guid)) { - continue; + if (CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Guid)) { + AuthData = WinCertUefiGuid->CertData; + AuthDataSize = WinCertUefiGuid->Hdr.dwLength - OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData); + IsAuthDataAssigned = TRUE; + HashStatus = HashPeImageByType (AuthData, AuthDataSize); } - AuthData = WinCertUefiGuid->CertData; - AuthDataSize = WinCertUefiGuid->Hdr.dwLength - OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData); } else { if (WinCertificate->dwLength < sizeof (WIN_CERTIFICATE)) { break; } - continue; } - HashStatus = HashPeImageByType (AuthData, AuthDataSize); - if (EFI_ERROR (HashStatus)) { - continue; - } - - // - // Check the digital signature against the revoked certificate in forbidden database (dbx). - // - if (IsForbiddenByDbx (AuthData, AuthDataSize)) { - Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED; - IsVerified = FALSE; - break; - } - - // - // Check the digital signature against the valid certificate in allowed database (db). - // - if (!IsVerified) { - if (IsAllowedByDb (AuthData, AuthDataSize)) { - IsVerified = TRUE; + if (IsAuthDataAssigned && !EFI_ERROR (HashStatus)) { + // + // Check the digital signature against the revoked certificate in forbidden database (dbx). + // + if (IsForbiddenByDbx (AuthData, AuthDataSize)) { + Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED; + IsVerified = FALSE; + break; } - } - // - // Check the image's hash value. - // - DbStatus = IsSignatureFoundInDatabase ( - EFI_IMAGE_SECURITY_DATABASE1, - mImageDigest, - &mCertType, - mImageDigestSize, - &IsFound - ); - if (EFI_ERROR (DbStatus) || IsFound) { - Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND; - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but %s hash of image is found in DBX.\n", mHashTypeStr)); - IsVerified = FALSE; - break; - } + // + // Check the digital signature against the valid certificate in allowed database (db). + // + if (!IsVerified) { + if (IsAllowedByDb (AuthData, AuthDataSize)) { + IsVerified = TRUE; + } + } - if (!IsVerified) { + // + // Check the image's hash value. + // DbStatus = IsSignatureFoundInDatabase ( - EFI_IMAGE_SECURITY_DATABASE, + EFI_IMAGE_SECURITY_DATABASE1, mImageDigest, &mCertType, mImageDigestSize, &IsFound ); - if (!EFI_ERROR (DbStatus) && IsFound) { - IsVerified = TRUE; - } else { - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but signature is not allowed by DB and %s hash of image is not found in DB/DBX.\n", mHashTypeStr)); + if (EFI_ERROR (DbStatus) || IsFound) { + Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND; + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but %s hash of image is found in DBX.\n", mHashTypeStr)); + IsVerified = FALSE; + break; } + + if (!IsVerified) { + DbStatus = IsSignatureFoundInDatabase ( + EFI_IMAGE_SECURITY_DATABASE, + mImageDigest, + &mCertType, + mImageDigestSize, + &IsFound + ); + if (!EFI_ERROR (DbStatus) && IsFound) { + IsVerified = TRUE; + } else { + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but signature is not allowed by DB and %s hash of image is not found in DB/DBX.\n", mHashTypeStr)); + } + } + } + + AddStatus = SafeUint32Add (OffSet, AlignedLength, &Result); + if (EFI_ERROR (AddStatus)) { + break; } + OffSet = Result; } if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) { -- 2.20.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64161): https://edk2.groups.io/g/devel/message/64161 Mute This Topic: https://groups.io/mt/76165658/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-