[edk2-devel] [PATCH 3/9] SecurityPkg/DxeImageVerificationLib: fix wrong fetching dbx in IsAllowedByDb(CVE-2019-14575)

Wang, Jian J posted 9 patches 4 years, 9 months ago
There is a newer version of this series
[edk2-devel] [PATCH 3/9] SecurityPkg/DxeImageVerificationLib: fix wrong fetching dbx in IsAllowedByDb(CVE-2019-14575)
Posted by Wang, Jian J 4 years, 9 months ago
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608

Normally two times of calling gRT->GetVariable() are needed to get
the data of a variable: get the variable size by passing zero variable
size, and then allocate enough memory and pass the correct variable size
and buffer.

But in the inner loop in IsAllowedByDb(), the DbxDataSize was not
initialized to zero before calling gRT->GetVariable(). It won't cause
problem if dbx does not exist. But it will give wrong result if dbx
exists and the DbxDataSize happens to be a small enough value. In this
situation, EFI_BUFFER_TOO_SMALL will be returned. Then the result check
code followed will jump to 'Done', which is not correct because it's
actually the value expected.

            if (Status == EFI_BUFFER_TOO_SMALL) {
              goto Done;
            }

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 .../Library/DxeImageVerificationLib/DxeImageVerificationLib.c  | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 5dcd6efed5..1efb2f96cd 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1456,8 +1456,9 @@ IsAllowedByDb (
             //
             // Here We still need to check if this RootCert's Hash is revoked
             //
+            DbxDataSize = 0;
             Status   = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, NULL);
-            if (Status == EFI_BUFFER_TOO_SMALL) {
+            if (Status != EFI_BUFFER_TOO_SMALL) {
               goto Done;
             }
             DbxData = (UINT8 *) AllocateZeroPool (DbxDataSize);
-- 
2.24.0.windows.2


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53869): https://edk2.groups.io/g/devel/message/53869
Mute This Topic: https://groups.io/mt/71023420/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/9] SecurityPkg/DxeImageVerificationLib: fix wrong fetching dbx in IsAllowedByDb(CVE-2019-14575)
Posted by Yao, Jiewen 4 years, 9 months ago
Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Thursday, February 6, 2020 10:19 PM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>
> Subject: [PATCH 3/9] SecurityPkg/DxeImageVerificationLib: fix wrong fetching
> dbx in IsAllowedByDb(CVE-2019-14575)
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
> 
> Normally two times of calling gRT->GetVariable() are needed to get
> the data of a variable: get the variable size by passing zero variable
> size, and then allocate enough memory and pass the correct variable size
> and buffer.
> 
> But in the inner loop in IsAllowedByDb(), the DbxDataSize was not
> initialized to zero before calling gRT->GetVariable(). It won't cause
> problem if dbx does not exist. But it will give wrong result if dbx
> exists and the DbxDataSize happens to be a small enough value. In this
> situation, EFI_BUFFER_TOO_SMALL will be returned. Then the result check
> code followed will jump to 'Done', which is not correct because it's
> actually the value expected.
> 
>             if (Status == EFI_BUFFER_TOO_SMALL) {
>               goto Done;
>             }
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  .../Library/DxeImageVerificationLib/DxeImageVerificationLib.c  | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 5dcd6efed5..1efb2f96cd 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1456,8 +1456,9 @@ IsAllowedByDb (
>              //
> 
>              // Here We still need to check if this RootCert's Hash is revoked
> 
>              //
> 
> +            DbxDataSize = 0;
> 
>              Status   = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1,
> &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, NULL);
> 
> -            if (Status == EFI_BUFFER_TOO_SMALL) {
> 
> +            if (Status != EFI_BUFFER_TOO_SMALL) {
> 
>                goto Done;
> 
>              }
> 
>              DbxData = (UINT8 *) AllocateZeroPool (DbxDataSize);
> 
> --
> 2.24.0.windows.2


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#54347): https://edk2.groups.io/g/devel/message/54347
Mute This Topic: https://groups.io/mt/71023420/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-