Thanks Wenyi. May I know how you test the new code logic?
Any unit test you can share, such as a mal-formed PE image, which may break the old implementation but is caught by this patch? Thank you Yao Jiewen > -----Original Message----- > From: Wenyi Xie <xiewen...@huawei.com> > Sent: Thursday, August 13, 2020 7:56 PM > To: devel@edk2.groups.io; Yao, Jiewen <jiewen....@intel.com>; Wang, Jian J > <jian.j.w...@intel.com>; ler...@redhat.com > Cc: huangmin...@huawei.com; songdongku...@huawei.com > Subject: [PATCH EDK2 v2 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced > verification of Offset > > 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 (#64221): https://edk2.groups.io/g/devel/message/64221 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] -=-=-=-=-=-=-=-=-=-=-=-
