* [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)
@ 2020-09-01 9:12 Laszlo Ersek
2020-09-01 9:12 ` [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft Laszlo Ersek
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Laszlo Ersek @ 2020-09-01 9:12 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Jian J Wang, Jiewen Yao, Min Xu, Wenyi Xie
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
Repo: https://pagure.io/lersek/edk2.git
Branch: tianocore_2215
I'm neutral on whether this becomes part of edk2-stable202008.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Wenyi Xie <xiewenyi2@huawei.com>
Thanks,
Laszlo
Laszlo Ersek (3):
SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd,
SecDataDirLeft
SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size
check
SecurityPkg/DxeImageVerificationLib: catch alignment overflow
(CVE-2019-14562)
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
--
2.19.1.3.g30247aa5d201
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft
2020-09-01 9:12 [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562) Laszlo Ersek
@ 2020-09-01 9:12 ` Laszlo Ersek
2020-09-01 15:37 ` [edk2-devel] " Philippe Mathieu-Daudé
` (2 more replies)
2020-09-01 9:12 ` [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check Laszlo Ersek
` (2 subsequent siblings)
3 siblings, 3 replies; 20+ messages in thread
From: Laszlo Ersek @ 2020-09-01 9:12 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Jian J Wang, Jiewen Yao, Min Xu, Wenyi Xie
The following two quantities:
SecDataDir->VirtualAddress + SecDataDir->Size
SecDataDir->VirtualAddress + SecDataDir->Size - OffSet
are used multiple times in DxeImageVerificationHandler(). Introduce helper
variables for them: "SecDataDirEnd" and "SecDataDirLeft", respectively.
This saves us multiple calculations and significantly simplifies the code.
Note that all three summands above have type UINT32, therefore the new
variables are also of type UINT32.
This patch does not change behavior.
(Note that the code already handles the case when the
SecDataDir->VirtualAddress + SecDataDir->Size
UINT32 addition overflows -- namely, in that case, the certificate loop is
never entered, and the corruption check right after the loop fires.)
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Wenyi Xie <xiewenyi2@huawei.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index b08fe24e85aa..377feebb205a 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1652,6 +1652,8 @@ DxeImageVerificationHandler (
UINT8 *AuthData;
UINTN AuthDataSize;
EFI_IMAGE_DATA_DIRECTORY *SecDataDir;
+ UINT32 SecDataDirEnd;
+ UINT32 SecDataDirLeft;
UINT32 OffSet;
CHAR16 *NameStr;
RETURN_STATUS PeCoffStatus;
@@ -1849,12 +1851,14 @@ DxeImageVerificationHandler (
// "Attribute Certificate Table".
// The first certificate starts at offset (SecDataDir->VirtualAddress) from the start of the file.
//
+ SecDataDirEnd = SecDataDir->VirtualAddress + SecDataDir->Size;
for (OffSet = SecDataDir->VirtualAddress;
- OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
+ OffSet < SecDataDirEnd;
OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) {
WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
- if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof (WIN_CERTIFICATE) ||
- (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertificate->dwLength) {
+ SecDataDirLeft = SecDataDirEnd - OffSet;
+ if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
+ SecDataDirLeft < WinCertificate->dwLength) {
break;
}
@@ -1948,7 +1952,7 @@ DxeImageVerificationHandler (
}
}
- if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) {
+ if (OffSet != SecDataDirEnd) {
//
// The Size in Certificate Table or the attribute certificate table is corrupted.
//
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check
2020-09-01 9:12 [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562) Laszlo Ersek
2020-09-01 9:12 ` [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft Laszlo Ersek
@ 2020-09-01 9:12 ` Laszlo Ersek
2020-09-01 15:40 ` [edk2-devel] " Philippe Mathieu-Daudé
` (2 more replies)
2020-09-01 9:12 ` [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562) Laszlo Ersek
2020-09-02 4:02 ` [edk2-devel] [PATCH 0/3] " Yao, Jiewen
3 siblings, 3 replies; 20+ messages in thread
From: Laszlo Ersek @ 2020-09-01 9:12 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Jian J Wang, Jiewen Yao, Min Xu, Wenyi Xie
Currently the (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) check only
guards the de-referencing of the "WinCertificate" pointer. It does not
guard the calculation of the pointer itself:
WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
This is wrong; if we don't know for sure that we have enough room for a
WIN_CERTIFICATE, then even creating such a pointer, not just
de-referencing it, may invoke undefined behavior.
Move the pointer calculation after the size check.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Wenyi Xie <xiewenyi2@huawei.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 377feebb205a..100739eb3eb6 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1855,10 +1855,12 @@ DxeImageVerificationHandler (
for (OffSet = SecDataDir->VirtualAddress;
OffSet < SecDataDirEnd;
OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) {
- WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
SecDataDirLeft = SecDataDirEnd - OffSet;
- if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
- SecDataDirLeft < WinCertificate->dwLength) {
+ if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) {
+ break;
+ }
+ WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
+ if (SecDataDirLeft < WinCertificate->dwLength) {
break;
}
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)
2020-09-01 9:12 [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562) Laszlo Ersek
2020-09-01 9:12 ` [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft Laszlo Ersek
2020-09-01 9:12 ` [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check Laszlo Ersek
@ 2020-09-01 9:12 ` Laszlo Ersek
2020-09-01 15:53 ` [edk2-devel] " Philippe Mathieu-Daudé
` (2 more replies)
2020-09-02 4:02 ` [edk2-devel] [PATCH 0/3] " Yao, Jiewen
3 siblings, 3 replies; 20+ messages in thread
From: Laszlo Ersek @ 2020-09-01 9:12 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Jian J Wang, Jiewen Yao, Min Xu, Wenyi Xie
The DxeImageVerificationHandler() function currently checks whether
"SecDataDir" has enough room for "WinCertificate->dwLength". However, for
advancing "OffSet", "WinCertificate->dwLength" is aligned to the next
multiple of 8. If "WinCertificate->dwLength" is large enough, the
alignment will return 0, and "OffSet" will be stuck at the same value.
Check whether "SecDataDir" has room left for both
"WinCertificate->dwLength" and the alignment.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Wenyi Xie <xiewenyi2@huawei.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 100739eb3eb6..11154b6cc58a 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1860,7 +1860,9 @@ DxeImageVerificationHandler (
break;
}
WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
- if (SecDataDirLeft < WinCertificate->dwLength) {
+ if (SecDataDirLeft < WinCertificate->dwLength ||
+ (SecDataDirLeft - WinCertificate->dwLength <
+ ALIGN_SIZE (WinCertificate->dwLength))) {
break;
}
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft
2020-09-01 9:12 ` [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft Laszlo Ersek
@ 2020-09-01 15:37 ` Philippe Mathieu-Daudé
2020-09-02 1:31 ` wenyi,xie
2020-09-02 1:55 ` Xu, Min M
2 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-01 15:37 UTC (permalink / raw)
To: devel, lersek; +Cc: Jian J Wang, Jiewen Yao, Min Xu, Wenyi Xie
On 9/1/20 11:12 AM, Laszlo Ersek wrote:
> The following two quantities:
>
> SecDataDir->VirtualAddress + SecDataDir->Size
> SecDataDir->VirtualAddress + SecDataDir->Size - OffSet
>
> are used multiple times in DxeImageVerificationHandler(). Introduce helper
> variables for them: "SecDataDirEnd" and "SecDataDirLeft", respectively.
> This saves us multiple calculations and significantly simplifies the code.
>
> Note that all three summands above have type UINT32, therefore the new
> variables are also of type UINT32.
>
> This patch does not change behavior.
>
> (Note that the code already handles the case when the
>
> SecDataDir->VirtualAddress + SecDataDir->Size
>
> UINT32 addition overflows -- namely, in that case, the certificate loop is
> never entered, and the corruption check right after the loop fires.)
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Wenyi Xie <xiewenyi2@huawei.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> ---
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index b08fe24e85aa..377feebb205a 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1652,6 +1652,8 @@ DxeImageVerificationHandler (
> UINT8 *AuthData;
> UINTN AuthDataSize;
> EFI_IMAGE_DATA_DIRECTORY *SecDataDir;
> + UINT32 SecDataDirEnd;
> + UINT32 SecDataDirLeft;
> UINT32 OffSet;
> CHAR16 *NameStr;
> RETURN_STATUS PeCoffStatus;
> @@ -1849,12 +1851,14 @@ DxeImageVerificationHandler (
> // "Attribute Certificate Table".
> // The first certificate starts at offset (SecDataDir->VirtualAddress) from the start of the file.
> //
> + SecDataDirEnd = SecDataDir->VirtualAddress + SecDataDir->Size;
> for (OffSet = SecDataDir->VirtualAddress;
> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
> + OffSet < SecDataDirEnd;
> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) {
> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> - if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof (WIN_CERTIFICATE) ||
> - (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertificate->dwLength) {
> + SecDataDirLeft = SecDataDirEnd - OffSet;
> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
> + SecDataDirLeft < WinCertificate->dwLength) {
> break;
> }
>
> @@ -1948,7 +1952,7 @@ DxeImageVerificationHandler (
> }
> }
>
> - if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) {
> + if (OffSet != SecDataDirEnd) {
> //
> // The Size in Certificate Table or the attribute certificate table is corrupted.
> //
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check
2020-09-01 9:12 ` [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check Laszlo Ersek
@ 2020-09-01 15:40 ` Philippe Mathieu-Daudé
2020-09-02 1:32 ` wenyi,xie
2020-09-02 2:19 ` Xu, Min M
2 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-01 15:40 UTC (permalink / raw)
To: devel, lersek; +Cc: Jian J Wang, Jiewen Yao, Min Xu, Wenyi Xie
On 9/1/20 11:12 AM, Laszlo Ersek wrote:
> Currently the (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) check only
> guards the de-referencing of the "WinCertificate" pointer. It does not
> guard the calculation of the pointer itself:
>
> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
>
> This is wrong; if we don't know for sure that we have enough room for a
> WIN_CERTIFICATE, then even creating such a pointer, not just
> de-referencing it, may invoke undefined behavior.
Tricky to catch...
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>
> Move the pointer calculation after the size check.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Wenyi Xie <xiewenyi2@huawei.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 377feebb205a..100739eb3eb6 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1855,10 +1855,12 @@ DxeImageVerificationHandler (
> for (OffSet = SecDataDir->VirtualAddress;
> OffSet < SecDataDirEnd;
> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) {
> - WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> SecDataDirLeft = SecDataDirEnd - OffSet;
> - if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
> - SecDataDirLeft < WinCertificate->dwLength) {
> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) {
> + break;
> + }
> + WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> + if (SecDataDirLeft < WinCertificate->dwLength) {
> break;
> }
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)
2020-09-01 9:12 ` [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562) Laszlo Ersek
@ 2020-09-01 15:53 ` Philippe Mathieu-Daudé
2020-09-01 16:52 ` Laszlo Ersek
2020-09-02 1:32 ` wenyi,xie
2020-09-02 2:34 ` Xu, Min M
2 siblings, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-01 15:53 UTC (permalink / raw)
To: devel, lersek; +Cc: Jian J Wang, Jiewen Yao, Min Xu, Wenyi Xie
Hi Laszlo,
On 9/1/20 11:12 AM, Laszlo Ersek wrote:
> The DxeImageVerificationHandler() function currently checks whether
> "SecDataDir" has enough room for "WinCertificate->dwLength". However, for
> advancing "OffSet", "WinCertificate->dwLength" is aligned to the next
> multiple of 8. If "WinCertificate->dwLength" is large enough, the
> alignment will return 0, and "OffSet" will be stuck at the same value.
>
> Check whether "SecDataDir" has room left for both
> "WinCertificate->dwLength" and the alignment.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Wenyi Xie <xiewenyi2@huawei.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 100739eb3eb6..11154b6cc58a 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1860,7 +1860,9 @@ DxeImageVerificationHandler (
> break;
> }
> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> - if (SecDataDirLeft < WinCertificate->dwLength) {
> + if (SecDataDirLeft < WinCertificate->dwLength ||
> + (SecDataDirLeft - WinCertificate->dwLength <
> + ALIGN_SIZE (WinCertificate->dwLength))) {
I dare to ask (probably again, I remember some similar boundary check
style question once), why not as (which is simpler for me to review):
if (SecDataDirLeft < WinCertificate->dwLength + ALIGN_SIZE
(WinCertificate->dwLength)) {
At any rate, for this patch:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> break;
> }
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)
2020-09-01 15:53 ` [edk2-devel] " Philippe Mathieu-Daudé
@ 2020-09-01 16:52 ` Laszlo Ersek
0 siblings, 0 replies; 20+ messages in thread
From: Laszlo Ersek @ 2020-09-01 16:52 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, devel
Cc: Jian J Wang, Jiewen Yao, Min Xu, Wenyi Xie
On 09/01/20 17:53, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
>
> On 9/1/20 11:12 AM, Laszlo Ersek wrote:
>> The DxeImageVerificationHandler() function currently checks whether
>> "SecDataDir" has enough room for "WinCertificate->dwLength". However, for
>> advancing "OffSet", "WinCertificate->dwLength" is aligned to the next
>> multiple of 8. If "WinCertificate->dwLength" is large enough, the
>> alignment will return 0, and "OffSet" will be stuck at the same value.
>>
>> Check whether "SecDataDir" has room left for both
>> "WinCertificate->dwLength" and the alignment.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Wenyi Xie <xiewenyi2@huawei.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> index 100739eb3eb6..11154b6cc58a 100644
>> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> @@ -1860,7 +1860,9 @@ DxeImageVerificationHandler (
>> break;
>> }
>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
>> - if (SecDataDirLeft < WinCertificate->dwLength) {
>> + if (SecDataDirLeft < WinCertificate->dwLength ||
>> + (SecDataDirLeft - WinCertificate->dwLength <
>> + ALIGN_SIZE (WinCertificate->dwLength))) {
>
> I dare to ask (probably again, I remember some similar boundary check
> style question once), why not as (which is simpler for me to review):
>
> if (SecDataDirLeft < WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength)) {
Indeed, the mathematical relation that we want to catch (and reject) is:
SecDataDirLeft < WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength)
But the problem is precisely with the addition on the right hand side.
(1) The "WinCertificate->dwLength" field is of type UINT32 (meaning, in
standard C terms, "unsigned int").
(2) The ALIGN_SIZE() macro is defined as follows, in
"SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h":
#define ALIGNMENT_SIZE 8
#define ALIGN_SIZE(a) (((a) % ALIGNMENT_SIZE) ? ALIGNMENT_SIZE - ((a) % ALIGNMENT_SIZE) : 0)
If you substitute "WinCertificate->dwLength" for "a", and 8 for
ALIGNMENT_SIZE, you get the following replacement text for the macro
invocation:
(((WinCertificate->dwLength) % 8) ? 8 - ((WinCertificate->dwLength) % 8) : 0)
Here the third operand of the conditional operator (that is, 0) is
of type "int".
The 2nd operand of the conditional operator ultimately has type
"unsigned int". That's because the type of
"WinCertificate->dwLength" conceptually "propagates" outwards across
the "%" and "-" operators, via the "usual arithmetic conversions".
Basically you have
(unsigned int) % (int) -->
converts the int to unsigned int, produces unsigned int
(int) - (unsigned int) -->
converts the int to unsigned int, produces unsigned int
And then we have the following, for the conditional operator too
(C99 6.5.15p5):
If both the second and third operands have arithmetic type, the
result type that would be determined by the usual arithmetic
conversions, were they applied to those two operands, is the type
of the result.
Which means that ALIGN_SIZE (WinCertificate->dwLength) produces a
UINT32 (unsigned int) as well.
(3) Furthermore, "SecDataDirLeft" is of type UINT32 (unsigned int) too.
(4) Ultimately, in the 2nd subcondition of the "if", we would have
UINT32 < UINT32 + UINT32
If the addition overflows on the right hand size (which is well
defined behavior for "unsigned int"s), possibly to a really small
number, then the comparison may evaluate to FALSE, even though the
*mathematical* result would be TRUE.
Specifically, if
WinCertificate->dwLength >= MAX_UINT32 - 6
then the sum
WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength)
wraps around to zero precisely, and then the 2nd subcondition would
decay to
SecDataDirLeft < 0
which would always evaluate to FALSE.
We can avoid this by taking the mathematically relevant expression
SecDataDirLeft < WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength)
and by subtracting "WinCertificate->dwLength" from both sides. That
yields the following (mathematically equivalent) inequality:
SecDataDirLeft - WinCertificate->dwLength < ALIGN_SIZE (WinCertificate->dwLength)
which we can safely evaluate in C.
The reason for that "safety" is the *directly preceding* subcondition in
the "if" statement:
if (SecDataDirLeft < WinCertificate->dwLength || ...
Namely, if this (1st) condition evaluates to TRUE, then we ignore the second
subcondition (due to the logical OR), and take the "break" at once. And
if the 1st condition evaluates to FALSE (so that we need to consider the
new, 2nd, subcondition), *then* we know:
SecDataDirLeft >= WinCertificate->dwLength
and then just subtract "WinCertificate->dwLength" from both sides, to
realize the following mathematical assurance:
SecDataDirLeft - WinCertificate->dwLength >= 0
Thus, the difference that is visible on the left-hand side above is
safe to *express* in the second subcondition that we actually care
about:
SecDataDirLeft - WinCertificate->dwLength < ALIGN_SIZE (WinCertificate->dwLength)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
|
this UINT32 subtraction will not
underflow because the first subcondition
evaluated to FALSE
In brief,
- we re-formulate the 2nd subcondition, using a subtraction in place of
the addition, for preventing an overflow in the addition,
- and the subtraction is safe due to the 1st subcondition, which we
check directly before.
So it's not just a style question, but a safety/security one.
> At any rate, for this patch:
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
Thanks!
Laszlo
>
>> break;
>> }
>>
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft
2020-09-01 9:12 ` [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft Laszlo Ersek
2020-09-01 15:37 ` [edk2-devel] " Philippe Mathieu-Daudé
@ 2020-09-02 1:31 ` wenyi,xie
2020-09-02 1:55 ` Xu, Min M
2 siblings, 0 replies; 20+ messages in thread
From: wenyi,xie @ 2020-09-02 1:31 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Jian J Wang, Jiewen Yao, Min Xu
On 2020/9/1 17:12, Laszlo Ersek wrote:
> The following two quantities:
>
> SecDataDir->VirtualAddress + SecDataDir->Size
> SecDataDir->VirtualAddress + SecDataDir->Size - OffSet
>
> are used multiple times in DxeImageVerificationHandler(). Introduce helper
> variables for them: "SecDataDirEnd" and "SecDataDirLeft", respectively.
> This saves us multiple calculations and significantly simplifies the code.
>
> Note that all three summands above have type UINT32, therefore the new
> variables are also of type UINT32.
>
> This patch does not change behavior.
>
> (Note that the code already handles the case when the
>
> SecDataDir->VirtualAddress + SecDataDir->Size
>
> UINT32 addition overflows -- namely, in that case, the certificate loop is
> never entered, and the corruption check right after the loop fires.)
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Wenyi Xie <xiewenyi2@huawei.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Wenyi Xie <xiewenyi2@huawei.com>
> ---
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index b08fe24e85aa..377feebb205a 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1652,6 +1652,8 @@ DxeImageVerificationHandler (
> UINT8 *AuthData;
> UINTN AuthDataSize;
> EFI_IMAGE_DATA_DIRECTORY *SecDataDir;
> + UINT32 SecDataDirEnd;
> + UINT32 SecDataDirLeft;
> UINT32 OffSet;
> CHAR16 *NameStr;
> RETURN_STATUS PeCoffStatus;
> @@ -1849,12 +1851,14 @@ DxeImageVerificationHandler (
> // "Attribute Certificate Table".
> // The first certificate starts at offset (SecDataDir->VirtualAddress) from the start of the file.
> //
> + SecDataDirEnd = SecDataDir->VirtualAddress + SecDataDir->Size;
> for (OffSet = SecDataDir->VirtualAddress;
> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
> + OffSet < SecDataDirEnd;
> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) {
> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> - if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof (WIN_CERTIFICATE) ||
> - (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertificate->dwLength) {
> + SecDataDirLeft = SecDataDirEnd - OffSet;
> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
> + SecDataDirLeft < WinCertificate->dwLength) {
> break;
> }
>
> @@ -1948,7 +1952,7 @@ DxeImageVerificationHandler (
> }
> }
>
> - if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) {
> + if (OffSet != SecDataDirEnd) {
> //
> // The Size in Certificate Table or the attribute certificate table is corrupted.
> //
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check
2020-09-01 9:12 ` [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check Laszlo Ersek
2020-09-01 15:40 ` [edk2-devel] " Philippe Mathieu-Daudé
@ 2020-09-02 1:32 ` wenyi,xie
2020-09-02 2:19 ` Xu, Min M
2 siblings, 0 replies; 20+ messages in thread
From: wenyi,xie @ 2020-09-02 1:32 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Jian J Wang, Jiewen Yao, Min Xu
On 2020/9/1 17:12, Laszlo Ersek wrote:
> Currently the (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) check only
> guards the de-referencing of the "WinCertificate" pointer. It does not
> guard the calculation of the pointer itself:
>
> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
>
> This is wrong; if we don't know for sure that we have enough room for a
> WIN_CERTIFICATE, then even creating such a pointer, not just
> de-referencing it, may invoke undefined behavior.
>
> Move the pointer calculation after the size check.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Wenyi Xie <xiewenyi2@huawei.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Wenyi Xie <xiewenyi2@huawei.com>
> ---
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 377feebb205a..100739eb3eb6 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1855,10 +1855,12 @@ DxeImageVerificationHandler (
> for (OffSet = SecDataDir->VirtualAddress;
> OffSet < SecDataDirEnd;
> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))) {
> - WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> SecDataDirLeft = SecDataDirEnd - OffSet;
> - if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
> - SecDataDirLeft < WinCertificate->dwLength) {
> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) {
> + break;
> + }
> + WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> + if (SecDataDirLeft < WinCertificate->dwLength) {
> break;
> }
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)
2020-09-01 9:12 ` [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562) Laszlo Ersek
2020-09-01 15:53 ` [edk2-devel] " Philippe Mathieu-Daudé
@ 2020-09-02 1:32 ` wenyi,xie
2020-09-02 2:34 ` Xu, Min M
2 siblings, 0 replies; 20+ messages in thread
From: wenyi,xie @ 2020-09-02 1:32 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Jian J Wang, Jiewen Yao, Min Xu
On 2020/9/1 17:12, Laszlo Ersek wrote:
> The DxeImageVerificationHandler() function currently checks whether
> "SecDataDir" has enough room for "WinCertificate->dwLength". However, for
> advancing "OffSet", "WinCertificate->dwLength" is aligned to the next
> multiple of 8. If "WinCertificate->dwLength" is large enough, the
> alignment will return 0, and "OffSet" will be stuck at the same value.
>
> Check whether "SecDataDir" has room left for both
> "WinCertificate->dwLength" and the alignment.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Wenyi Xie <xiewenyi2@huawei.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Wenyi Xie <xiewenyi2@huawei.com>
> ---
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 100739eb3eb6..11154b6cc58a 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1860,7 +1860,9 @@ DxeImageVerificationHandler (
> break;
> }
> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> - if (SecDataDirLeft < WinCertificate->dwLength) {
> + if (SecDataDirLeft < WinCertificate->dwLength ||
> + (SecDataDirLeft - WinCertificate->dwLength <
> + ALIGN_SIZE (WinCertificate->dwLength))) {
> break;
> }
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft
2020-09-01 9:12 ` [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft Laszlo Ersek
2020-09-01 15:37 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-09-02 1:31 ` wenyi,xie
@ 2020-09-02 1:55 ` Xu, Min M
2 siblings, 0 replies; 20+ messages in thread
From: Xu, Min M @ 2020-09-02 1:55 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Wang, Jian J, Yao, Jiewen, Wenyi Xie
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, September 01, 2020 5:12 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Wenyi Xie
> <xiewenyi2@huawei.com>
> Subject: [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract
> SecDataDirEnd, SecDataDirLeft
>
> The following two quantities:
>
> SecDataDir->VirtualAddress + SecDataDir->Size
> SecDataDir->VirtualAddress + SecDataDir->Size - OffSet
>
> are used multiple times in DxeImageVerificationHandler(). Introduce helper
> variables for them: "SecDataDirEnd" and "SecDataDirLeft", respectively.
> This saves us multiple calculations and significantly simplifies the code.
>
> Note that all three summands above have type UINT32, therefore the new
> variables are also of type UINT32.
>
> This patch does not change behavior.
>
> (Note that the code already handles the case when the
>
> SecDataDir->VirtualAddress + SecDataDir->Size
>
> UINT32 addition overflows -- namely, in that case, the certificate loop is
> never entered, and the corruption check right after the loop fires.)
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Wenyi Xie <xiewenyi2@huawei.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Min M Xu <min.m.xu@intel.com>
> ---
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 12
> ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index b08fe24e85aa..377feebb205a 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLi
> +++ b.c
> @@ -1652,6 +1652,8 @@ DxeImageVerificationHandler (
> UINT8 *AuthData;
> UINTN AuthDataSize;
> EFI_IMAGE_DATA_DIRECTORY *SecDataDir;
> + UINT32 SecDataDirEnd;
> + UINT32 SecDataDirLeft;
> UINT32 OffSet;
> CHAR16 *NameStr;
> RETURN_STATUS PeCoffStatus;
> @@ -1849,12 +1851,14 @@ DxeImageVerificationHandler (
> // "Attribute Certificate Table".
> // The first certificate starts at offset (SecDataDir->VirtualAddress) from the
> start of the file.
> //
> + SecDataDirEnd = SecDataDir->VirtualAddress + SecDataDir->Size;
> for (OffSet = SecDataDir->VirtualAddress;
> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
> + OffSet < SecDataDirEnd;
> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate-
> >dwLength))) {
> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> - if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof
> (WIN_CERTIFICATE) ||
> - (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <
> WinCertificate->dwLength) {
> + SecDataDirLeft = SecDataDirEnd - OffSet;
> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
> + SecDataDirLeft < WinCertificate->dwLength) {
> break;
> }
>
> @@ -1948,7 +1952,7 @@ DxeImageVerificationHandler (
> }
> }
>
> - if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) {
> + if (OffSet != SecDataDirEnd) {
> //
> // The Size in Certificate Table or the attribute certificate table is
> corrupted.
> //
> --
> 2.19.1.3.g30247aa5d201
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check
2020-09-01 9:12 ` [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check Laszlo Ersek
2020-09-01 15:40 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-09-02 1:32 ` wenyi,xie
@ 2020-09-02 2:19 ` Xu, Min M
2 siblings, 0 replies; 20+ messages in thread
From: Xu, Min M @ 2020-09-02 2:19 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Wang, Jian J, Yao, Jiewen, Wenyi Xie
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, September 01, 2020 5:12 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Wenyi Xie
> <xiewenyi2@huawei.com>
> Subject: [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign
> WinCertificate after size check
>
> Currently the (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) check only
> guards the de-referencing of the "WinCertificate" pointer. It does not guard
> the calculation of the pointer itself:
>
> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
>
> This is wrong; if we don't know for sure that we have enough room for a
> WIN_CERTIFICATE, then even creating such a pointer, not just de-
> referencing it, may invoke undefined behavior.
>
> Move the pointer calculation after the size check.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Wenyi Xie <xiewenyi2@huawei.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Min M Xu <min.m.xu@intel.com>
> ---
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 8
> +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 377feebb205a..100739eb3eb6 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLi
> +++ b.c
> @@ -1855,10 +1855,12 @@ DxeImageVerificationHandler (
> for (OffSet = SecDataDir->VirtualAddress;
> OffSet < SecDataDirEnd;
> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate-
> >dwLength))) {
> - WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> SecDataDirLeft = SecDataDirEnd - OffSet;
> - if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
> - SecDataDirLeft < WinCertificate->dwLength) {
> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) {
> + break;
> + }
> + WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> + if (SecDataDirLeft < WinCertificate->dwLength) {
> break;
> }
>
> --
> 2.19.1.3.g30247aa5d201
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)
2020-09-01 9:12 ` [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562) Laszlo Ersek
2020-09-01 15:53 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-09-02 1:32 ` wenyi,xie
@ 2020-09-02 2:34 ` Xu, Min M
2 siblings, 0 replies; 20+ messages in thread
From: Xu, Min M @ 2020-09-02 2:34 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Wang, Jian J, Yao, Jiewen, Wenyi Xie
On September 01, 2020 5:12 PM, Laszlo Ersek wrote:
> The DxeImageVerificationHandler() function currently checks whether
> "SecDataDir" has enough room for "WinCertificate->dwLength". However,
> for advancing "OffSet", "WinCertificate->dwLength" is aligned to the next
> multiple of 8. If "WinCertificate->dwLength" is large enough, the alignment
> will return 0, and "OffSet" will be stuck at the same value.
>
> Check whether "SecDataDir" has room left for both "WinCertificate-
> >dwLength" and the alignment.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Wenyi Xie <xiewenyi2@huawei.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Min M Xu <min.m.xu@intel.com>
> ---
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 4
> +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 100739eb3eb6..11154b6cc58a 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLi
> +++ b.c
> @@ -1860,7 +1860,9 @@ DxeImageVerificationHandler (
> break;
> }
> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> - if (SecDataDirLeft < WinCertificate->dwLength) {
> + if (SecDataDirLeft < WinCertificate->dwLength ||
> + (SecDataDirLeft - WinCertificate->dwLength <
> + ALIGN_SIZE (WinCertificate->dwLength))) {
> break;
> }
>
> --
> 2.19.1.3.g30247aa5d201
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)
2020-09-01 9:12 [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562) Laszlo Ersek
` (2 preceding siblings ...)
2020-09-01 9:12 ` [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562) Laszlo Ersek
@ 2020-09-02 4:02 ` Yao, Jiewen
2020-09-02 6:35 ` Laszlo Ersek
3 siblings, 1 reply; 20+ messages in thread
From: Yao, Jiewen @ 2020-09-02 4:02 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com
Cc: Wang, Jian J, Xu, Min M, Wenyi Xie
The series (1~3) is reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Tuesday, September 1, 2020 5:12 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Xu, Min M <min.m.xu@intel.com>; Wenyi Xie <xiewenyi2@huawei.com>
> Subject: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch
> alignment overflow (CVE-2019-14562)
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> Repo: https://pagure.io/lersek/edk2.git
> Branch: tianocore_2215
>
> I'm neutral on whether this becomes part of edk2-stable202008.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Wenyi Xie <xiewenyi2@huawei.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (3):
> SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd,
> SecDataDirLeft
> SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size
> check
> SecurityPkg/DxeImageVerificationLib: catch alignment overflow
> (CVE-2019-14562)
>
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 16
> ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> --
> 2.19.1.3.g30247aa5d201
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)
2020-09-02 4:02 ` [edk2-devel] [PATCH 0/3] " Yao, Jiewen
@ 2020-09-02 6:35 ` Laszlo Ersek
2020-09-02 6:41 ` Yao, Jiewen
0 siblings, 1 reply; 20+ messages in thread
From: Laszlo Ersek @ 2020-09-02 6:35 UTC (permalink / raw)
To: devel, jiewen.yao
Cc: Wang, Jian J, Xu, Min M, Wenyi Xie, Philippe Mathieu-Daudé,
Liming Gao (Byosoft address)
(+Liming, +Phil)
On 09/02/20 06:02, Yao, Jiewen wrote:
> The series (1~3) is reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Thank you Everyone for the reviews and testing.
Jiewen: do you think we should merge this series into the master branch
before edk2-stable202008? I think it qualifies (it is a CVE fix), but I
would like *you* to decide about it.
Thanks
Laszlo
>
> Thank you
> Yao Jiewen
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Tuesday, September 1, 2020 5:12 PM
>> To: edk2-devel-groups-io <devel@edk2.groups.io>
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
>> Xu, Min M <min.m.xu@intel.com>; Wenyi Xie <xiewenyi2@huawei.com>
>> Subject: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch
>> alignment overflow (CVE-2019-14562)
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
>> Repo: https://pagure.io/lersek/edk2.git
>> Branch: tianocore_2215
>>
>> I'm neutral on whether this becomes part of edk2-stable202008.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Wenyi Xie <xiewenyi2@huawei.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (3):
>> SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd,
>> SecDataDirLeft
>> SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size
>> check
>> SecurityPkg/DxeImageVerificationLib: catch alignment overflow
>> (CVE-2019-14562)
>>
>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 16
>> ++++++++++++----
>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> --
>> 2.19.1.3.g30247aa5d201
>>
>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)
2020-09-02 6:35 ` Laszlo Ersek
@ 2020-09-02 6:41 ` Yao, Jiewen
2020-09-02 6:46 ` Laszlo Ersek
2020-09-02 10:33 ` Laszlo Ersek
0 siblings, 2 replies; 20+ messages in thread
From: Yao, Jiewen @ 2020-09-02 6:41 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com
Cc: Wang, Jian J, Xu, Min M, Wenyi Xie, Philippe Mathieu-Daudé,
Liming Gao (Byosoft address)
Yes. I recommend to merge to stable202008.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Wednesday, September 2, 2020 2:35 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Xu, Min M <min.m.xu@intel.com>;
> Wenyi Xie <xiewenyi2@huawei.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Liming Gao (Byosoft address)
> <gaoliming@byosoft.com.cn>
> Subject: Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib:
> catch alignment overflow (CVE-2019-14562)
>
> (+Liming, +Phil)
>
> On 09/02/20 06:02, Yao, Jiewen wrote:
> > The series (1~3) is reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>
> Thank you Everyone for the reviews and testing.
>
> Jiewen: do you think we should merge this series into the master branch
> before edk2-stable202008? I think it qualifies (it is a CVE fix), but I
> would like *you* to decide about it.
>
> Thanks
> Laszlo
>
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> >> Sent: Tuesday, September 1, 2020 5:12 PM
> >> To: edk2-devel-groups-io <devel@edk2.groups.io>
> >> Cc: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>;
> >> Xu, Min M <min.m.xu@intel.com>; Wenyi Xie <xiewenyi2@huawei.com>
> >> Subject: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib:
> catch
> >> alignment overflow (CVE-2019-14562)
> >>
> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> >> Repo: https://pagure.io/lersek/edk2.git
> >> Branch: tianocore_2215
> >>
> >> I'm neutral on whether this becomes part of edk2-stable202008.
> >>
> >> Cc: Jian J Wang <jian.j.wang@intel.com>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Min Xu <min.m.xu@intel.com>
> >> Cc: Wenyi Xie <xiewenyi2@huawei.com>
> >>
> >> Thanks,
> >> Laszlo
> >>
> >> Laszlo Ersek (3):
> >> SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd,
> >> SecDataDirLeft
> >> SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size
> >> check
> >> SecurityPkg/DxeImageVerificationLib: catch alignment overflow
> >> (CVE-2019-14562)
> >>
> >> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 16
> >> ++++++++++++----
> >> 1 file changed, 12 insertions(+), 4 deletions(-)
> >>
> >> --
> >> 2.19.1.3.g30247aa5d201
> >>
> >>
> >>
> >
> >
> >
> >
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)
2020-09-02 6:41 ` Yao, Jiewen
@ 2020-09-02 6:46 ` Laszlo Ersek
2020-09-02 8:58 ` 回复: " gaoliming
2020-09-02 10:33 ` Laszlo Ersek
1 sibling, 1 reply; 20+ messages in thread
From: Laszlo Ersek @ 2020-09-02 6:46 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io
Cc: Wang, Jian J, Xu, Min M, Wenyi Xie, Philippe Mathieu-Daudé,
Liming Gao (Byosoft address)
On 09/02/20 08:41, Yao, Jiewen wrote:
> Yes. I recommend to merge to stable202008.
Thank you, I will do that soon.
Laszlo
>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Wednesday, September 2, 2020 2:35 PM
>> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Xu, Min M <min.m.xu@intel.com>;
>> Wenyi Xie <xiewenyi2@huawei.com>; Philippe Mathieu-Daudé
>> <philmd@redhat.com>; Liming Gao (Byosoft address)
>> <gaoliming@byosoft.com.cn>
>> Subject: Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib:
>> catch alignment overflow (CVE-2019-14562)
>>
>> (+Liming, +Phil)
>>
>> On 09/02/20 06:02, Yao, Jiewen wrote:
>>> The series (1~3) is reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>>
>> Thank you Everyone for the reviews and testing.
>>
>> Jiewen: do you think we should merge this series into the master branch
>> before edk2-stable202008? I think it qualifies (it is a CVE fix), but I
>> would like *you* to decide about it.
>>
>> Thanks
>> Laszlo
>>
>>>
>>> Thank you
>>> Yao Jiewen
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
>> Ersek
>>>> Sent: Tuesday, September 1, 2020 5:12 PM
>>>> To: edk2-devel-groups-io <devel@edk2.groups.io>
>>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>;
>>>> Xu, Min M <min.m.xu@intel.com>; Wenyi Xie <xiewenyi2@huawei.com>
>>>> Subject: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib:
>> catch
>>>> alignment overflow (CVE-2019-14562)
>>>>
>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
>>>> Repo: https://pagure.io/lersek/edk2.git
>>>> Branch: tianocore_2215
>>>>
>>>> I'm neutral on whether this becomes part of edk2-stable202008.
>>>>
>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>> Cc: Min Xu <min.m.xu@intel.com>
>>>> Cc: Wenyi Xie <xiewenyi2@huawei.com>
>>>>
>>>> Thanks,
>>>> Laszlo
>>>>
>>>> Laszlo Ersek (3):
>>>> SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd,
>>>> SecDataDirLeft
>>>> SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size
>>>> check
>>>> SecurityPkg/DxeImageVerificationLib: catch alignment overflow
>>>> (CVE-2019-14562)
>>>>
>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 16
>>>> ++++++++++++----
>>>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> --
>>>> 2.19.1.3.g30247aa5d201
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* 回复: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)
2020-09-02 6:46 ` Laszlo Ersek
@ 2020-09-02 8:58 ` gaoliming
0 siblings, 0 replies; 20+ messages in thread
From: gaoliming @ 2020-09-02 8:58 UTC (permalink / raw)
To: 'Laszlo Ersek', 'Yao, Jiewen', devel
Cc: 'Wang, Jian J', 'Xu, Min M', 'Wenyi Xie',
'Philippe Mathieu-Daudé'
Laszlo:
I am ok to merge it as the security bug fix for this stable tag.
Thanks
Liming
> -----邮件原件-----
> 发件人: Laszlo Ersek <lersek@redhat.com>
> 发送时间: 2020年9月2日 14:46
> 收件人: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> 抄送: Wang, Jian J <jian.j.wang@intel.com>; Xu, Min M
> <min.m.xu@intel.com>; Wenyi Xie <xiewenyi2@huawei.com>; Philippe
> Mathieu-Daudé <philmd@redhat.com>; Liming Gao (Byosoft address)
> <gaoliming@byosoft.com.cn>
> 主题: Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib:
> catch alignment overflow (CVE-2019-14562)
>
> On 09/02/20 08:41, Yao, Jiewen wrote:
> > Yes. I recommend to merge to stable202008.
>
> Thank you, I will do that soon.
> Laszlo
>
> >
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> >> Sent: Wednesday, September 2, 2020 2:35 PM
> >> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> >> Cc: Wang, Jian J <jian.j.wang@intel.com>; Xu, Min M
> <min.m.xu@intel.com>;
> >> Wenyi Xie <xiewenyi2@huawei.com>; Philippe Mathieu-Daudé
> >> <philmd@redhat.com>; Liming Gao (Byosoft address)
> >> <gaoliming@byosoft.com.cn>
> >> Subject: Re: [edk2-devel] [PATCH 0/3]
> SecurityPkg/DxeImageVerificationLib:
> >> catch alignment overflow (CVE-2019-14562)
> >>
> >> (+Liming, +Phil)
> >>
> >> On 09/02/20 06:02, Yao, Jiewen wrote:
> >>> The series (1~3) is reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
> >>
> >> Thank you Everyone for the reviews and testing.
> >>
> >> Jiewen: do you think we should merge this series into the master branch
> >> before edk2-stable202008? I think it qualifies (it is a CVE fix), but I
> >> would like *you* to decide about it.
> >>
> >> Thanks
> >> Laszlo
> >>
> >>>
> >>> Thank you
> >>> Yao Jiewen
> >>>
> >>>> -----Original Message-----
> >>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Laszlo
> >> Ersek
> >>>> Sent: Tuesday, September 1, 2020 5:12 PM
> >>>> To: edk2-devel-groups-io <devel@edk2.groups.io>
> >>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen
> >> <jiewen.yao@intel.com>;
> >>>> Xu, Min M <min.m.xu@intel.com>; Wenyi Xie
> <xiewenyi2@huawei.com>
> >>>> Subject: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib:
> >> catch
> >>>> alignment overflow (CVE-2019-14562)
> >>>>
> >>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> >>>> Repo: https://pagure.io/lersek/edk2.git
> >>>> Branch: tianocore_2215
> >>>>
> >>>> I'm neutral on whether this becomes part of edk2-stable202008.
> >>>>
> >>>> Cc: Jian J Wang <jian.j.wang@intel.com>
> >>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >>>> Cc: Min Xu <min.m.xu@intel.com>
> >>>> Cc: Wenyi Xie <xiewenyi2@huawei.com>
> >>>>
> >>>> Thanks,
> >>>> Laszlo
> >>>>
> >>>> Laszlo Ersek (3):
> >>>> SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd,
> >>>> SecDataDirLeft
> >>>> SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size
> >>>> check
> >>>> SecurityPkg/DxeImageVerificationLib: catch alignment overflow
> >>>> (CVE-2019-14562)
> >>>>
> >>>>
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 16
> >>>> ++++++++++++----
> >>>> 1 file changed, 12 insertions(+), 4 deletions(-)
> >>>>
> >>>> --
> >>>> 2.19.1.3.g30247aa5d201
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)
2020-09-02 6:41 ` Yao, Jiewen
2020-09-02 6:46 ` Laszlo Ersek
@ 2020-09-02 10:33 ` Laszlo Ersek
1 sibling, 0 replies; 20+ messages in thread
From: Laszlo Ersek @ 2020-09-02 10:33 UTC (permalink / raw)
To: devel, jiewen.yao
Cc: Wang, Jian J, Xu, Min M, Wenyi Xie, Philippe Mathieu-Daudé,
Liming Gao (Byosoft address)
On 09/02/20 08:41, Yao, Jiewen wrote:
> Yes. I recommend to merge to stable202008.
Merged in commit range 751355992635..0b143fa43e92, via
<https://github.com/tianocore/edk2/pull/911>.
Thanks!
Laszlo
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Wednesday, September 2, 2020 2:35 PM
>> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Xu, Min M <min.m.xu@intel.com>;
>> Wenyi Xie <xiewenyi2@huawei.com>; Philippe Mathieu-Daudé
>> <philmd@redhat.com>; Liming Gao (Byosoft address)
>> <gaoliming@byosoft.com.cn>
>> Subject: Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib:
>> catch alignment overflow (CVE-2019-14562)
>>
>> (+Liming, +Phil)
>>
>> On 09/02/20 06:02, Yao, Jiewen wrote:
>>> The series (1~3) is reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>>
>> Thank you Everyone for the reviews and testing.
>>
>> Jiewen: do you think we should merge this series into the master branch
>> before edk2-stable202008? I think it qualifies (it is a CVE fix), but I
>> would like *you* to decide about it.
>>
>> Thanks
>> Laszlo
>>
>>>
>>> Thank you
>>> Yao Jiewen
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
>> Ersek
>>>> Sent: Tuesday, September 1, 2020 5:12 PM
>>>> To: edk2-devel-groups-io <devel@edk2.groups.io>
>>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>;
>>>> Xu, Min M <min.m.xu@intel.com>; Wenyi Xie <xiewenyi2@huawei.com>
>>>> Subject: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib:
>> catch
>>>> alignment overflow (CVE-2019-14562)
>>>>
>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2215
>>>> Repo: https://pagure.io/lersek/edk2.git
>>>> Branch: tianocore_2215
>>>>
>>>> I'm neutral on whether this becomes part of edk2-stable202008.
>>>>
>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>> Cc: Min Xu <min.m.xu@intel.com>
>>>> Cc: Wenyi Xie <xiewenyi2@huawei.com>
>>>>
>>>> Thanks,
>>>> Laszlo
>>>>
>>>> Laszlo Ersek (3):
>>>> SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd,
>>>> SecDataDirLeft
>>>> SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size
>>>> check
>>>> SecurityPkg/DxeImageVerificationLib: catch alignment overflow
>>>> (CVE-2019-14562)
>>>>
>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 16
>>>> ++++++++++++----
>>>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> --
>>>> 2.19.1.3.g30247aa5d201
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-09-02 10:33 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-01 9:12 [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562) Laszlo Ersek
2020-09-01 9:12 ` [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft Laszlo Ersek
2020-09-01 15:37 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-09-02 1:31 ` wenyi,xie
2020-09-02 1:55 ` Xu, Min M
2020-09-01 9:12 ` [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check Laszlo Ersek
2020-09-01 15:40 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-09-02 1:32 ` wenyi,xie
2020-09-02 2:19 ` Xu, Min M
2020-09-01 9:12 ` [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562) Laszlo Ersek
2020-09-01 15:53 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-09-01 16:52 ` Laszlo Ersek
2020-09-02 1:32 ` wenyi,xie
2020-09-02 2:34 ` Xu, Min M
2020-09-02 4:02 ` [edk2-devel] [PATCH 0/3] " Yao, Jiewen
2020-09-02 6:35 ` Laszlo Ersek
2020-09-02 6:41 ` Yao, Jiewen
2020-09-02 6:46 ` Laszlo Ersek
2020-09-02 8:58 ` 回复: " gaoliming
2020-09-02 10:33 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox