[v1] Fix false negative issue in DxeImageVerificationHandler

[edk2-devel] [PATCH 1/9] SecurityPkg/DxeImageVerificationLib: Fix memory leaks(CVE-2019-14575)

Posted by Wang, Jian J 4 years, 9 months ago

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608

Pointer HashCtx used in IsCertHashFoundInDatabase() is not freed inside
the while-loop, if it will run more than once.

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, 3 insertions(+)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index dbfbfcb4fb..74dbffa122 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -908,6 +908,9 @@ IsCertHashFoundInDatabase (
       goto Done;
     }
 
+    FreePool (HashCtx);
+    HashCtx = NULL;
+
     SiglistHeaderSize = sizeof (EFI_SIGNATURE_LIST) + DbxList->SignatureHeaderSize;
     CertHash          = (EFI_SIGNATURE_DATA *) ((UINT8 *) DbxList + SiglistHeaderSize);
     CertHashCount     = (DbxList->SignatureListSize - SiglistHeaderSize) / DbxList->SignatureSize;
-- 
2.24.0.windows.2


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

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

Re: [edk2-devel] [PATCH 1/9] SecurityPkg/DxeImageVerificationLib: Fix memory leaks(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 1/9] SecurityPkg/DxeImageVerificationLib: Fix memory
> leaks(CVE-2019-14575)
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
> 
> Pointer HashCtx used in IsCertHashFoundInDatabase() is not freed inside
> the while-loop, if it will run more than once.
> 
> 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, 3 insertions(+)
> 
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index dbfbfcb4fb..74dbffa122 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -908,6 +908,9 @@ IsCertHashFoundInDatabase (
>        goto Done;
> 
>      }
> 
> 
> 
> +    FreePool (HashCtx);
> 
> +    HashCtx = NULL;
> 
> +
> 
>      SiglistHeaderSize = sizeof (EFI_SIGNATURE_LIST) + DbxList-
> >SignatureHeaderSize;
> 
>      CertHash          = (EFI_SIGNATURE_DATA *) ((UINT8 *) DbxList +
> SiglistHeaderSize);
> 
>      CertHashCount     = (DbxList->SignatureListSize - SiglistHeaderSize) / DbxList-
> >SignatureSize;
> 
> --
> 2.24.0.windows.2


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

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

Re: [edk2-devel] [PATCH 1/9] SecurityPkg/DxeImageVerificationLib: Fix memory leaks(CVE-2019-14575)

Posted by Philippe Mathieu-Daudé 4 years, 9 months ago

On 2/6/20 3:19 PM, Wang, Jian J wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
> 
> Pointer HashCtx used in IsCertHashFoundInDatabase() is not freed inside
> the while-loop, if it will run more than once.

By extracting part of the code from the big while() statement into a new 
function, IsCertHashFoundInDatabase() would be easier to review (and 
this mistake could have been avoided).

> 
> 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, 3 insertions(+)
> 
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index dbfbfcb4fb..74dbffa122 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -908,6 +908,9 @@ IsCertHashFoundInDatabase (
>         goto Done;
>       }
>   
> +    FreePool (HashCtx);
> +    HashCtx = NULL;
> +
>       SiglistHeaderSize = sizeof (EFI_SIGNATURE_LIST) + DbxList->SignatureHeaderSize;
>       CertHash          = (EFI_SIGNATURE_DATA *) ((UINT8 *) DbxList + SiglistHeaderSize);
>       CertHashCount     = (DbxList->SignatureListSize - SiglistHeaderSize) / DbxList->SignatureSize;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>


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

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