On 08/12/20 09:04, wenyi,xie via groups.io wrote: > 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: Chao Zhang <chao.b.zh...@intel.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 | 21 > +++++++++++++++----- > 3 files changed, 18 insertions(+), 5 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..2b42d4595f2c 100644 > --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > @@ -1658,6 +1658,9 @@ DxeImageVerificationHandler ( > EFI_STATUS HashStatus; > EFI_STATUS DbStatus; > BOOLEAN IsFound; > + UINT32 AlignedLength; > + UINT32 Result; > + EFI_STATUS AddStatus; > > SignatureList = NULL; > SignatureListSize = 0; > @@ -1667,6 +1670,7 @@ DxeImageVerificationHandler ( > Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED; > IsVerified = FALSE; > IsFound = FALSE; > + Result = 0; > > // > // Check the image type and get policy setting. > @@ -1850,9 +1854,9 @@ 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));) { > 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; > @@ -1881,7 +1885,7 @@ DxeImageVerificationHandler ( > break; > } > if (!CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Guid)) { > - continue; > + goto NEXT_LOOP; > } > AuthData = WinCertUefiGuid->CertData; > AuthDataSize = WinCertUefiGuid->Hdr.dwLength - > OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData); > @@ -1889,12 +1893,12 @@ DxeImageVerificationHandler ( > if (WinCertificate->dwLength < sizeof (WIN_CERTIFICATE)) { > break; > } > - continue; > + goto NEXT_LOOP; > } > > HashStatus = HashPeImageByType (AuthData, AuthDataSize); > if (EFI_ERROR (HashStatus)) { > - continue; > + goto NEXT_LOOP; > } > > // > @@ -1946,6 +1950,13 @@ DxeImageVerificationHandler ( > 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)); > } > } > + > +NEXT_LOOP: > + AddStatus = SafeUint32Add (OffSet, AlignedLength, &Result); > + if (EFI_ERROR (AddStatus)) { > + break; > + } > + OffSet = Result; > } > > if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) { >
(1) Please mark the patch URL <https://edk2.groups.io/g/devel/message/64059> on the bugzilla ticket, in a comment. (2) We should not use "goto" statements for such control transfers, only for error handling. If necessary, please split the function in two, or reorganize the loop otherwise. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#64068): https://edk2.groups.io/g/devel/message/64068 Mute This Topic: https://groups.io/mt/76143922/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
