* [PATCH EDK2 v1 0/1] Enhanced verification of Offset(CVE-2019-14562)
@ 2020-08-12 7:04 wenyi,xie
2020-08-12 7:04 ` [PATCH EDK2 v1 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced " wenyi,xie
0 siblings, 1 reply; 4+ messages in thread
From: wenyi,xie @ 2020-08-12 7:04 UTC (permalink / raw)
To: devel, jiewen.yao, jian.j.wang, chao.b.zhang; +Cc: huangming23, songdongkuang
Main Changes:
1.check offset inbetween VirtualAddress and VirtualAddress + Size.
2.Using SafeintLib to do offset addition with result check.
Code can also be found in github:
https://github.com/leadsama/edk2.git
branch: bug-2215-v1
Wenyi Xie (1):
SecurityPkg/DxeImageVerificationLib:Enhanced verification of
Offset(CVE-2019-14562)
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(-)
--
2.20.1.windows.1
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH EDK2 v1 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset(CVE-2019-14562) 2020-08-12 7:04 [PATCH EDK2 v1 0/1] Enhanced verification of Offset(CVE-2019-14562) wenyi,xie @ 2020-08-12 7:04 ` wenyi,xie 2020-08-12 12:23 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: wenyi,xie @ 2020-08-12 7:04 UTC (permalink / raw) To: devel, jiewen.yao, jian.j.wang, chao.b.zhang; +Cc: huangming23, songdongkuang 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.yao@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Chao Zhang <chao.b.zhang@intel.com> Signed-off-by: Wenyi Xie <xiewenyi2@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)) { -- 2.20.1.windows.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH EDK2 v1 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset(CVE-2019-14562) 2020-08-12 7:04 ` [PATCH EDK2 v1 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced " wenyi,xie @ 2020-08-12 12:23 ` Laszlo Ersek 2020-08-13 12:43 ` wenyi,xie 0 siblings, 1 reply; 4+ messages in thread From: Laszlo Ersek @ 2020-08-12 12:23 UTC (permalink / raw) To: devel, xiewenyi2, jiewen.yao, jian.j.wang, chao.b.zhang Cc: huangming23, songdongkuang 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.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Chao Zhang <chao.b.zhang@intel.com> > Signed-off-by: Wenyi Xie <xiewenyi2@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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH EDK2 v1 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset(CVE-2019-14562) 2020-08-12 12:23 ` [edk2-devel] " Laszlo Ersek @ 2020-08-13 12:43 ` wenyi,xie 0 siblings, 0 replies; 4+ messages in thread From: wenyi,xie @ 2020-08-13 12:43 UTC (permalink / raw) To: Laszlo Ersek, devel, jiewen.yao, jian.j.wang; +Cc: huangming23, songdongkuang On 2020/8/12 20:23, Laszlo Ersek wrote: > 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.yao@intel.com> >> Cc: Jian J Wang <jian.j.wang@intel.com> >> Cc: Chao Zhang <chao.b.zhang@intel.com> >> Signed-off-by: Wenyi Xie <xiewenyi2@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. I saw you have marked URL on the bugzilla ticket, so I didn't mark it again. > > (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. Ok, I removed "goto" and add a new flag IsAuthDataAssigned. When AuthData and AuthDataSize are assigned, the flag is set true. If either this flag or status of HashPeImageByType is false, then skip signature verification, only increasing offset and step to next loop. And thank you for your review. > > Thanks > Laszlo > > > . > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-13 12:43 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-08-12 7:04 [PATCH EDK2 v1 0/1] Enhanced verification of Offset(CVE-2019-14562) wenyi,xie 2020-08-12 7:04 ` [PATCH EDK2 v1 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced " wenyi,xie 2020-08-12 12:23 ` [edk2-devel] " Laszlo Ersek 2020-08-13 12:43 ` wenyi,xie
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox