[PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[Laszlo Ersek]

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

[PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft

[Philippe Mathieu-Daudé]

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.
>      //
> 

Re: [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft

[wenyi,xie]

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.
>      //
> 

Re: [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract SecDataDirEnd, SecDataDirLeft

[Xu, Min M]

> -----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
> 

[PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check

[Philippe Mathieu-Daudé]

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;
>      }
>  
> 

Re: [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check

[wenyi,xie]

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;
>      }
>  
> 

Re: [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign WinCertificate after size check

[Xu, Min M]

> -----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
> 

[PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[Philippe Mathieu-Daudé]

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;
>      }
>  
> 

Re: [edk2-devel] [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[Laszlo Ersek]

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;
>>      }
>>
>>
>

Re: [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[wenyi,xie]

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;
>      }
>  
> 

Re: [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[Xu, Min M]

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

Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[Yao, Jiewen]

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
> 
> 
> 

Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[Laszlo Ersek]

(+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
>>
>>
>>
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[Yao, Jiewen]

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
> >>
> >>
> >>
> >
> >
> >
> >
> 
> 
> 

Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[Laszlo Ersek]

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
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>> 
> 

回复: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[gaoliming]

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
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >> 
> >

Re: [edk2-devel] [PATCH 0/3] SecurityPkg/DxeImageVerificationLib: catch alignment overflow (CVE-2019-14562)

[Laszlo Ersek]

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
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
> 
> 
> 
>