* [PATCH 1/9] SecurityPkg/DxeImageVerificationLib: Fix memory leaks(CVE-2019-14575)
2020-02-06 14:19 [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Wang, Jian J
@ 2020-02-06 14:19 ` Wang, Jian J
2020-02-13 9:34 ` Yao, Jiewen
2020-02-13 16:43 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-02-06 14:19 ` [PATCH 2/9] SecurityPkg/DxeImageVerificationLib: reject CertStack.CertNumber==0 per DBX(CVE-2019-14575) Wang, Jian J
` (8 subsequent siblings)
9 siblings, 2 replies; 27+ messages in thread
From: Wang, Jian J @ 2020-02-06 14:19 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Chao Zhang
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 1/9] SecurityPkg/DxeImageVerificationLib: Fix memory leaks(CVE-2019-14575)
2020-02-06 14:19 ` [PATCH 1/9] SecurityPkg/DxeImageVerificationLib: Fix memory leaks(CVE-2019-14575) Wang, Jian J
@ 2020-02-13 9:34 ` Yao, Jiewen
2020-02-13 16:43 ` [edk2-devel] " Philippe Mathieu-Daudé
1 sibling, 0 replies; 27+ messages in thread
From: Yao, Jiewen @ 2020-02-13 9:34 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io; +Cc: Zhang, Chao B
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 1/9] SecurityPkg/DxeImageVerificationLib: Fix memory leaks(CVE-2019-14575)
2020-02-06 14:19 ` [PATCH 1/9] SecurityPkg/DxeImageVerificationLib: Fix memory leaks(CVE-2019-14575) Wang, Jian J
2020-02-13 9:34 ` Yao, Jiewen
@ 2020-02-13 16:43 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-13 16:43 UTC (permalink / raw)
To: devel, jian.j.wang; +Cc: Jiewen Yao, Chao Zhang
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>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/9] SecurityPkg/DxeImageVerificationLib: reject CertStack.CertNumber==0 per DBX(CVE-2019-14575)
2020-02-06 14:19 [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Wang, Jian J
2020-02-06 14:19 ` [PATCH 1/9] SecurityPkg/DxeImageVerificationLib: Fix memory leaks(CVE-2019-14575) Wang, Jian J
@ 2020-02-06 14:19 ` Wang, Jian J
2020-02-13 9:36 ` Yao, Jiewen
2020-02-06 14:19 ` [PATCH 3/9] SecurityPkg/DxeImageVerificationLib: fix wrong fetching dbx in IsAllowedByDb(CVE-2019-14575) Wang, Jian J
` (7 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Wang, Jian J @ 2020-02-06 14:19 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Chao Zhang, Laszlo Ersek
In case the signers' certificate stack, retrieved from the PE/COFF image's
Authenticode blob, has zero elements (= there are zero signer certificates),
then we should consider the image forbidden by DBX, not accepted by DBX.
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
.../Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 74dbffa122..5dcd6efed5 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1326,7 +1326,7 @@ IsForbiddenByDbx (
// UINT8 Certn[];
//
Pkcs7GetSigners (AuthData, AuthDataSize, &CertBuffer, &BufferLength, &TrustedCert, &TrustedCertLength);
- if ((BufferLength == 0) || (CertBuffer == NULL)) {
+ if ((BufferLength == 0) || (CertBuffer == NULL) || (*CertBuffer) == 0) {
IsForbidden = TRUE;
goto Done;
}
--
2.24.0.windows.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 2/9] SecurityPkg/DxeImageVerificationLib: reject CertStack.CertNumber==0 per DBX(CVE-2019-14575)
2020-02-06 14:19 ` [PATCH 2/9] SecurityPkg/DxeImageVerificationLib: reject CertStack.CertNumber==0 per DBX(CVE-2019-14575) Wang, Jian J
@ 2020-02-13 9:36 ` Yao, Jiewen
0 siblings, 0 replies; 27+ messages in thread
From: Yao, Jiewen @ 2020-02-13 9:36 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io; +Cc: Zhang, Chao B, Laszlo Ersek
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>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 2/9] SecurityPkg/DxeImageVerificationLib: reject
> CertStack.CertNumber==0 per DBX(CVE-2019-14575)
>
> In case the signers' certificate stack, retrieved from the PE/COFF image's
> Authenticode blob, has zero elements (= there are zero signer certificates),
> then we should consider the image forbidden by DBX, not accepted by DBX.
>
> 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> .../Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 74dbffa122..5dcd6efed5 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1326,7 +1326,7 @@ IsForbiddenByDbx (
> // UINT8 Certn[];
>
> //
>
> Pkcs7GetSigners (AuthData, AuthDataSize, &CertBuffer, &BufferLength,
> &TrustedCert, &TrustedCertLength);
>
> - if ((BufferLength == 0) || (CertBuffer == NULL)) {
>
> + if ((BufferLength == 0) || (CertBuffer == NULL) || (*CertBuffer) == 0) {
>
> IsForbidden = TRUE;
>
> goto Done;
>
> }
>
> --
> 2.24.0.windows.2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/9] SecurityPkg/DxeImageVerificationLib: fix wrong fetching dbx in IsAllowedByDb(CVE-2019-14575)
2020-02-06 14:19 [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Wang, Jian J
2020-02-06 14:19 ` [PATCH 1/9] SecurityPkg/DxeImageVerificationLib: Fix memory leaks(CVE-2019-14575) Wang, Jian J
2020-02-06 14:19 ` [PATCH 2/9] SecurityPkg/DxeImageVerificationLib: reject CertStack.CertNumber==0 per DBX(CVE-2019-14575) Wang, Jian J
@ 2020-02-06 14:19 ` Wang, Jian J
2020-02-13 9:38 ` Yao, Jiewen
2020-02-06 14:19 ` [PATCH 4/9] SecurityPkg/DxeImageVerificationLib: avoid bypass in " Wang, Jian J
` (6 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Wang, Jian J @ 2020-02-06 14:19 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Chao Zhang
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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 3/9] SecurityPkg/DxeImageVerificationLib: fix wrong fetching dbx in IsAllowedByDb(CVE-2019-14575)
2020-02-06 14:19 ` [PATCH 3/9] SecurityPkg/DxeImageVerificationLib: fix wrong fetching dbx in IsAllowedByDb(CVE-2019-14575) Wang, Jian J
@ 2020-02-13 9:38 ` Yao, Jiewen
0 siblings, 0 replies; 27+ messages in thread
From: Yao, Jiewen @ 2020-02-13 9:38 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io; +Cc: Zhang, Chao B
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 4/9] SecurityPkg/DxeImageVerificationLib: avoid bypass in fetching dbx in IsAllowedByDb(CVE-2019-14575)
2020-02-06 14:19 [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Wang, Jian J
` (2 preceding siblings ...)
2020-02-06 14:19 ` [PATCH 3/9] SecurityPkg/DxeImageVerificationLib: fix wrong fetching dbx in IsAllowedByDb(CVE-2019-14575) Wang, Jian J
@ 2020-02-06 14:19 ` Wang, Jian J
2020-02-13 9:39 ` Yao, Jiewen
2020-02-06 14:19 ` [PATCH 5/9] SecurityPkg/DxeImageVerificationLib: refactor db/dbx fetching code " Wang, Jian J
` (5 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Wang, Jian J @ 2020-02-06 14:19 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
In timestamp check after the cert is found in db, the original code jumps
to 'Done' if any error happens in fetching dbx variable. At any of the jump,
VerifyStatus equals to TRUE, which means allowed-by-db. This should not
be allowed except to EFI_NOT_FOUND case (meaning dbx doesn't exist), because
it could be used to bypass timestamp check.
This patch add code to change VerifyStatus to FALSE in the case of memory
allocation failure and dbx fetching failure to avoid potential bypass issue.
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>
---
.../DxeImageVerificationLib/DxeImageVerificationLib.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 1efb2f96cd..ed5dbf26b0 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1459,15 +1459,26 @@ IsAllowedByDb (
DbxDataSize = 0;
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, NULL);
if (Status != EFI_BUFFER_TOO_SMALL) {
+ if (Status != EFI_NOT_FOUND) {
+ VerifyStatus = FALSE;
+ }
goto Done;
}
DbxData = (UINT8 *) AllocateZeroPool (DbxDataSize);
if (DbxData == NULL) {
+ //
+ // Force not-allowed-by-db to avoid bypass
+ //
+ VerifyStatus = FALSE;
goto Done;
}
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, (VOID *) DbxData);
if (EFI_ERROR (Status)) {
+ //
+ // Force not-allowed-by-db to avoid bypass
+ //
+ VerifyStatus = FALSE;
goto Done;
}
--
2.24.0.windows.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 4/9] SecurityPkg/DxeImageVerificationLib: avoid bypass in fetching dbx in IsAllowedByDb(CVE-2019-14575)
2020-02-06 14:19 ` [PATCH 4/9] SecurityPkg/DxeImageVerificationLib: avoid bypass in " Wang, Jian J
@ 2020-02-13 9:39 ` Yao, Jiewen
0 siblings, 0 replies; 27+ messages in thread
From: Yao, Jiewen @ 2020-02-13 9:39 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io; +Cc: Zhang, Chao B
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 4/9] SecurityPkg/DxeImageVerificationLib: avoid bypass in
> fetching dbx in IsAllowedByDb(CVE-2019-14575)
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
>
> In timestamp check after the cert is found in db, the original code jumps
> to 'Done' if any error happens in fetching dbx variable. At any of the jump,
> VerifyStatus equals to TRUE, which means allowed-by-db. This should not
> be allowed except to EFI_NOT_FOUND case (meaning dbx doesn't exist),
> because
> it could be used to bypass timestamp check.
>
> This patch add code to change VerifyStatus to FALSE in the case of memory
> allocation failure and dbx fetching failure to avoid potential bypass issue.
>
> 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>
> ---
> .../DxeImageVerificationLib/DxeImageVerificationLib.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 1efb2f96cd..ed5dbf26b0 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1459,15 +1459,26 @@ IsAllowedByDb (
> DbxDataSize = 0;
>
> Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1,
> &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, NULL);
>
> if (Status != EFI_BUFFER_TOO_SMALL) {
>
> + if (Status != EFI_NOT_FOUND) {
>
> + VerifyStatus = FALSE;
>
> + }
>
> goto Done;
>
> }
>
> DbxData = (UINT8 *) AllocateZeroPool (DbxDataSize);
>
> if (DbxData == NULL) {
>
> + //
>
> + // Force not-allowed-by-db to avoid bypass
>
> + //
>
> + VerifyStatus = FALSE;
>
> goto Done;
>
> }
>
>
>
> Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1,
> &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, (VOID *) DbxData);
>
> if (EFI_ERROR (Status)) {
>
> + //
>
> + // Force not-allowed-by-db to avoid bypass
>
> + //
>
> + VerifyStatus = FALSE;
>
> goto Done;
>
> }
>
>
>
> --
> 2.24.0.windows.2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 5/9] SecurityPkg/DxeImageVerificationLib: refactor db/dbx fetching code in IsAllowedByDb(CVE-2019-14575)
2020-02-06 14:19 [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Wang, Jian J
` (3 preceding siblings ...)
2020-02-06 14:19 ` [PATCH 4/9] SecurityPkg/DxeImageVerificationLib: avoid bypass in " Wang, Jian J
@ 2020-02-06 14:19 ` Wang, Jian J
2020-02-13 9:44 ` Yao, Jiewen
2020-02-06 14:19 ` [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575) Wang, Jian J
` (4 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Wang, Jian J @ 2020-02-06 14:19 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
The dbx fetching code inside the while/for-loop causes code hard to
understand. Since there's no need to get dbx more than once, this patch
simplify the code logic by moving related code to be outside the while-
loop. db fetching code is also refined accordingly to reduce the indent
level of code.
More comments are also added or refined to explain more details.
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>
---
.../DxeImageVerificationLib.c | 144 ++++++++++--------
1 file changed, 83 insertions(+), 61 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index ed5dbf26b0..8739d1fa29 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1412,76 +1412,92 @@ IsAllowedByDb (
RootCertSize = 0;
VerifyStatus = FALSE;
+ //
+ // Fetch 'db' content. If 'db' doesn't exist or encounters problem to get the
+ // data, return not-allowed-by-db (FALSE).
+ //
DataSize = 0;
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, NULL);
- if (Status == EFI_BUFFER_TOO_SMALL) {
- Data = (UINT8 *) AllocateZeroPool (DataSize);
- if (Data == NULL) {
- return VerifyStatus;
+ ASSERT (EFI_ERROR (Status));
+ if (Status != EFI_BUFFER_TOO_SMALL) {
+ return VerifyStatus;
+ }
+
+ Data = (UINT8 *) AllocateZeroPool (DataSize);
+ if (Data == NULL) {
+ return VerifyStatus;
+ }
+
+ Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, (VOID *) Data);
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ //
+ // Fetch 'dbx' content. If 'dbx' doesn't exist, continue to check 'db'.
+ // If any other errors occured, no need to check 'db' but just return
+ // not-allowed-by-db (FALSE) to avoid bypass.
+ //
+ DbxDataSize = 0;
+ Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, NULL);
+ ASSERT (EFI_ERROR (Status));
+ if (Status != EFI_BUFFER_TOO_SMALL) {
+ if (Status != EFI_NOT_FOUND) {
+ goto Done;
+ }
+ //
+ // 'dbx' does not exist. Continue to check 'db'.
+ //
+ } else {
+ //
+ // 'dbx' exists. Get its content.
+ //
+ DbxData = (UINT8 *) AllocateZeroPool (DbxDataSize);
+ if (DbxData == NULL) {
+ goto Done;
}
- Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, (VOID *) Data);
+ Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, (VOID *) DbxData);
if (EFI_ERROR (Status)) {
goto Done;
}
+ }
- //
- // Find X509 certificate in Signature List to verify the signature in pkcs7 signed data.
- //
- CertList = (EFI_SIGNATURE_LIST *) Data;
- while ((DataSize > 0) && (DataSize >= CertList->SignatureListSize)) {
- if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) {
- CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
- CertCount = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) - CertList->SignatureHeaderSize) / CertList->SignatureSize;
+ //
+ // Find X509 certificate in Signature List to verify the signature in pkcs7 signed data.
+ //
+ CertList = (EFI_SIGNATURE_LIST *) Data;
+ while ((DataSize > 0) && (DataSize >= CertList->SignatureListSize)) {
+ if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) {
+ CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
+ CertCount = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) - CertList->SignatureHeaderSize) / CertList->SignatureSize;
- for (Index = 0; Index < CertCount; Index++) {
- //
- // Iterate each Signature Data Node within this CertList for verify.
- //
- RootCert = CertData->SignatureData;
- RootCertSize = CertList->SignatureSize - sizeof (EFI_GUID);
+ for (Index = 0; Index < CertCount; Index++) {
+ //
+ // Iterate each Signature Data Node within this CertList for verify.
+ //
+ RootCert = CertData->SignatureData;
+ RootCertSize = CertList->SignatureSize - sizeof (EFI_GUID);
+ //
+ // Call AuthenticodeVerify library to Verify Authenticode struct.
+ //
+ VerifyStatus = AuthenticodeVerify (
+ AuthData,
+ AuthDataSize,
+ RootCert,
+ RootCertSize,
+ mImageDigest,
+ mImageDigestSize
+ );
+ if (VerifyStatus) {
//
- // Call AuthenticodeVerify library to Verify Authenticode struct.
+ // The image is signed and its signature is found in 'db'.
//
- VerifyStatus = AuthenticodeVerify (
- AuthData,
- AuthDataSize,
- RootCert,
- RootCertSize,
- mImageDigest,
- mImageDigestSize
- );
- if (VerifyStatus) {
+ if (DbxData != NULL) {
//
// 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_NOT_FOUND) {
- VerifyStatus = FALSE;
- }
- goto Done;
- }
- DbxData = (UINT8 *) AllocateZeroPool (DbxDataSize);
- if (DbxData == NULL) {
- //
- // Force not-allowed-by-db to avoid bypass
- //
- VerifyStatus = FALSE;
- goto Done;
- }
-
- Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, (VOID *) DbxData);
- if (EFI_ERROR (Status)) {
- //
- // Force not-allowed-by-db to avoid bypass
- //
- VerifyStatus = FALSE;
- goto Done;
- }
-
if (IsCertHashFoundInDatabase (RootCert, RootCertSize, (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
//
// Check the timestamp signature and signing time to determine if the RootCert can be trusted.
@@ -1491,17 +1507,23 @@ IsAllowedByDb (
DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed and signature is accepted by DB, but its root cert failed the timestamp check.\n"));
}
}
-
- goto Done;
}
- CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertData + CertList->SignatureSize);
+ //
+ // There's no 'dbx' to check revocation time against (must-be pass),
+ // or, there's revocation time found in 'dbx' and checked againt 'dbt'
+ // (maybe pass or fail, depending on timestamp compare result). Either
+ // way the verification job has been completed at this point.
+ //
+ goto Done;
}
- }
- DataSize -= CertList->SignatureListSize;
- CertList = (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + CertList->SignatureListSize);
+ CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertData + CertList->SignatureSize);
+ }
}
+
+ DataSize -= CertList->SignatureListSize;
+ CertList = (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + CertList->SignatureListSize);
}
Done:
--
2.24.0.windows.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 5/9] SecurityPkg/DxeImageVerificationLib: refactor db/dbx fetching code in IsAllowedByDb(CVE-2019-14575)
2020-02-06 14:19 ` [PATCH 5/9] SecurityPkg/DxeImageVerificationLib: refactor db/dbx fetching code " Wang, Jian J
@ 2020-02-13 9:44 ` Yao, Jiewen
0 siblings, 0 replies; 27+ messages in thread
From: Yao, Jiewen @ 2020-02-13 9:44 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io; +Cc: Zhang, Chao B
Good enhancement.
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 5/9] SecurityPkg/DxeImageVerificationLib: refactor db/dbx
> fetching code in IsAllowedByDb(CVE-2019-14575)
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
>
> The dbx fetching code inside the while/for-loop causes code hard to
> understand. Since there's no need to get dbx more than once, this patch
> simplify the code logic by moving related code to be outside the while-
> loop. db fetching code is also refined accordingly to reduce the indent
> level of code.
>
> More comments are also added or refined to explain more details.
>
> 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>
> ---
> .../DxeImageVerificationLib.c | 144 ++++++++++--------
> 1 file changed, 83 insertions(+), 61 deletions(-)
>
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index ed5dbf26b0..8739d1fa29 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1412,76 +1412,92 @@ IsAllowedByDb (
> RootCertSize = 0;
>
> VerifyStatus = FALSE;
>
>
>
> + //
>
> + // Fetch 'db' content. If 'db' doesn't exist or encounters problem to get the
>
> + // data, return not-allowed-by-db (FALSE).
>
> + //
>
> DataSize = 0;
>
> Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE,
> &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, NULL);
>
> - if (Status == EFI_BUFFER_TOO_SMALL) {
>
> - Data = (UINT8 *) AllocateZeroPool (DataSize);
>
> - if (Data == NULL) {
>
> - return VerifyStatus;
>
> + ASSERT (EFI_ERROR (Status));
>
> + if (Status != EFI_BUFFER_TOO_SMALL) {
>
> + return VerifyStatus;
>
> + }
>
> +
>
> + Data = (UINT8 *) AllocateZeroPool (DataSize);
>
> + if (Data == NULL) {
>
> + return VerifyStatus;
>
> + }
>
> +
>
> + Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE,
> &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, (VOID *) Data);
>
> + if (EFI_ERROR (Status)) {
>
> + goto Done;
>
> + }
>
> +
>
> + //
>
> + // Fetch 'dbx' content. If 'dbx' doesn't exist, continue to check 'db'.
>
> + // If any other errors occured, no need to check 'db' but just return
>
> + // not-allowed-by-db (FALSE) to avoid bypass.
>
> + //
>
> + DbxDataSize = 0;
>
> + Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1,
> &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, NULL);
>
> + ASSERT (EFI_ERROR (Status));
>
> + if (Status != EFI_BUFFER_TOO_SMALL) {
>
> + if (Status != EFI_NOT_FOUND) {
>
> + goto Done;
>
> + }
>
> + //
>
> + // 'dbx' does not exist. Continue to check 'db'.
>
> + //
>
> + } else {
>
> + //
>
> + // 'dbx' exists. Get its content.
>
> + //
>
> + DbxData = (UINT8 *) AllocateZeroPool (DbxDataSize);
>
> + if (DbxData == NULL) {
>
> + goto Done;
>
> }
>
>
>
> - Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE,
> &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, (VOID *) Data);
>
> + Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1,
> &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, (VOID *) DbxData);
>
> if (EFI_ERROR (Status)) {
>
> goto Done;
>
> }
>
> + }
>
>
>
> - //
>
> - // Find X509 certificate in Signature List to verify the signature in pkcs7 signed
> data.
>
> - //
>
> - CertList = (EFI_SIGNATURE_LIST *) Data;
>
> - while ((DataSize > 0) && (DataSize >= CertList->SignatureListSize)) {
>
> - if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) {
>
> - CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof
> (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
>
> - CertCount = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) -
> CertList->SignatureHeaderSize) / CertList->SignatureSize;
>
> + //
>
> + // Find X509 certificate in Signature List to verify the signature in pkcs7 signed
> data.
>
> + //
>
> + CertList = (EFI_SIGNATURE_LIST *) Data;
>
> + while ((DataSize > 0) && (DataSize >= CertList->SignatureListSize)) {
>
> + if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) {
>
> + CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof
> (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
>
> + CertCount = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) -
> CertList->SignatureHeaderSize) / CertList->SignatureSize;
>
>
>
> - for (Index = 0; Index < CertCount; Index++) {
>
> - //
>
> - // Iterate each Signature Data Node within this CertList for verify.
>
> - //
>
> - RootCert = CertData->SignatureData;
>
> - RootCertSize = CertList->SignatureSize - sizeof (EFI_GUID);
>
> + for (Index = 0; Index < CertCount; Index++) {
>
> + //
>
> + // Iterate each Signature Data Node within this CertList for verify.
>
> + //
>
> + RootCert = CertData->SignatureData;
>
> + RootCertSize = CertList->SignatureSize - sizeof (EFI_GUID);
>
>
>
> + //
>
> + // Call AuthenticodeVerify library to Verify Authenticode struct.
>
> + //
>
> + VerifyStatus = AuthenticodeVerify (
>
> + AuthData,
>
> + AuthDataSize,
>
> + RootCert,
>
> + RootCertSize,
>
> + mImageDigest,
>
> + mImageDigestSize
>
> + );
>
> + if (VerifyStatus) {
>
> //
>
> - // Call AuthenticodeVerify library to Verify Authenticode struct.
>
> + // The image is signed and its signature is found in 'db'.
>
> //
>
> - VerifyStatus = AuthenticodeVerify (
>
> - AuthData,
>
> - AuthDataSize,
>
> - RootCert,
>
> - RootCertSize,
>
> - mImageDigest,
>
> - mImageDigestSize
>
> - );
>
> - if (VerifyStatus) {
>
> + if (DbxData != NULL) {
>
> //
>
> // 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_NOT_FOUND) {
>
> - VerifyStatus = FALSE;
>
> - }
>
> - goto Done;
>
> - }
>
> - DbxData = (UINT8 *) AllocateZeroPool (DbxDataSize);
>
> - if (DbxData == NULL) {
>
> - //
>
> - // Force not-allowed-by-db to avoid bypass
>
> - //
>
> - VerifyStatus = FALSE;
>
> - goto Done;
>
> - }
>
> -
>
> - Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1,
> &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, (VOID *) DbxData);
>
> - if (EFI_ERROR (Status)) {
>
> - //
>
> - // Force not-allowed-by-db to avoid bypass
>
> - //
>
> - VerifyStatus = FALSE;
>
> - goto Done;
>
> - }
>
> -
>
> if (IsCertHashFoundInDatabase (RootCert, RootCertSize,
> (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
>
> //
>
> // Check the timestamp signature and signing time to determine if the
> RootCert can be trusted.
>
> @@ -1491,17 +1507,23 @@ IsAllowedByDb (
> DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed and
> signature is accepted by DB, but its root cert failed the timestamp check.\n"));
>
> }
>
> }
>
> -
>
> - goto Done;
>
> }
>
>
>
> - CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertData + CertList-
> >SignatureSize);
>
> + //
>
> + // There's no 'dbx' to check revocation time against (must-be pass),
>
> + // or, there's revocation time found in 'dbx' and checked againt 'dbt'
>
> + // (maybe pass or fail, depending on timestamp compare result). Either
>
> + // way the verification job has been completed at this point.
>
> + //
>
> + goto Done;
>
> }
>
> - }
>
>
>
> - DataSize -= CertList->SignatureListSize;
>
> - CertList = (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + CertList-
> >SignatureListSize);
>
> + CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertData + CertList-
> >SignatureSize);
>
> + }
>
> }
>
> +
>
> + DataSize -= CertList->SignatureListSize;
>
> + CertList = (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + CertList-
> >SignatureListSize);
>
> }
>
>
>
> Done:
>
> --
> 2.24.0.windows.2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
2020-02-06 14:19 [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Wang, Jian J
` (4 preceding siblings ...)
2020-02-06 14:19 ` [PATCH 5/9] SecurityPkg/DxeImageVerificationLib: refactor db/dbx fetching code " Wang, Jian J
@ 2020-02-06 14:19 ` Wang, Jian J
2020-02-13 10:11 ` Yao, Jiewen
2020-02-06 14:19 ` [PATCH 7/9] SecurityPkg/DxeImageVerificationLib: tighten default result of IsForbiddenByDbx()(CVE-2019-14575) Wang, Jian J
` (3 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Wang, Jian J @ 2020-02-06 14:19 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Chao Zhang, Laszlo Ersek
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
To avoid false-negative issue in check hash against dbx, both error
condition (as return value) and check result (as out parameter) of
IsCertHashFoundInDatabase() are added. So the caller of this function
will know exactly if a failure is caused by a black list hit or
other error happening, and enforce a more secure operation to prevent
secure boot from being bypassed. For a white list check (db), there's
no such necessity.
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>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.../DxeImageVerificationLib.c | 68 +++++++++++--------
1 file changed, 41 insertions(+), 27 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 8739d1fa29..a5dfee0f8e 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -822,22 +822,23 @@ AddImageExeInfo (
@param[in] SignatureList Pointer to the Signature List in forbidden database.
@param[in] SignatureListSize Size of Signature List.
@param[out] RevocationTime Return the time that the certificate was revoked.
+ @param[out] IsFound Search result. Only valid if EFI_SUCCESS returned.
- @return TRUE The certificate hash is found in the forbidden database.
- @return FALSE The certificate hash is not found in the forbidden database.
+ @retval EFI_SUCCESS Finished the search without any error.
+ @retval Others Error occurred in the search of database.
**/
-BOOLEAN
+EFI_STATUS
IsCertHashFoundInDatabase (
IN UINT8 *Certificate,
IN UINTN CertSize,
IN EFI_SIGNATURE_LIST *SignatureList,
IN UINTN SignatureListSize,
- OUT EFI_TIME *RevocationTime
+ OUT EFI_TIME *RevocationTime,
+ OUT BOOLEAN *IsFound
)
{
- BOOLEAN IsFound;
- BOOLEAN Status;
+ EFI_STATUS Status;
EFI_SIGNATURE_LIST *DbxList;
UINTN DbxSize;
EFI_SIGNATURE_DATA *CertHash;
@@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
UINT8 *TBSCert;
UINTN TBSCertSize;
- IsFound = FALSE;
+ Status = EFI_ABORTED;
+ *IsFound = FALSE;
DbxList = SignatureList;
DbxSize = SignatureListSize;
HashCtx = NULL;
HashAlg = HASHALG_MAX;
if ((RevocationTime == NULL) || (DbxList == NULL)) {
- return FALSE;
+ return EFI_INVALID_PARAMETER;
}
//
// Retrieve the TBSCertificate from the X.509 Certificate.
//
if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {
- return FALSE;
+ return Status;
}
while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {
@@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
if (HashCtx == NULL) {
goto Done;
}
- Status = mHash[HashAlg].HashInit (HashCtx);
- if (!Status) {
+ if (!mHash[HashAlg].HashInit (HashCtx)) {
goto Done;
}
- Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);
- if (!Status) {
+ if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {
goto Done;
}
- Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);
- if (!Status) {
+ if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {
goto Done;
}
@@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
//
// Hash of Certificate is found in forbidden database.
//
- IsFound = TRUE;
+ Status = EFI_SUCCESS;
+ *IsFound = TRUE;
//
// Return the revocation time.
@@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
DbxList = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList->SignatureListSize);
}
+ Status = EFI_SUCCESS;
+
Done:
if (HashCtx != NULL) {
FreePool (HashCtx);
}
- return IsFound;
+ return Status;
}
/**
@@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
{
EFI_STATUS Status;
BOOLEAN IsForbidden;
+ BOOLEAN IsFound;
UINT8 *Data;
UINTN DataSize;
EFI_SIGNATURE_LIST *CertList;
@@ -1344,12 +1347,13 @@ IsForbiddenByDbx (
//
CertPtr = CertPtr + sizeof (UINT32) + CertSize;
- if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST *)Data, DataSize, &RevocationTime)) {
+ Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST *)Data, DataSize, &RevocationTime, &IsFound);
+ if (EFI_ERROR (Status) || IsFound) {
//
// Check the timestamp signature and signing time to determine if the image can be trusted.
//
IsForbidden = TRUE;
- if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
+ if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
IsForbidden = FALSE;
//
// Pass DBT check. Continue to check other certs in image signer's cert list against DBX, DBT
@@ -1392,6 +1396,7 @@ IsAllowedByDb (
{
EFI_STATUS Status;
BOOLEAN VerifyStatus;
+ BOOLEAN IsFound;
EFI_SIGNATURE_LIST *CertList;
EFI_SIGNATURE_DATA *CertData;
UINTN DataSize;
@@ -1495,17 +1500,26 @@ IsAllowedByDb (
// The image is signed and its signature is found in 'db'.
//
if (DbxData != NULL) {
+ VerifyStatus = FALSE;
//
// Here We still need to check if this RootCert's Hash is revoked
//
- if (IsCertHashFoundInDatabase (RootCert, RootCertSize, (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
- //
- // Check the timestamp signature and signing time to determine if the RootCert can be trusted.
- //
- VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime);
- if (!VerifyStatus) {
- DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed and signature is accepted by DB, but its root cert failed the timestamp check.\n"));
- }
+ Status = IsCertHashFoundInDatabase (RootCert, RootCertSize, (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ if (!IsFound) {
+ VerifyStatus = TRUE;
+ goto Done;
+ }
+
+ //
+ // Check the timestamp signature and signing time to determine if the RootCert can be trusted.
+ //
+ VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime);
+ if (!VerifyStatus) {
+ DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed and signature is accepted by DB, but its root cert failed the timestamp check.\n"));
}
}
--
2.24.0.windows.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
2020-02-06 14:19 ` [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575) Wang, Jian J
@ 2020-02-13 10:11 ` Yao, Jiewen
2020-02-13 15:07 ` Wang, Jian J
0 siblings, 1 reply; 27+ messages in thread
From: Yao, Jiewen @ 2020-02-13 10:11 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io; +Cc: Zhang, Chao B, Laszlo Ersek
Comment below:
1) I think the function name - IsCertHashFoundInDatabase() and the implementation { DbxList = SignatureList; DbxSize = SignatureListSize; } bring some confusion to me.
If this is a *generic* database search function, I recommend we use a generic name - not use DbxList/DbxSize in the function implementation.
If the input SignatureList of the function must be *Dbx*, I recommend we use IsCertHashFoundInDbx() as the function name.
Either change is OK for me.
2) Now we have to check 2 output: Status and IsFound in IsCertHashFoundInDatabase().
I am struggling to understand the different between 2 different ways of error handling:
===========================
Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST *)Data, DataSize, &RevocationTime, &IsFound);
if (EFI_ERROR (Status) || IsFound) {
//
// Check the timestamp signature and signing time to determine if the image can be trusted.
//
IsForbidden = TRUE;
if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
IsForbidden = FALSE;
============================
and
============================
VerifyStatus = FALSE;
//
// Here We still need to check if this RootCert's Hash is revoked
//
Status = IsCertHashFoundInDatabase (RootCert, RootCertSize, (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);
if (EFI_ERROR (Status)) {
goto Done;
}
if (!IsFound) {
VerifyStatus = TRUE;
goto Done;
}
//
// Check the timestamp signature and signing time to determine if the RootCert can be trusted.
//
VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime);
if (!VerifyStatus) {
===============================
I *believe* the logic behind is same. If so, we can use a consistent way to check the 2 output and decide if PassTimestampCheck() is required.
Or, can we create a one single function to perform such check for both IsCertHashFoundInDatabase() and PassTimestampCheck() ?
If I am wrong, there is *difference* between them. Then I think we need much better description to help reviewer to catch the difference.
Thank you
Yao Jiewen
> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Thursday, February 6, 2020 10:20 PM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error
> and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
>
> To avoid false-negative issue in check hash against dbx, both error
> condition (as return value) and check result (as out parameter) of
> IsCertHashFoundInDatabase() are added. So the caller of this function
> will know exactly if a failure is caused by a black list hit or
> other error happening, and enforce a more secure operation to prevent
> secure boot from being bypassed. For a white list check (db), there's
> no such necessity.
>
> 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>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> .../DxeImageVerificationLib.c | 68 +++++++++++--------
> 1 file changed, 41 insertions(+), 27 deletions(-)
>
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 8739d1fa29..a5dfee0f8e 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -822,22 +822,23 @@ AddImageExeInfo (
> @param[in] SignatureList Pointer to the Signature List in forbidden database.
>
> @param[in] SignatureListSize Size of Signature List.
>
> @param[out] RevocationTime Return the time that the certificate was
> revoked.
>
> + @param[out] IsFound Search result. Only valid if EFI_SUCCESS returned.
>
>
>
> - @return TRUE The certificate hash is found in the forbidden database.
>
> - @return FALSE The certificate hash is not found in the forbidden database.
>
> + @retval EFI_SUCCESS Finished the search without any error.
>
> + @retval Others Error occurred in the search of database.
>
>
>
> **/
>
> -BOOLEAN
>
> +EFI_STATUS
>
> IsCertHashFoundInDatabase (
>
> IN UINT8 *Certificate,
>
> IN UINTN CertSize,
>
> IN EFI_SIGNATURE_LIST *SignatureList,
>
> IN UINTN SignatureListSize,
>
> - OUT EFI_TIME *RevocationTime
>
> + OUT EFI_TIME *RevocationTime,
>
> + OUT BOOLEAN *IsFound
>
> )
>
> {
>
> - BOOLEAN IsFound;
>
> - BOOLEAN Status;
>
> + EFI_STATUS Status;
>
> EFI_SIGNATURE_LIST *DbxList;
>
> UINTN DbxSize;
>
> EFI_SIGNATURE_DATA *CertHash;
>
> @@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
> UINT8 *TBSCert;
>
> UINTN TBSCertSize;
>
>
>
> - IsFound = FALSE;
>
> + Status = EFI_ABORTED;
>
> + *IsFound = FALSE;
>
> DbxList = SignatureList;
>
> DbxSize = SignatureListSize;
>
> HashCtx = NULL;
>
> HashAlg = HASHALG_MAX;
>
>
>
> if ((RevocationTime == NULL) || (DbxList == NULL)) {
>
> - return FALSE;
>
> + return EFI_INVALID_PARAMETER;
>
> }
>
>
>
> //
>
> // Retrieve the TBSCertificate from the X.509 Certificate.
>
> //
>
> if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {
>
> - return FALSE;
>
> + return Status;
>
> }
>
>
>
> while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {
>
> @@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
> if (HashCtx == NULL) {
>
> goto Done;
>
> }
>
> - Status = mHash[HashAlg].HashInit (HashCtx);
>
> - if (!Status) {
>
> + if (!mHash[HashAlg].HashInit (HashCtx)) {
>
> goto Done;
>
> }
>
> - Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);
>
> - if (!Status) {
>
> + if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {
>
> goto Done;
>
> }
>
> - Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);
>
> - if (!Status) {
>
> + if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {
>
> goto Done;
>
> }
>
>
>
> @@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
> //
>
> // Hash of Certificate is found in forbidden database.
>
> //
>
> - IsFound = TRUE;
>
> + Status = EFI_SUCCESS;
>
> + *IsFound = TRUE;
>
>
>
> //
>
> // Return the revocation time.
>
> @@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
> DbxList = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList-
> >SignatureListSize);
>
> }
>
>
>
> + Status = EFI_SUCCESS;
>
> +
>
> Done:
>
> if (HashCtx != NULL) {
>
> FreePool (HashCtx);
>
> }
>
>
>
> - return IsFound;
>
> + return Status;
>
> }
>
>
>
> /**
>
> @@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
> {
>
> EFI_STATUS Status;
>
> BOOLEAN IsForbidden;
>
> + BOOLEAN IsFound;
>
> UINT8 *Data;
>
> UINTN DataSize;
>
> EFI_SIGNATURE_LIST *CertList;
>
> @@ -1344,12 +1347,13 @@ IsForbiddenByDbx (
> //
>
> CertPtr = CertPtr + sizeof (UINT32) + CertSize;
>
>
>
> - if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST *)Data,
> DataSize, &RevocationTime)) {
>
> + Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> *)Data, DataSize, &RevocationTime, &IsFound);
>
> + if (EFI_ERROR (Status) || IsFound) {
>
> //
>
> // Check the timestamp signature and signing time to determine if the image
> can be trusted.
>
> //
>
> IsForbidden = TRUE;
>
> - if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
>
> + if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime)) {
>
> IsForbidden = FALSE;
>
> //
>
> // Pass DBT check. Continue to check other certs in image signer's cert list
> against DBX, DBT
>
> @@ -1392,6 +1396,7 @@ IsAllowedByDb (
> {
>
> EFI_STATUS Status;
>
> BOOLEAN VerifyStatus;
>
> + BOOLEAN IsFound;
>
> EFI_SIGNATURE_LIST *CertList;
>
> EFI_SIGNATURE_DATA *CertData;
>
> UINTN DataSize;
>
> @@ -1495,17 +1500,26 @@ IsAllowedByDb (
> // The image is signed and its signature is found in 'db'.
>
> //
>
> if (DbxData != NULL) {
>
> + VerifyStatus = FALSE;
>
> //
>
> // Here We still need to check if this RootCert's Hash is revoked
>
> //
>
> - if (IsCertHashFoundInDatabase (RootCert, RootCertSize,
> (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
>
> - //
>
> - // Check the timestamp signature and signing time to determine if the
> RootCert can be trusted.
>
> - //
>
> - VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime);
>
> - if (!VerifyStatus) {
>
> - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed and
> signature is accepted by DB, but its root cert failed the timestamp check.\n"));
>
> - }
>
> + Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);
>
> + if (EFI_ERROR (Status)) {
>
> + goto Done;
>
> + }
>
> +
>
> + if (!IsFound) {
>
> + VerifyStatus = TRUE;
>
> + goto Done;
>
> + }
>
> +
>
> + //
>
> + // Check the timestamp signature and signing time to determine if the
> RootCert can be trusted.
>
> + //
>
> + VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime);
>
> + if (!VerifyStatus) {
>
> + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed and
> signature is accepted by DB, but its root cert failed the timestamp check.\n"));
>
> }
>
> }
>
>
>
> --
> 2.24.0.windows.2
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
2020-02-13 10:11 ` Yao, Jiewen
@ 2020-02-13 15:07 ` Wang, Jian J
2020-02-14 0:54 ` Yao, Jiewen
0 siblings, 1 reply; 27+ messages in thread
From: Wang, Jian J @ 2020-02-13 15:07 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io; +Cc: Zhang, Chao B, Laszlo Ersek
Jiewen,
Thanks for the comments.
1) You're right. IsCertHashFoundInDatabase is quite general and cause confusions between
db and dbx situation. Since it's not newly introduced in this patch series, do you think it's ok
to fix it in separate patch series later? Or do you prefer fix it in this patch series? I'm ok with
both.
2) I checked both code again. I think you're right. Both callings are for dbx, any error Status
should be taken as IsFound(==TRUE). What about following change for the second case?
Please help double check if any logic hole here.
Status = IsCertHashFoundInDatabase (...);
if (EFI_ERROR (Status) || IsFound) {
//
// Check the timestamp signature and signing time to determine if the RootCert can be trusted.
//
VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime);
if (!VerifyStatus) {
DEBUG ((...));
}
} else {
VerifyStatus = TRUE;
}
goto Done;
Regards,
Jian
> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Thursday, February 13, 2020 6:11 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
>
> Comment below:
>
> 1) I think the function name - IsCertHashFoundInDatabase() and the
> implementation { DbxList = SignatureList; DbxSize = SignatureListSize; } bring
> some confusion to me.
>
> If this is a *generic* database search function, I recommend we use a generic
> name - not use DbxList/DbxSize in the function implementation.
>
> If the input SignatureList of the function must be *Dbx*, I recommend we use
> IsCertHashFoundInDbx() as the function name.
>
> Either change is OK for me.
>
> 2) Now we have to check 2 output: Status and IsFound in
> IsCertHashFoundInDatabase().
>
> I am struggling to understand the different between 2 different ways of error
> handling:
>
> ===========================
> Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> *)Data, DataSize, &RevocationTime, &IsFound);
> if (EFI_ERROR (Status) || IsFound) {
> //
> // Check the timestamp signature and signing time to determine if the image
> can be trusted.
> //
> IsForbidden = TRUE;
> if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime)) {
> IsForbidden = FALSE;
> ============================
>
> and
>
> ============================
> VerifyStatus = FALSE;
> //
> // Here We still need to check if this RootCert's Hash is revoked
> //
> Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);
> if (EFI_ERROR (Status)) {
> goto Done;
> }
>
> if (!IsFound) {
> VerifyStatus = TRUE;
> goto Done;
> }
>
> //
> // Check the timestamp signature and signing time to determine if the
> RootCert can be trusted.
> //
> VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime);
> if (!VerifyStatus) {
> ===============================
>
> I *believe* the logic behind is same. If so, we can use a consistent way to check
> the 2 output and decide if PassTimestampCheck() is required.
>
> Or, can we create a one single function to perform such check for both
> IsCertHashFoundInDatabase() and PassTimestampCheck() ?
>
> If I am wrong, there is *difference* between them. Then I think we need much
> better description to help reviewer to catch the difference.
>
> Thank you
> Yao Jiewen
>
>
> > -----Original Message-----
> > From: Wang, Jian J <jian.j.wang@intel.com>
> > Sent: Thursday, February 6, 2020 10:20 PM
> > To: devel@edk2.groups.io
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > Subject: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error
> > and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
> >
> > To avoid false-negative issue in check hash against dbx, both error
> > condition (as return value) and check result (as out parameter) of
> > IsCertHashFoundInDatabase() are added. So the caller of this function
> > will know exactly if a failure is caused by a black list hit or
> > other error happening, and enforce a more secure operation to prevent
> > secure boot from being bypassed. For a white list check (db), there's
> > no such necessity.
> >
> > 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>
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> > .../DxeImageVerificationLib.c | 68 +++++++++++--------
> > 1 file changed, 41 insertions(+), 27 deletions(-)
> >
> > diff --git
> > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > index 8739d1fa29..a5dfee0f8e 100644
> > --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > @@ -822,22 +822,23 @@ AddImageExeInfo (
> > @param[in] SignatureList Pointer to the Signature List in forbidden
> database.
> >
> > @param[in] SignatureListSize Size of Signature List.
> >
> > @param[out] RevocationTime Return the time that the certificate was
> > revoked.
> >
> > + @param[out] IsFound Search result. Only valid if EFI_SUCCESS
> returned.
> >
> >
> >
> > - @return TRUE The certificate hash is found in the forbidden database.
> >
> > - @return FALSE The certificate hash is not found in the forbidden database.
> >
> > + @retval EFI_SUCCESS Finished the search without any error.
> >
> > + @retval Others Error occurred in the search of database.
> >
> >
> >
> > **/
> >
> > -BOOLEAN
> >
> > +EFI_STATUS
> >
> > IsCertHashFoundInDatabase (
> >
> > IN UINT8 *Certificate,
> >
> > IN UINTN CertSize,
> >
> > IN EFI_SIGNATURE_LIST *SignatureList,
> >
> > IN UINTN SignatureListSize,
> >
> > - OUT EFI_TIME *RevocationTime
> >
> > + OUT EFI_TIME *RevocationTime,
> >
> > + OUT BOOLEAN *IsFound
> >
> > )
> >
> > {
> >
> > - BOOLEAN IsFound;
> >
> > - BOOLEAN Status;
> >
> > + EFI_STATUS Status;
> >
> > EFI_SIGNATURE_LIST *DbxList;
> >
> > UINTN DbxSize;
> >
> > EFI_SIGNATURE_DATA *CertHash;
> >
> > @@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
> > UINT8 *TBSCert;
> >
> > UINTN TBSCertSize;
> >
> >
> >
> > - IsFound = FALSE;
> >
> > + Status = EFI_ABORTED;
> >
> > + *IsFound = FALSE;
> >
> > DbxList = SignatureList;
> >
> > DbxSize = SignatureListSize;
> >
> > HashCtx = NULL;
> >
> > HashAlg = HASHALG_MAX;
> >
> >
> >
> > if ((RevocationTime == NULL) || (DbxList == NULL)) {
> >
> > - return FALSE;
> >
> > + return EFI_INVALID_PARAMETER;
> >
> > }
> >
> >
> >
> > //
> >
> > // Retrieve the TBSCertificate from the X.509 Certificate.
> >
> > //
> >
> > if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {
> >
> > - return FALSE;
> >
> > + return Status;
> >
> > }
> >
> >
> >
> > while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {
> >
> > @@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
> > if (HashCtx == NULL) {
> >
> > goto Done;
> >
> > }
> >
> > - Status = mHash[HashAlg].HashInit (HashCtx);
> >
> > - if (!Status) {
> >
> > + if (!mHash[HashAlg].HashInit (HashCtx)) {
> >
> > goto Done;
> >
> > }
> >
> > - Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);
> >
> > - if (!Status) {
> >
> > + if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {
> >
> > goto Done;
> >
> > }
> >
> > - Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);
> >
> > - if (!Status) {
> >
> > + if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {
> >
> > goto Done;
> >
> > }
> >
> >
> >
> > @@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
> > //
> >
> > // Hash of Certificate is found in forbidden database.
> >
> > //
> >
> > - IsFound = TRUE;
> >
> > + Status = EFI_SUCCESS;
> >
> > + *IsFound = TRUE;
> >
> >
> >
> > //
> >
> > // Return the revocation time.
> >
> > @@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
> > DbxList = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList-
> > >SignatureListSize);
> >
> > }
> >
> >
> >
> > + Status = EFI_SUCCESS;
> >
> > +
> >
> > Done:
> >
> > if (HashCtx != NULL) {
> >
> > FreePool (HashCtx);
> >
> > }
> >
> >
> >
> > - return IsFound;
> >
> > + return Status;
> >
> > }
> >
> >
> >
> > /**
> >
> > @@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
> > {
> >
> > EFI_STATUS Status;
> >
> > BOOLEAN IsForbidden;
> >
> > + BOOLEAN IsFound;
> >
> > UINT8 *Data;
> >
> > UINTN DataSize;
> >
> > EFI_SIGNATURE_LIST *CertList;
> >
> > @@ -1344,12 +1347,13 @@ IsForbiddenByDbx (
> > //
> >
> > CertPtr = CertPtr + sizeof (UINT32) + CertSize;
> >
> >
> >
> > - if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> *)Data,
> > DataSize, &RevocationTime)) {
> >
> > + Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> > *)Data, DataSize, &RevocationTime, &IsFound);
> >
> > + if (EFI_ERROR (Status) || IsFound) {
> >
> > //
> >
> > // Check the timestamp signature and signing time to determine if the
> image
> > can be trusted.
> >
> > //
> >
> > IsForbidden = TRUE;
> >
> > - if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
> >
> > + if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime)) {
> >
> > IsForbidden = FALSE;
> >
> > //
> >
> > // Pass DBT check. Continue to check other certs in image signer's cert list
> > against DBX, DBT
> >
> > @@ -1392,6 +1396,7 @@ IsAllowedByDb (
> > {
> >
> > EFI_STATUS Status;
> >
> > BOOLEAN VerifyStatus;
> >
> > + BOOLEAN IsFound;
> >
> > EFI_SIGNATURE_LIST *CertList;
> >
> > EFI_SIGNATURE_DATA *CertData;
> >
> > UINTN DataSize;
> >
> > @@ -1495,17 +1500,26 @@ IsAllowedByDb (
> > // The image is signed and its signature is found in 'db'.
> >
> > //
> >
> > if (DbxData != NULL) {
> >
> > + VerifyStatus = FALSE;
> >
> > //
> >
> > // Here We still need to check if this RootCert's Hash is revoked
> >
> > //
> >
> > - if (IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
> >
> > - //
> >
> > - // Check the timestamp signature and signing time to determine if the
> > RootCert can be trusted.
> >
> > - //
> >
> > - VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime);
> >
> > - if (!VerifyStatus) {
> >
> > - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed
> and
> > signature is accepted by DB, but its root cert failed the timestamp check.\n"));
> >
> > - }
> >
> > + Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);
> >
> > + if (EFI_ERROR (Status)) {
> >
> > + goto Done;
> >
> > + }
> >
> > +
> >
> > + if (!IsFound) {
> >
> > + VerifyStatus = TRUE;
> >
> > + goto Done;
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // Check the timestamp signature and signing time to determine if the
> > RootCert can be trusted.
> >
> > + //
> >
> > + VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime);
> >
> > + if (!VerifyStatus) {
> >
> > + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed
> and
> > signature is accepted by DB, but its root cert failed the timestamp check.\n"));
> >
> > }
> >
> > }
> >
> >
> >
> > --
> > 2.24.0.windows.2
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
2020-02-13 15:07 ` Wang, Jian J
@ 2020-02-14 0:54 ` Yao, Jiewen
2020-02-14 3:31 ` Wang, Jian J
0 siblings, 1 reply; 27+ messages in thread
From: Yao, Jiewen @ 2020-02-14 0:54 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io; +Cc: Zhang, Chao B, Laszlo Ersek
1) I prefer we do a little bit simple clean up in this series. Just name change. Maybe as patch-10.
2) When PassTimestampCheck() need to be called? Only Dbx is found? Or even the Dbx is broken?
I prefer we need use a consistent rule.
Case 1 in original patch:
if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime)) {
Case 2 in your email:
> VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime);
> if (!VerifyStatus) {
It seems they are not consistent...
Thank you
Yao Jiewen
> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Thursday, February 13, 2020 11:08 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
>
> Jiewen,
>
> Thanks for the comments.
>
> 1) You're right. IsCertHashFoundInDatabase is quite general and cause
> confusions between
> db and dbx situation. Since it's not newly introduced in this patch series, do you
> think it's ok
> to fix it in separate patch series later? Or do you prefer fix it in this patch series?
> I'm ok with
> both.
>
> 2) I checked both code again. I think you're right. Both callings are for dbx, any
> error Status
> should be taken as IsFound(==TRUE). What about following change for the
> second case?
> Please help double check if any logic hole here.
>
> Status = IsCertHashFoundInDatabase (...);
> if (EFI_ERROR (Status) || IsFound) {
> //
> // Check the timestamp signature and signing time to determine if the
> RootCert can be trusted.
> //
> VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> &RevocationTime);
> if (!VerifyStatus) {
> DEBUG ((...));
> }
> } else {
> VerifyStatus = TRUE;
> }
>
> goto Done;
>
> Regards,
> Jian
>
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Thursday, February 13, 2020 6:11 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>
> > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> >
> > Comment below:
> >
> > 1) I think the function name - IsCertHashFoundInDatabase() and the
> > implementation { DbxList = SignatureList; DbxSize = SignatureListSize; } bring
> > some confusion to me.
> >
> > If this is a *generic* database search function, I recommend we use a generic
> > name - not use DbxList/DbxSize in the function implementation.
> >
> > If the input SignatureList of the function must be *Dbx*, I recommend we use
> > IsCertHashFoundInDbx() as the function name.
> >
> > Either change is OK for me.
> >
> > 2) Now we have to check 2 output: Status and IsFound in
> > IsCertHashFoundInDatabase().
> >
> > I am struggling to understand the different between 2 different ways of error
> > handling:
> >
> > ===========================
> > Status = IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> > *)Data, DataSize, &RevocationTime, &IsFound);
> > if (EFI_ERROR (Status) || IsFound) {
> > //
> > // Check the timestamp signature and signing time to determine if the
> image
> > can be trusted.
> > //
> > IsForbidden = TRUE;
> > if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime)) {
> > IsForbidden = FALSE;
> > ============================
> >
> > and
> >
> > ============================
> > VerifyStatus = FALSE;
> > //
> > // Here We still need to check if this RootCert's Hash is revoked
> > //
> > Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime, &IsFound);
> > if (EFI_ERROR (Status)) {
> > goto Done;
> > }
> >
> > if (!IsFound) {
> > VerifyStatus = TRUE;
> > goto Done;
> > }
> >
> > //
> > // Check the timestamp signature and signing time to determine if the
> > RootCert can be trusted.
> > //
> > VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime);
> > if (!VerifyStatus) {
> > ===============================
> >
> > I *believe* the logic behind is same. If so, we can use a consistent way to
> check
> > the 2 output and decide if PassTimestampCheck() is required.
> >
> > Or, can we create a one single function to perform such check for both
> > IsCertHashFoundInDatabase() and PassTimestampCheck() ?
> >
> > If I am wrong, there is *difference* between them. Then I think we need much
> > better description to help reviewer to catch the difference.
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > Sent: Thursday, February 6, 2020 10:20 PM
> > > To: devel@edk2.groups.io
> > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> > > <chao.b.zhang@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > > Subject: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> error
> > > and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
> > >
> > > To avoid false-negative issue in check hash against dbx, both error
> > > condition (as return value) and check result (as out parameter) of
> > > IsCertHashFoundInDatabase() are added. So the caller of this function
> > > will know exactly if a failure is caused by a black list hit or
> > > other error happening, and enforce a more secure operation to prevent
> > > secure boot from being bypassed. For a white list check (db), there's
> > > no such necessity.
> > >
> > > 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>
> > > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > > ---
> > > .../DxeImageVerificationLib.c | 68 +++++++++++--------
> > > 1 file changed, 41 insertions(+), 27 deletions(-)
> > >
> > > diff --git
> > > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > index 8739d1fa29..a5dfee0f8e 100644
> > > ---
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > +++
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > @@ -822,22 +822,23 @@ AddImageExeInfo (
> > > @param[in] SignatureList Pointer to the Signature List in forbidden
> > database.
> > >
> > > @param[in] SignatureListSize Size of Signature List.
> > >
> > > @param[out] RevocationTime Return the time that the certificate was
> > > revoked.
> > >
> > > + @param[out] IsFound Search result. Only valid if EFI_SUCCESS
> > returned.
> > >
> > >
> > >
> > > - @return TRUE The certificate hash is found in the forbidden database.
> > >
> > > - @return FALSE The certificate hash is not found in the forbidden database.
> > >
> > > + @retval EFI_SUCCESS Finished the search without any error.
> > >
> > > + @retval Others Error occurred in the search of database.
> > >
> > >
> > >
> > > **/
> > >
> > > -BOOLEAN
> > >
> > > +EFI_STATUS
> > >
> > > IsCertHashFoundInDatabase (
> > >
> > > IN UINT8 *Certificate,
> > >
> > > IN UINTN CertSize,
> > >
> > > IN EFI_SIGNATURE_LIST *SignatureList,
> > >
> > > IN UINTN SignatureListSize,
> > >
> > > - OUT EFI_TIME *RevocationTime
> > >
> > > + OUT EFI_TIME *RevocationTime,
> > >
> > > + OUT BOOLEAN *IsFound
> > >
> > > )
> > >
> > > {
> > >
> > > - BOOLEAN IsFound;
> > >
> > > - BOOLEAN Status;
> > >
> > > + EFI_STATUS Status;
> > >
> > > EFI_SIGNATURE_LIST *DbxList;
> > >
> > > UINTN DbxSize;
> > >
> > > EFI_SIGNATURE_DATA *CertHash;
> > >
> > > @@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
> > > UINT8 *TBSCert;
> > >
> > > UINTN TBSCertSize;
> > >
> > >
> > >
> > > - IsFound = FALSE;
> > >
> > > + Status = EFI_ABORTED;
> > >
> > > + *IsFound = FALSE;
> > >
> > > DbxList = SignatureList;
> > >
> > > DbxSize = SignatureListSize;
> > >
> > > HashCtx = NULL;
> > >
> > > HashAlg = HASHALG_MAX;
> > >
> > >
> > >
> > > if ((RevocationTime == NULL) || (DbxList == NULL)) {
> > >
> > > - return FALSE;
> > >
> > > + return EFI_INVALID_PARAMETER;
> > >
> > > }
> > >
> > >
> > >
> > > //
> > >
> > > // Retrieve the TBSCertificate from the X.509 Certificate.
> > >
> > > //
> > >
> > > if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {
> > >
> > > - return FALSE;
> > >
> > > + return Status;
> > >
> > > }
> > >
> > >
> > >
> > > while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {
> > >
> > > @@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
> > > if (HashCtx == NULL) {
> > >
> > > goto Done;
> > >
> > > }
> > >
> > > - Status = mHash[HashAlg].HashInit (HashCtx);
> > >
> > > - if (!Status) {
> > >
> > > + if (!mHash[HashAlg].HashInit (HashCtx)) {
> > >
> > > goto Done;
> > >
> > > }
> > >
> > > - Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);
> > >
> > > - if (!Status) {
> > >
> > > + if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {
> > >
> > > goto Done;
> > >
> > > }
> > >
> > > - Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);
> > >
> > > - if (!Status) {
> > >
> > > + if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {
> > >
> > > goto Done;
> > >
> > > }
> > >
> > >
> > >
> > > @@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
> > > //
> > >
> > > // Hash of Certificate is found in forbidden database.
> > >
> > > //
> > >
> > > - IsFound = TRUE;
> > >
> > > + Status = EFI_SUCCESS;
> > >
> > > + *IsFound = TRUE;
> > >
> > >
> > >
> > > //
> > >
> > > // Return the revocation time.
> > >
> > > @@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
> > > DbxList = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList-
> > > >SignatureListSize);
> > >
> > > }
> > >
> > >
> > >
> > > + Status = EFI_SUCCESS;
> > >
> > > +
> > >
> > > Done:
> > >
> > > if (HashCtx != NULL) {
> > >
> > > FreePool (HashCtx);
> > >
> > > }
> > >
> > >
> > >
> > > - return IsFound;
> > >
> > > + return Status;
> > >
> > > }
> > >
> > >
> > >
> > > /**
> > >
> > > @@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
> > > {
> > >
> > > EFI_STATUS Status;
> > >
> > > BOOLEAN IsForbidden;
> > >
> > > + BOOLEAN IsFound;
> > >
> > > UINT8 *Data;
> > >
> > > UINTN DataSize;
> > >
> > > EFI_SIGNATURE_LIST *CertList;
> > >
> > > @@ -1344,12 +1347,13 @@ IsForbiddenByDbx (
> > > //
> > >
> > > CertPtr = CertPtr + sizeof (UINT32) + CertSize;
> > >
> > >
> > >
> > > - if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> > *)Data,
> > > DataSize, &RevocationTime)) {
> > >
> > > + Status = IsCertHashFoundInDatabase (Cert, CertSize,
> (EFI_SIGNATURE_LIST
> > > *)Data, DataSize, &RevocationTime, &IsFound);
> > >
> > > + if (EFI_ERROR (Status) || IsFound) {
> > >
> > > //
> > >
> > > // Check the timestamp signature and signing time to determine if the
> > image
> > > can be trusted.
> > >
> > > //
> > >
> > > IsForbidden = TRUE;
> > >
> > > - if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
> > >
> > > + if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData,
> AuthDataSize,
> > > &RevocationTime)) {
> > >
> > > IsForbidden = FALSE;
> > >
> > > //
> > >
> > > // Pass DBT check. Continue to check other certs in image signer's cert
> list
> > > against DBX, DBT
> > >
> > > @@ -1392,6 +1396,7 @@ IsAllowedByDb (
> > > {
> > >
> > > EFI_STATUS Status;
> > >
> > > BOOLEAN VerifyStatus;
> > >
> > > + BOOLEAN IsFound;
> > >
> > > EFI_SIGNATURE_LIST *CertList;
> > >
> > > EFI_SIGNATURE_DATA *CertData;
> > >
> > > UINTN DataSize;
> > >
> > > @@ -1495,17 +1500,26 @@ IsAllowedByDb (
> > > // The image is signed and its signature is found in 'db'.
> > >
> > > //
> > >
> > > if (DbxData != NULL) {
> > >
> > > + VerifyStatus = FALSE;
> > >
> > > //
> > >
> > > // Here We still need to check if this RootCert's Hash is revoked
> > >
> > > //
> > >
> > > - if (IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
> > >
> > > - //
> > >
> > > - // Check the timestamp signature and signing time to determine if
> the
> > > RootCert can be trusted.
> > >
> > > - //
> > >
> > > - VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > &RevocationTime);
> > >
> > > - if (!VerifyStatus) {
> > >
> > > - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed
> > and
> > > signature is accepted by DB, but its root cert failed the timestamp check.\n"));
> > >
> > > - }
> > >
> > > + Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime,
> &IsFound);
> > >
> > > + if (EFI_ERROR (Status)) {
> > >
> > > + goto Done;
> > >
> > > + }
> > >
> > > +
> > >
> > > + if (!IsFound) {
> > >
> > > + VerifyStatus = TRUE;
> > >
> > > + goto Done;
> > >
> > > + }
> > >
> > > +
> > >
> > > + //
> > >
> > > + // Check the timestamp signature and signing time to determine if
> the
> > > RootCert can be trusted.
> > >
> > > + //
> > >
> > > + VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > &RevocationTime);
> > >
> > > + if (!VerifyStatus) {
> > >
> > > + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed
> > and
> > > signature is accepted by DB, but its root cert failed the timestamp check.\n"));
> > >
> > > }
> > >
> > > }
> > >
> > >
> > >
> > > --
> > > 2.24.0.windows.2
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
2020-02-14 0:54 ` Yao, Jiewen
@ 2020-02-14 3:31 ` Wang, Jian J
2020-02-14 3:33 ` Yao, Jiewen
0 siblings, 1 reply; 27+ messages in thread
From: Wang, Jian J @ 2020-02-14 3:31 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io; +Cc: Zhang, Chao B, Laszlo Ersek
Jiewen,
> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Friday, February 14, 2020 8:54 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
>
> 1) I prefer we do a little bit simple clean up in this series. Just name change.
> Maybe as patch-10.
>
Sure. I'll add it in v2.
> 2) When PassTimestampCheck() need to be called? Only Dbx is found? Or even
> the Dbx is broken?
>
> I prefer we need use a consistent rule.
>
> Case 1 in original patch:
> if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> > > &RevocationTime)) {
>
> Case 2 in your email:
> > VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime);
> > if (!VerifyStatus) {
>
> It seems they are not consistent...
>
Just talked to Chao privately. He mentioned that RevocationTime might not
be valid if Status != EFI_SUCCESS. So we should only call PassTimestampCheck()
when Status == EFI_SUCCESS and IsFound == TRUE.
Here's my new proposal (for case 1, case 2 is similar).
Status = IsCertHashFoundInDatabase (...);
if (EFI_ERROR(Status)) {
//
// Error in searching dbx. Consider it as 'found'. RevocationTime might
// not be valid in such situation.
//
IsForbidden = TRUE;
} else if (IsFound) {
//
// Found Cert in dbx successfully. Check the timestamp signature and
// signing time to determine if the image can be trusted.
//
if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
IsForbidden = FALSE;
//
// Pass DBT check. Continue to check other certs in image signer's cert list against DBX, DBT
//
continue;
} else {
IsForbidden = TRUE;
DEBUG((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but signature failed the timestamp check.\n"));
goto Done;
}
}
If no objection, I'll include it in v2.
Regards,
Jian
> Thank you
> Yao Jiewen
>
>
> > -----Original Message-----
> > From: Wang, Jian J <jian.j.wang@intel.com>
> > Sent: Thursday, February 13, 2020 11:08 PM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>
> > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> >
> > Jiewen,
> >
> > Thanks for the comments.
> >
> > 1) You're right. IsCertHashFoundInDatabase is quite general and cause
> > confusions between
> > db and dbx situation. Since it's not newly introduced in this patch series, do you
> > think it's ok
> > to fix it in separate patch series later? Or do you prefer fix it in this patch series?
> > I'm ok with
> > both.
> >
> > 2) I checked both code again. I think you're right. Both callings are for dbx, any
> > error Status
> > should be taken as IsFound(==TRUE). What about following change for the
> > second case?
> > Please help double check if any logic hole here.
> >
> > Status = IsCertHashFoundInDatabase (...);
> > if (EFI_ERROR (Status) || IsFound) {
> > //
> > // Check the timestamp signature and signing time to determine if the
> > RootCert can be trusted.
> > //
> > VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > &RevocationTime);
> > if (!VerifyStatus) {
> > DEBUG ((...));
> > }
> > } else {
> > VerifyStatus = TRUE;
> > }
> >
> > goto Done;
> >
> > Regards,
> > Jian
> >
> > > -----Original Message-----
> > > From: Yao, Jiewen <jiewen.yao@intel.com>
> > > Sent: Thursday, February 13, 2020 6:11 PM
> > > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> > > <lersek@redhat.com>
> > > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> > >
> > > Comment below:
> > >
> > > 1) I think the function name - IsCertHashFoundInDatabase() and the
> > > implementation { DbxList = SignatureList; DbxSize = SignatureListSize; }
> bring
> > > some confusion to me.
> > >
> > > If this is a *generic* database search function, I recommend we use a
> generic
> > > name - not use DbxList/DbxSize in the function implementation.
> > >
> > > If the input SignatureList of the function must be *Dbx*, I recommend we
> use
> > > IsCertHashFoundInDbx() as the function name.
> > >
> > > Either change is OK for me.
> > >
> > > 2) Now we have to check 2 output: Status and IsFound in
> > > IsCertHashFoundInDatabase().
> > >
> > > I am struggling to understand the different between 2 different ways of error
> > > handling:
> > >
> > > ===========================
> > > Status = IsCertHashFoundInDatabase (Cert, CertSize,
> (EFI_SIGNATURE_LIST
> > > *)Data, DataSize, &RevocationTime, &IsFound);
> > > if (EFI_ERROR (Status) || IsFound) {
> > > //
> > > // Check the timestamp signature and signing time to determine if the
> > image
> > > can be trusted.
> > > //
> > > IsForbidden = TRUE;
> > > if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData,
> AuthDataSize,
> > > &RevocationTime)) {
> > > IsForbidden = FALSE;
> > > ============================
> > >
> > > and
> > >
> > > ============================
> > > VerifyStatus = FALSE;
> > > //
> > > // Here We still need to check if this RootCert's Hash is revoked
> > > //
> > > Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime,
> &IsFound);
> > > if (EFI_ERROR (Status)) {
> > > goto Done;
> > > }
> > >
> > > if (!IsFound) {
> > > VerifyStatus = TRUE;
> > > goto Done;
> > > }
> > >
> > > //
> > > // Check the timestamp signature and signing time to determine if the
> > > RootCert can be trusted.
> > > //
> > > VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > &RevocationTime);
> > > if (!VerifyStatus) {
> > > ===============================
> > >
> > > I *believe* the logic behind is same. If so, we can use a consistent way to
> > check
> > > the 2 output and decide if PassTimestampCheck() is required.
> > >
> > > Or, can we create a one single function to perform such check for both
> > > IsCertHashFoundInDatabase() and PassTimestampCheck() ?
> > >
> > > If I am wrong, there is *difference* between them. Then I think we need
> much
> > > better description to help reviewer to catch the difference.
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > > Sent: Thursday, February 6, 2020 10:20 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> > > > <chao.b.zhang@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > > > Subject: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > error
> > > > and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> > > >
> > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
> > > >
> > > > To avoid false-negative issue in check hash against dbx, both error
> > > > condition (as return value) and check result (as out parameter) of
> > > > IsCertHashFoundInDatabase() are added. So the caller of this function
> > > > will know exactly if a failure is caused by a black list hit or
> > > > other error happening, and enforce a more secure operation to prevent
> > > > secure boot from being bypassed. For a white list check (db), there's
> > > > no such necessity.
> > > >
> > > > 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>
> > > > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > > > ---
> > > > .../DxeImageVerificationLib.c | 68 +++++++++++--------
> > > > 1 file changed, 41 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git
> > > > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > > index 8739d1fa29..a5dfee0f8e 100644
> > > > ---
> > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > > +++
> > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > > @@ -822,22 +822,23 @@ AddImageExeInfo (
> > > > @param[in] SignatureList Pointer to the Signature List in forbidden
> > > database.
> > > >
> > > > @param[in] SignatureListSize Size of Signature List.
> > > >
> > > > @param[out] RevocationTime Return the time that the certificate was
> > > > revoked.
> > > >
> > > > + @param[out] IsFound Search result. Only valid if EFI_SUCCESS
> > > returned.
> > > >
> > > >
> > > >
> > > > - @return TRUE The certificate hash is found in the forbidden database.
> > > >
> > > > - @return FALSE The certificate hash is not found in the forbidden
> database.
> > > >
> > > > + @retval EFI_SUCCESS Finished the search without any error.
> > > >
> > > > + @retval Others Error occurred in the search of database.
> > > >
> > > >
> > > >
> > > > **/
> > > >
> > > > -BOOLEAN
> > > >
> > > > +EFI_STATUS
> > > >
> > > > IsCertHashFoundInDatabase (
> > > >
> > > > IN UINT8 *Certificate,
> > > >
> > > > IN UINTN CertSize,
> > > >
> > > > IN EFI_SIGNATURE_LIST *SignatureList,
> > > >
> > > > IN UINTN SignatureListSize,
> > > >
> > > > - OUT EFI_TIME *RevocationTime
> > > >
> > > > + OUT EFI_TIME *RevocationTime,
> > > >
> > > > + OUT BOOLEAN *IsFound
> > > >
> > > > )
> > > >
> > > > {
> > > >
> > > > - BOOLEAN IsFound;
> > > >
> > > > - BOOLEAN Status;
> > > >
> > > > + EFI_STATUS Status;
> > > >
> > > > EFI_SIGNATURE_LIST *DbxList;
> > > >
> > > > UINTN DbxSize;
> > > >
> > > > EFI_SIGNATURE_DATA *CertHash;
> > > >
> > > > @@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
> > > > UINT8 *TBSCert;
> > > >
> > > > UINTN TBSCertSize;
> > > >
> > > >
> > > >
> > > > - IsFound = FALSE;
> > > >
> > > > + Status = EFI_ABORTED;
> > > >
> > > > + *IsFound = FALSE;
> > > >
> > > > DbxList = SignatureList;
> > > >
> > > > DbxSize = SignatureListSize;
> > > >
> > > > HashCtx = NULL;
> > > >
> > > > HashAlg = HASHALG_MAX;
> > > >
> > > >
> > > >
> > > > if ((RevocationTime == NULL) || (DbxList == NULL)) {
> > > >
> > > > - return FALSE;
> > > >
> > > > + return EFI_INVALID_PARAMETER;
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > > //
> > > >
> > > > // Retrieve the TBSCertificate from the X.509 Certificate.
> > > >
> > > > //
> > > >
> > > > if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {
> > > >
> > > > - return FALSE;
> > > >
> > > > + return Status;
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > > while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize)) {
> > > >
> > > > @@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
> > > > if (HashCtx == NULL) {
> > > >
> > > > goto Done;
> > > >
> > > > }
> > > >
> > > > - Status = mHash[HashAlg].HashInit (HashCtx);
> > > >
> > > > - if (!Status) {
> > > >
> > > > + if (!mHash[HashAlg].HashInit (HashCtx)) {
> > > >
> > > > goto Done;
> > > >
> > > > }
> > > >
> > > > - Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);
> > > >
> > > > - if (!Status) {
> > > >
> > > > + if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {
> > > >
> > > > goto Done;
> > > >
> > > > }
> > > >
> > > > - Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);
> > > >
> > > > - if (!Status) {
> > > >
> > > > + if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {
> > > >
> > > > goto Done;
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > > @@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
> > > > //
> > > >
> > > > // Hash of Certificate is found in forbidden database.
> > > >
> > > > //
> > > >
> > > > - IsFound = TRUE;
> > > >
> > > > + Status = EFI_SUCCESS;
> > > >
> > > > + *IsFound = TRUE;
> > > >
> > > >
> > > >
> > > > //
> > > >
> > > > // Return the revocation time.
> > > >
> > > > @@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
> > > > DbxList = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList-
> > > > >SignatureListSize);
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > > + Status = EFI_SUCCESS;
> > > >
> > > > +
> > > >
> > > > Done:
> > > >
> > > > if (HashCtx != NULL) {
> > > >
> > > > FreePool (HashCtx);
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > > - return IsFound;
> > > >
> > > > + return Status;
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > > /**
> > > >
> > > > @@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
> > > > {
> > > >
> > > > EFI_STATUS Status;
> > > >
> > > > BOOLEAN IsForbidden;
> > > >
> > > > + BOOLEAN IsFound;
> > > >
> > > > UINT8 *Data;
> > > >
> > > > UINTN DataSize;
> > > >
> > > > EFI_SIGNATURE_LIST *CertList;
> > > >
> > > > @@ -1344,12 +1347,13 @@ IsForbiddenByDbx (
> > > > //
> > > >
> > > > CertPtr = CertPtr + sizeof (UINT32) + CertSize;
> > > >
> > > >
> > > >
> > > > - if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> > > *)Data,
> > > > DataSize, &RevocationTime)) {
> > > >
> > > > + Status = IsCertHashFoundInDatabase (Cert, CertSize,
> > (EFI_SIGNATURE_LIST
> > > > *)Data, DataSize, &RevocationTime, &IsFound);
> > > >
> > > > + if (EFI_ERROR (Status) || IsFound) {
> > > >
> > > > //
> > > >
> > > > // Check the timestamp signature and signing time to determine if the
> > > image
> > > > can be trusted.
> > > >
> > > > //
> > > >
> > > > IsForbidden = TRUE;
> > > >
> > > > - if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime))
> {
> > > >
> > > > + if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData,
> > AuthDataSize,
> > > > &RevocationTime)) {
> > > >
> > > > IsForbidden = FALSE;
> > > >
> > > > //
> > > >
> > > > // Pass DBT check. Continue to check other certs in image signer's cert
> > list
> > > > against DBX, DBT
> > > >
> > > > @@ -1392,6 +1396,7 @@ IsAllowedByDb (
> > > > {
> > > >
> > > > EFI_STATUS Status;
> > > >
> > > > BOOLEAN VerifyStatus;
> > > >
> > > > + BOOLEAN IsFound;
> > > >
> > > > EFI_SIGNATURE_LIST *CertList;
> > > >
> > > > EFI_SIGNATURE_DATA *CertData;
> > > >
> > > > UINTN DataSize;
> > > >
> > > > @@ -1495,17 +1500,26 @@ IsAllowedByDb (
> > > > // The image is signed and its signature is found in 'db'.
> > > >
> > > > //
> > > >
> > > > if (DbxData != NULL) {
> > > >
> > > > + VerifyStatus = FALSE;
> > > >
> > > > //
> > > >
> > > > // Here We still need to check if this RootCert's Hash is revoked
> > > >
> > > > //
> > > >
> > > > - if (IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
> > > >
> > > > - //
> > > >
> > > > - // Check the timestamp signature and signing time to determine if
> > the
> > > > RootCert can be trusted.
> > > >
> > > > - //
> > > >
> > > > - VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > > &RevocationTime);
> > > >
> > > > - if (!VerifyStatus) {
> > > >
> > > > - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> signed
> > > and
> > > > signature is accepted by DB, but its root cert failed the timestamp
> check.\n"));
> > > >
> > > > - }
> > > >
> > > > + Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime,
> > &IsFound);
> > > >
> > > > + if (EFI_ERROR (Status)) {
> > > >
> > > > + goto Done;
> > > >
> > > > + }
> > > >
> > > > +
> > > >
> > > > + if (!IsFound) {
> > > >
> > > > + VerifyStatus = TRUE;
> > > >
> > > > + goto Done;
> > > >
> > > > + }
> > > >
> > > > +
> > > >
> > > > + //
> > > >
> > > > + // Check the timestamp signature and signing time to determine if
> > the
> > > > RootCert can be trusted.
> > > >
> > > > + //
> > > >
> > > > + VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > > &RevocationTime);
> > > >
> > > > + if (!VerifyStatus) {
> > > >
> > > > + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed
> > > and
> > > > signature is accepted by DB, but its root cert failed the timestamp
> check.\n"));
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > > --
> > > > 2.24.0.windows.2
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
2020-02-14 3:31 ` Wang, Jian J
@ 2020-02-14 3:33 ` Yao, Jiewen
0 siblings, 0 replies; 27+ messages in thread
From: Yao, Jiewen @ 2020-02-14 3:33 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io; +Cc: Zhang, Chao B, Laszlo Ersek
Fine. Thanks for the update.
> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Friday, February 14, 2020 11:32 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
>
> Jiewen,
>
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Friday, February 14, 2020 8:54 AM
> > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>
> > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> >
> > 1) I prefer we do a little bit simple clean up in this series. Just name change.
> > Maybe as patch-10.
> >
>
> Sure. I'll add it in v2.
>
> > 2) When PassTimestampCheck() need to be called? Only Dbx is found? Or even
> > the Dbx is broken?
> >
> > I prefer we need use a consistent rule.
> >
> > Case 1 in original patch:
> > if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData, AuthDataSize,
> > > > &RevocationTime)) {
> >
> > Case 2 in your email:
> > > VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > &RevocationTime);
> > > if (!VerifyStatus) {
> >
> > It seems they are not consistent...
> >
>
> Just talked to Chao privately. He mentioned that RevocationTime might not
> be valid if Status != EFI_SUCCESS. So we should only call PassTimestampCheck()
> when Status == EFI_SUCCESS and IsFound == TRUE.
>
> Here's my new proposal (for case 1, case 2 is similar).
>
> Status = IsCertHashFoundInDatabase (...);
> if (EFI_ERROR(Status)) {
> //
> // Error in searching dbx. Consider it as 'found'. RevocationTime might
> // not be valid in such situation.
> //
> IsForbidden = TRUE;
> } else if (IsFound) {
> //
> // Found Cert in dbx successfully. Check the timestamp signature and
> // signing time to determine if the image can be trusted.
> //
> if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime)) {
> IsForbidden = FALSE;
> //
> // Pass DBT check. Continue to check other certs in image signer's cert list
> against DBX, DBT
> //
> continue;
> } else {
> IsForbidden = TRUE;
> DEBUG((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but
> signature failed the timestamp check.\n"));
> goto Done;
> }
> }
>
> If no objection, I'll include it in v2.
>
> Regards,
> Jian
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > Sent: Thursday, February 13, 2020 11:08 PM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> > > <lersek@redhat.com>
> > > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> > >
> > > Jiewen,
> > >
> > > Thanks for the comments.
> > >
> > > 1) You're right. IsCertHashFoundInDatabase is quite general and cause
> > > confusions between
> > > db and dbx situation. Since it's not newly introduced in this patch series, do
> you
> > > think it's ok
> > > to fix it in separate patch series later? Or do you prefer fix it in this patch
> series?
> > > I'm ok with
> > > both.
> > >
> > > 2) I checked both code again. I think you're right. Both callings are for dbx,
> any
> > > error Status
> > > should be taken as IsFound(==TRUE). What about following change for the
> > > second case?
> > > Please help double check if any logic hole here.
> > >
> > > Status = IsCertHashFoundInDatabase (...);
> > > if (EFI_ERROR (Status) || IsFound) {
> > > //
> > > // Check the timestamp signature and signing time to determine if
> the
> > > RootCert can be trusted.
> > > //
> > > VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > &RevocationTime);
> > > if (!VerifyStatus) {
> > > DEBUG ((...));
> > > }
> > > } else {
> > > VerifyStatus = TRUE;
> > > }
> > >
> > > goto Done;
> > >
> > > Regards,
> > > Jian
> > >
> > > > -----Original Message-----
> > > > From: Yao, Jiewen <jiewen.yao@intel.com>
> > > > Sent: Thursday, February 13, 2020 6:11 PM
> > > > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > > > Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Laszlo Ersek
> > > > <lersek@redhat.com>
> > > > Subject: RE: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib:
> Differentiate
> > > > error and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> > > >
> > > > Comment below:
> > > >
> > > > 1) I think the function name - IsCertHashFoundInDatabase() and the
> > > > implementation { DbxList = SignatureList; DbxSize = SignatureListSize; }
> > bring
> > > > some confusion to me.
> > > >
> > > > If this is a *generic* database search function, I recommend we use a
> > generic
> > > > name - not use DbxList/DbxSize in the function implementation.
> > > >
> > > > If the input SignatureList of the function must be *Dbx*, I recommend we
> > use
> > > > IsCertHashFoundInDbx() as the function name.
> > > >
> > > > Either change is OK for me.
> > > >
> > > > 2) Now we have to check 2 output: Status and IsFound in
> > > > IsCertHashFoundInDatabase().
> > > >
> > > > I am struggling to understand the different between 2 different ways of
> error
> > > > handling:
> > > >
> > > > ===========================
> > > > Status = IsCertHashFoundInDatabase (Cert, CertSize,
> > (EFI_SIGNATURE_LIST
> > > > *)Data, DataSize, &RevocationTime, &IsFound);
> > > > if (EFI_ERROR (Status) || IsFound) {
> > > > //
> > > > // Check the timestamp signature and signing time to determine if the
> > > image
> > > > can be trusted.
> > > > //
> > > > IsForbidden = TRUE;
> > > > if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData,
> > AuthDataSize,
> > > > &RevocationTime)) {
> > > > IsForbidden = FALSE;
> > > > ============================
> > > >
> > > > and
> > > >
> > > > ============================
> > > > VerifyStatus = FALSE;
> > > > //
> > > > // Here We still need to check if this RootCert's Hash is revoked
> > > > //
> > > > Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime,
> > &IsFound);
> > > > if (EFI_ERROR (Status)) {
> > > > goto Done;
> > > > }
> > > >
> > > > if (!IsFound) {
> > > > VerifyStatus = TRUE;
> > > > goto Done;
> > > > }
> > > >
> > > > //
> > > > // Check the timestamp signature and signing time to determine if
> the
> > > > RootCert can be trusted.
> > > > //
> > > > VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > > &RevocationTime);
> > > > if (!VerifyStatus) {
> > > > ===============================
> > > >
> > > > I *believe* the logic behind is same. If so, we can use a consistent way to
> > > check
> > > > the 2 output and decide if PassTimestampCheck() is required.
> > > >
> > > > Or, can we create a one single function to perform such check for both
> > > > IsCertHashFoundInDatabase() and PassTimestampCheck() ?
> > > >
> > > > If I am wrong, there is *difference* between them. Then I think we need
> > much
> > > > better description to help reviewer to catch the difference.
> > > >
> > > > Thank you
> > > > Yao Jiewen
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Jian J <jian.j.wang@intel.com>
> > > > > Sent: Thursday, February 6, 2020 10:20 PM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> > > > > <chao.b.zhang@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > > > > Subject: [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate
> > > error
> > > > > and search result in IsCertHashFoundInDatabase(CVE-2019-14575)
> > > > >
> > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
> > > > >
> > > > > To avoid false-negative issue in check hash against dbx, both error
> > > > > condition (as return value) and check result (as out parameter) of
> > > > > IsCertHashFoundInDatabase() are added. So the caller of this function
> > > > > will know exactly if a failure is caused by a black list hit or
> > > > > other error happening, and enforce a more secure operation to prevent
> > > > > secure boot from being bypassed. For a white list check (db), there's
> > > > > no such necessity.
> > > > >
> > > > > 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>
> > > > > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > > > > ---
> > > > > .../DxeImageVerificationLib.c | 68 +++++++++++--------
> > > > > 1 file changed, 41 insertions(+), 27 deletions(-)
> > > > >
> > > > > diff --git
> > > > >
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > > >
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > > > index 8739d1fa29..a5dfee0f8e 100644
> > > > > ---
> > > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > > > +++
> > > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> > > > > @@ -822,22 +822,23 @@ AddImageExeInfo (
> > > > > @param[in] SignatureList Pointer to the Signature List in forbidden
> > > > database.
> > > > >
> > > > > @param[in] SignatureListSize Size of Signature List.
> > > > >
> > > > > @param[out] RevocationTime Return the time that the certificate
> was
> > > > > revoked.
> > > > >
> > > > > + @param[out] IsFound Search result. Only valid if EFI_SUCCESS
> > > > returned.
> > > > >
> > > > >
> > > > >
> > > > > - @return TRUE The certificate hash is found in the forbidden database.
> > > > >
> > > > > - @return FALSE The certificate hash is not found in the forbidden
> > database.
> > > > >
> > > > > + @retval EFI_SUCCESS Finished the search without any error.
> > > > >
> > > > > + @retval Others Error occurred in the search of database.
> > > > >
> > > > >
> > > > >
> > > > > **/
> > > > >
> > > > > -BOOLEAN
> > > > >
> > > > > +EFI_STATUS
> > > > >
> > > > > IsCertHashFoundInDatabase (
> > > > >
> > > > > IN UINT8 *Certificate,
> > > > >
> > > > > IN UINTN CertSize,
> > > > >
> > > > > IN EFI_SIGNATURE_LIST *SignatureList,
> > > > >
> > > > > IN UINTN SignatureListSize,
> > > > >
> > > > > - OUT EFI_TIME *RevocationTime
> > > > >
> > > > > + OUT EFI_TIME *RevocationTime,
> > > > >
> > > > > + OUT BOOLEAN *IsFound
> > > > >
> > > > > )
> > > > >
> > > > > {
> > > > >
> > > > > - BOOLEAN IsFound;
> > > > >
> > > > > - BOOLEAN Status;
> > > > >
> > > > > + EFI_STATUS Status;
> > > > >
> > > > > EFI_SIGNATURE_LIST *DbxList;
> > > > >
> > > > > UINTN DbxSize;
> > > > >
> > > > > EFI_SIGNATURE_DATA *CertHash;
> > > > >
> > > > > @@ -851,21 +852,22 @@ IsCertHashFoundInDatabase (
> > > > > UINT8 *TBSCert;
> > > > >
> > > > > UINTN TBSCertSize;
> > > > >
> > > > >
> > > > >
> > > > > - IsFound = FALSE;
> > > > >
> > > > > + Status = EFI_ABORTED;
> > > > >
> > > > > + *IsFound = FALSE;
> > > > >
> > > > > DbxList = SignatureList;
> > > > >
> > > > > DbxSize = SignatureListSize;
> > > > >
> > > > > HashCtx = NULL;
> > > > >
> > > > > HashAlg = HASHALG_MAX;
> > > > >
> > > > >
> > > > >
> > > > > if ((RevocationTime == NULL) || (DbxList == NULL)) {
> > > > >
> > > > > - return FALSE;
> > > > >
> > > > > + return EFI_INVALID_PARAMETER;
> > > > >
> > > > > }
> > > > >
> > > > >
> > > > >
> > > > > //
> > > > >
> > > > > // Retrieve the TBSCertificate from the X.509 Certificate.
> > > > >
> > > > > //
> > > > >
> > > > > if (!X509GetTBSCert (Certificate, CertSize, &TBSCert, &TBSCertSize)) {
> > > > >
> > > > > - return FALSE;
> > > > >
> > > > > + return Status;
> > > > >
> > > > > }
> > > > >
> > > > >
> > > > >
> > > > > while ((DbxSize > 0) && (SignatureListSize >= DbxList->SignatureListSize))
> {
> > > > >
> > > > > @@ -895,16 +897,13 @@ IsCertHashFoundInDatabase (
> > > > > if (HashCtx == NULL) {
> > > > >
> > > > > goto Done;
> > > > >
> > > > > }
> > > > >
> > > > > - Status = mHash[HashAlg].HashInit (HashCtx);
> > > > >
> > > > > - if (!Status) {
> > > > >
> > > > > + if (!mHash[HashAlg].HashInit (HashCtx)) {
> > > > >
> > > > > goto Done;
> > > > >
> > > > > }
> > > > >
> > > > > - Status = mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize);
> > > > >
> > > > > - if (!Status) {
> > > > >
> > > > > + if (!mHash[HashAlg].HashUpdate (HashCtx, TBSCert, TBSCertSize)) {
> > > > >
> > > > > goto Done;
> > > > >
> > > > > }
> > > > >
> > > > > - Status = mHash[HashAlg].HashFinal (HashCtx, CertDigest);
> > > > >
> > > > > - if (!Status) {
> > > > >
> > > > > + if (!mHash[HashAlg].HashFinal (HashCtx, CertDigest)) {
> > > > >
> > > > > goto Done;
> > > > >
> > > > > }
> > > > >
> > > > >
> > > > >
> > > > > @@ -923,7 +922,8 @@ IsCertHashFoundInDatabase (
> > > > > //
> > > > >
> > > > > // Hash of Certificate is found in forbidden database.
> > > > >
> > > > > //
> > > > >
> > > > > - IsFound = TRUE;
> > > > >
> > > > > + Status = EFI_SUCCESS;
> > > > >
> > > > > + *IsFound = TRUE;
> > > > >
> > > > >
> > > > >
> > > > > //
> > > > >
> > > > > // Return the revocation time.
> > > > >
> > > > > @@ -938,12 +938,14 @@ IsCertHashFoundInDatabase (
> > > > > DbxList = (EFI_SIGNATURE_LIST *) ((UINT8 *) DbxList + DbxList-
> > > > > >SignatureListSize);
> > > > >
> > > > > }
> > > > >
> > > > >
> > > > >
> > > > > + Status = EFI_SUCCESS;
> > > > >
> > > > > +
> > > > >
> > > > > Done:
> > > > >
> > > > > if (HashCtx != NULL) {
> > > > >
> > > > > FreePool (HashCtx);
> > > > >
> > > > > }
> > > > >
> > > > >
> > > > >
> > > > > - return IsFound;
> > > > >
> > > > > + return Status;
> > > > >
> > > > > }
> > > > >
> > > > >
> > > > >
> > > > > /**
> > > > >
> > > > > @@ -1216,6 +1218,7 @@ IsForbiddenByDbx (
> > > > > {
> > > > >
> > > > > EFI_STATUS Status;
> > > > >
> > > > > BOOLEAN IsForbidden;
> > > > >
> > > > > + BOOLEAN IsFound;
> > > > >
> > > > > UINT8 *Data;
> > > > >
> > > > > UINTN DataSize;
> > > > >
> > > > > EFI_SIGNATURE_LIST *CertList;
> > > > >
> > > > > @@ -1344,12 +1347,13 @@ IsForbiddenByDbx (
> > > > > //
> > > > >
> > > > > CertPtr = CertPtr + sizeof (UINT32) + CertSize;
> > > > >
> > > > >
> > > > >
> > > > > - if (IsCertHashFoundInDatabase (Cert, CertSize, (EFI_SIGNATURE_LIST
> > > > *)Data,
> > > > > DataSize, &RevocationTime)) {
> > > > >
> > > > > + Status = IsCertHashFoundInDatabase (Cert, CertSize,
> > > (EFI_SIGNATURE_LIST
> > > > > *)Data, DataSize, &RevocationTime, &IsFound);
> > > > >
> > > > > + if (EFI_ERROR (Status) || IsFound) {
> > > > >
> > > > > //
> > > > >
> > > > > // Check the timestamp signature and signing time to determine if the
> > > > image
> > > > > can be trusted.
> > > > >
> > > > > //
> > > > >
> > > > > IsForbidden = TRUE;
> > > > >
> > > > > - if (PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime))
> > {
> > > > >
> > > > > + if (!EFI_ERROR (Status) && PassTimestampCheck (AuthData,
> > > AuthDataSize,
> > > > > &RevocationTime)) {
> > > > >
> > > > > IsForbidden = FALSE;
> > > > >
> > > > > //
> > > > >
> > > > > // Pass DBT check. Continue to check other certs in image signer's
> cert
> > > list
> > > > > against DBX, DBT
> > > > >
> > > > > @@ -1392,6 +1396,7 @@ IsAllowedByDb (
> > > > > {
> > > > >
> > > > > EFI_STATUS Status;
> > > > >
> > > > > BOOLEAN VerifyStatus;
> > > > >
> > > > > + BOOLEAN IsFound;
> > > > >
> > > > > EFI_SIGNATURE_LIST *CertList;
> > > > >
> > > > > EFI_SIGNATURE_DATA *CertData;
> > > > >
> > > > > UINTN DataSize;
> > > > >
> > > > > @@ -1495,17 +1500,26 @@ IsAllowedByDb (
> > > > > // The image is signed and its signature is found in 'db'.
> > > > >
> > > > > //
> > > > >
> > > > > if (DbxData != NULL) {
> > > > >
> > > > > + VerifyStatus = FALSE;
> > > > >
> > > > > //
> > > > >
> > > > > // Here We still need to check if this RootCert's Hash is revoked
> > > > >
> > > > > //
> > > > >
> > > > > - if (IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
> > > > >
> > > > > - //
> > > > >
> > > > > - // Check the timestamp signature and signing time to determine
> if
> > > the
> > > > > RootCert can be trusted.
> > > > >
> > > > > - //
> > > > >
> > > > > - VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > > > &RevocationTime);
> > > > >
> > > > > - if (!VerifyStatus) {
> > > > >
> > > > > - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> > signed
> > > > and
> > > > > signature is accepted by DB, but its root cert failed the timestamp
> > check.\n"));
> > > > >
> > > > > - }
> > > > >
> > > > > + Status = IsCertHashFoundInDatabase (RootCert, RootCertSize,
> > > > > (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime,
> > > &IsFound);
> > > > >
> > > > > + if (EFI_ERROR (Status)) {
> > > > >
> > > > > + goto Done;
> > > > >
> > > > > + }
> > > > >
> > > > > +
> > > > >
> > > > > + if (!IsFound) {
> > > > >
> > > > > + VerifyStatus = TRUE;
> > > > >
> > > > > + goto Done;
> > > > >
> > > > > + }
> > > > >
> > > > > +
> > > > >
> > > > > + //
> > > > >
> > > > > + // Check the timestamp signature and signing time to determine
> if
> > > the
> > > > > RootCert can be trusted.
> > > > >
> > > > > + //
> > > > >
> > > > > + VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize,
> > > > > &RevocationTime);
> > > > >
> > > > > + if (!VerifyStatus) {
> > > > >
> > > > > + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> signed
> > > > and
> > > > > signature is accepted by DB, but its root cert failed the timestamp
> > check.\n"));
> > > > >
> > > > > }
> > > > >
> > > > > }
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > 2.24.0.windows.2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 7/9] SecurityPkg/DxeImageVerificationLib: tighten default result of IsForbiddenByDbx()(CVE-2019-14575)
2020-02-06 14:19 [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Wang, Jian J
` (5 preceding siblings ...)
2020-02-06 14:19 ` [PATCH 6/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsCertHashFoundInDatabase(CVE-2019-14575) Wang, Jian J
@ 2020-02-06 14:19 ` Wang, Jian J
2020-02-13 10:13 ` Yao, Jiewen
2020-02-06 14:19 ` [PATCH 8/9] SecurityPkg/DxeImageVerificationLib: plug Data leak in IsForbiddenByDbx()(CVE-2019-14575) Wang, Jian J
` (2 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Wang, Jian J @ 2020-02-06 14:19 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Chao Zhang, Laszlo Ersek
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
All intermediate results inside this function will be checked and
returned immediately upon any failure or error, like out-of-resource,
hash calculation error or certificate retrieval failure.
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>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.../DxeImageVerificationLib/DxeImageVerificationLib.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index a5dfee0f8e..2236ce98ce 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1240,7 +1240,7 @@ IsForbiddenByDbx (
//
// Variable Initialization
//
- IsForbidden = FALSE;
+ IsForbidden = TRUE;
Data = NULL;
CertList = NULL;
CertData = NULL;
@@ -1257,7 +1257,14 @@ IsForbiddenByDbx (
//
DataSize = 0;
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, NULL);
+ ASSERT (EFI_ERROR (Status));
if (Status != EFI_BUFFER_TOO_SMALL) {
+ if (Status == EFI_NOT_FOUND) {
+ //
+ // Evidently not in dbx if the database doesn't exist.
+ //
+ IsForbidden = FALSE;
+ }
return IsForbidden;
}
Data = (UINT8 *) AllocateZeroPool (DataSize);
@@ -1366,6 +1373,8 @@ IsForbiddenByDbx (
}
+ IsForbidden = FALSE;
+
Done:
if (Data != NULL) {
FreePool (Data);
--
2.24.0.windows.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 7/9] SecurityPkg/DxeImageVerificationLib: tighten default result of IsForbiddenByDbx()(CVE-2019-14575)
2020-02-06 14:19 ` [PATCH 7/9] SecurityPkg/DxeImageVerificationLib: tighten default result of IsForbiddenByDbx()(CVE-2019-14575) Wang, Jian J
@ 2020-02-13 10:13 ` Yao, Jiewen
0 siblings, 0 replies; 27+ messages in thread
From: Yao, Jiewen @ 2020-02-13 10:13 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io; +Cc: Zhang, Chao B, Laszlo Ersek
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:20 PM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 7/9] SecurityPkg/DxeImageVerificationLib: tighten default
> result of IsForbiddenByDbx()(CVE-2019-14575)
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
>
> All intermediate results inside this function will be checked and
> returned immediately upon any failure or error, like out-of-resource,
> hash calculation error or certificate retrieval failure.
>
> 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>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> .../DxeImageVerificationLib/DxeImageVerificationLib.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index a5dfee0f8e..2236ce98ce 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1240,7 +1240,7 @@ IsForbiddenByDbx (
> //
>
> // Variable Initialization
>
> //
>
> - IsForbidden = FALSE;
>
> + IsForbidden = TRUE;
>
> Data = NULL;
>
> CertList = NULL;
>
> CertData = NULL;
>
> @@ -1257,7 +1257,14 @@ IsForbiddenByDbx (
> //
>
> DataSize = 0;
>
> Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1,
> &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, NULL);
>
> + ASSERT (EFI_ERROR (Status));
>
> if (Status != EFI_BUFFER_TOO_SMALL) {
>
> + if (Status == EFI_NOT_FOUND) {
>
> + //
>
> + // Evidently not in dbx if the database doesn't exist.
>
> + //
>
> + IsForbidden = FALSE;
>
> + }
>
> return IsForbidden;
>
> }
>
> Data = (UINT8 *) AllocateZeroPool (DataSize);
>
> @@ -1366,6 +1373,8 @@ IsForbiddenByDbx (
>
>
> }
>
>
>
> + IsForbidden = FALSE;
>
> +
>
> Done:
>
> if (Data != NULL) {
>
> FreePool (Data);
>
> --
> 2.24.0.windows.2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 8/9] SecurityPkg/DxeImageVerificationLib: plug Data leak in IsForbiddenByDbx()(CVE-2019-14575)
2020-02-06 14:19 [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Wang, Jian J
` (6 preceding siblings ...)
2020-02-06 14:19 ` [PATCH 7/9] SecurityPkg/DxeImageVerificationLib: tighten default result of IsForbiddenByDbx()(CVE-2019-14575) Wang, Jian J
@ 2020-02-06 14:19 ` Wang, Jian J
2020-02-13 10:14 ` Yao, Jiewen
2020-02-13 16:56 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-02-06 14:19 ` [PATCH 9/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsSignatureFoundInDatabase(CVE-2019-14575) Wang, Jian J
2020-02-13 1:53 ` [edk2-devel] [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Liming Gao
9 siblings, 2 replies; 27+ messages in thread
From: Wang, Jian J @ 2020-02-06 14:19 UTC (permalink / raw)
To: devel; +Cc: Laszlo Ersek, Jiewen Yao, Chao Zhang
From: Laszlo Ersek <lersek@redhat.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
If the second GetVariable() call for "dbx" fails, in IsForbiddenByDbx(),
we have to free Data. Jump to "Done" for that.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.../Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 2236ce98ce..5b7a67f811 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1274,7 +1274,7 @@ IsForbiddenByDbx (
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, (VOID *) Data);
if (EFI_ERROR (Status)) {
- return IsForbidden;
+ goto Done;
}
//
--
2.24.0.windows.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 8/9] SecurityPkg/DxeImageVerificationLib: plug Data leak in IsForbiddenByDbx()(CVE-2019-14575)
2020-02-06 14:19 ` [PATCH 8/9] SecurityPkg/DxeImageVerificationLib: plug Data leak in IsForbiddenByDbx()(CVE-2019-14575) Wang, Jian J
@ 2020-02-13 10:14 ` Yao, Jiewen
2020-02-13 16:56 ` [edk2-devel] " Philippe Mathieu-Daudé
1 sibling, 0 replies; 27+ messages in thread
From: Yao, Jiewen @ 2020-02-13 10:14 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io; +Cc: Laszlo Ersek, Zhang, Chao B
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:20 PM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Zhang, Chao B <chao.b.zhang@intel.com>
> Subject: [PATCH 8/9] SecurityPkg/DxeImageVerificationLib: plug Data leak in
> IsForbiddenByDbx()(CVE-2019-14575)
>
> From: Laszlo Ersek <lersek@redhat.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
>
> If the second GetVariable() call for "dbx" fails, in IsForbiddenByDbx(),
> we have to free Data. Jump to "Done" for that.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> .../Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 2236ce98ce..5b7a67f811 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1274,7 +1274,7 @@ IsForbiddenByDbx (
>
>
> Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1,
> &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, (VOID *) Data);
>
> if (EFI_ERROR (Status)) {
>
> - return IsForbidden;
>
> + goto Done;
>
> }
>
>
>
> //
>
> --
> 2.24.0.windows.2
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [edk2-devel] [PATCH 8/9] SecurityPkg/DxeImageVerificationLib: plug Data leak in IsForbiddenByDbx()(CVE-2019-14575)
2020-02-06 14:19 ` [PATCH 8/9] SecurityPkg/DxeImageVerificationLib: plug Data leak in IsForbiddenByDbx()(CVE-2019-14575) Wang, Jian J
2020-02-13 10:14 ` Yao, Jiewen
@ 2020-02-13 16:56 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 27+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-13 16:56 UTC (permalink / raw)
To: devel, jian.j.wang; +Cc: Laszlo Ersek, Jiewen Yao, Chao Zhang
On 2/6/20 3:19 PM, Wang, Jian J wrote:
> From: Laszlo Ersek <lersek@redhat.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
What a painful review...
> If the second GetVariable() call for "dbx" fails, in IsForbiddenByDbx(),
> we have to free Data. Jump to "Done" for that.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> .../Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 2236ce98ce..5b7a67f811 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1274,7 +1274,7 @@ IsForbiddenByDbx (
>
> Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, (VOID *) Data);
> if (EFI_ERROR (Status)) {
> - return IsForbidden;
> + goto Done;
> }
>
> //
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 9/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsSignatureFoundInDatabase(CVE-2019-14575)
2020-02-06 14:19 [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Wang, Jian J
` (7 preceding siblings ...)
2020-02-06 14:19 ` [PATCH 8/9] SecurityPkg/DxeImageVerificationLib: plug Data leak in IsForbiddenByDbx()(CVE-2019-14575) Wang, Jian J
@ 2020-02-06 14:19 ` Wang, Jian J
2020-02-13 9:02 ` [edk2-devel] " Zhang, Chao B
2020-02-13 10:20 ` Yao, Jiewen
2020-02-13 1:53 ` [edk2-devel] [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Liming Gao
9 siblings, 2 replies; 27+ messages in thread
From: Wang, Jian J @ 2020-02-06 14:19 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
To avoid false-negative issue in check hash against dbx, both error
condition (as return value) and check result (as out parameter) of
IsSignatureFoundInDatabase() are added. So the caller of this function
will know exactly if a failure is caused by a black list hit or
other error happening, and enforce a more secure operation to prevent
secure boot from being bypassed. For a white list check (db), there's
no such necessity.
All intermediate results inside this function will be checked and
returned immediately upon any failure or error, like out-of-resource,
hash calculation error or certificate retrieval failure.
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>
---
.../DxeImageVerificationLib.c | 77 ++++++++++++++-----
1 file changed, 58 insertions(+), 19 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 5b7a67f811..8e599ca0be 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -955,17 +955,19 @@ Done:
@param[in] Signature Pointer to signature that is searched for.
@param[in] CertType Pointer to hash algorithm.
@param[in] SignatureSize Size of Signature.
+ @param[out] IsFound Search result. Only valid if EFI_SUCCESS returned
- @return TRUE Found the signature in the variable database.
- @return FALSE Not found the signature in the variable database.
+ @retval EFI_SUCCESS Finished the search without any error.
+ @retval Others Error occurred in the search of database.
**/
-BOOLEAN
+EFI_STATUS
IsSignatureFoundInDatabase (
- IN CHAR16 *VariableName,
- IN UINT8 *Signature,
- IN EFI_GUID *CertType,
- IN UINTN SignatureSize
+ IN CHAR16 *VariableName,
+ IN UINT8 *Signature,
+ IN EFI_GUID *CertType,
+ IN UINTN SignatureSize,
+ OUT BOOLEAN *IsFound
)
{
EFI_STATUS Status;
@@ -975,22 +977,28 @@ IsSignatureFoundInDatabase (
UINT8 *Data;
UINTN Index;
UINTN CertCount;
- BOOLEAN IsFound;
//
// Read signature database variable.
//
- IsFound = FALSE;
+ *IsFound = FALSE;
Data = NULL;
DataSize = 0;
Status = gRT->GetVariable (VariableName, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, NULL);
if (Status != EFI_BUFFER_TOO_SMALL) {
- return FALSE;
+ if (Status == EFI_NOT_FOUND) {
+ //
+ // No database, no need to search.
+ //
+ Status = EFI_SUCCESS;
+ }
+
+ return Status;
}
Data = (UINT8 *) AllocateZeroPool (DataSize);
if (Data == NULL) {
- return FALSE;
+ return EFI_OUT_OF_RESOURCES;
}
Status = gRT->GetVariable (VariableName, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, Data);
@@ -1010,7 +1018,7 @@ IsSignatureFoundInDatabase (
//
// Find the signature in database.
//
- IsFound = TRUE;
+ *IsFound = TRUE;
//
// Entries in UEFI_IMAGE_SECURITY_DATABASE that are used to validate image should be measured
//
@@ -1023,7 +1031,7 @@ IsSignatureFoundInDatabase (
Cert = (EFI_SIGNATURE_DATA *) ((UINT8 *) Cert + CertList->SignatureSize);
}
- if (IsFound) {
+ if (*IsFound) {
break;
}
}
@@ -1037,7 +1045,7 @@ Done:
FreePool (Data);
}
- return IsFound;
+ return Status;
}
/**
@@ -1642,6 +1650,8 @@ DxeImageVerificationHandler (
CHAR16 *NameStr;
RETURN_STATUS PeCoffStatus;
EFI_STATUS HashStatus;
+ EFI_STATUS DbStatus;
+ BOOLEAN IsFound;
SignatureList = NULL;
SignatureListSize = 0;
@@ -1650,7 +1660,7 @@ DxeImageVerificationHandler (
PkcsCertData = NULL;
Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
IsVerified = FALSE;
-
+ IsFound = FALSE;
//
// Check the image type and get policy setting.
@@ -1792,7 +1802,14 @@ DxeImageVerificationHandler (
goto Failed;
}
- if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1, mImageDigest, &mCertType, mImageDigestSize)) {
+ DbStatus = IsSignatureFoundInDatabase (
+ EFI_IMAGE_SECURITY_DATABASE1,
+ mImageDigest,
+ &mCertType,
+ mImageDigestSize,
+ &IsFound
+ );
+ if (EFI_ERROR (DbStatus) || IsFound) {
//
// Image Hash is in forbidden database (DBX).
//
@@ -1800,7 +1817,14 @@ DxeImageVerificationHandler (
goto Failed;
}
- if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE, mImageDigest, &mCertType, mImageDigestSize)) {
+ DbStatus = IsSignatureFoundInDatabase (
+ EFI_IMAGE_SECURITY_DATABASE,
+ mImageDigest,
+ &mCertType,
+ mImageDigestSize,
+ &IsFound
+ );
+ if (!EFI_ERROR (DbStatus) && IsFound) {
//
// Image Hash is in allowed database (DB).
//
@@ -1888,14 +1912,29 @@ DxeImageVerificationHandler (
//
// Check the image's hash value.
//
- if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1, mImageDigest, &mCertType, mImageDigestSize)) {
+ DbStatus = IsSignatureFoundInDatabase (
+ EFI_IMAGE_SECURITY_DATABASE1,
+ mImageDigest,
+ &mCertType,
+ mImageDigestSize,
+ &IsFound
+ );
+ if (EFI_ERROR (DbStatus) || IsFound) {
Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;
DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but %s hash of image is found in DBX.\n", mHashTypeStr));
IsVerified = FALSE;
break;
}
+
if (!IsVerified) {
- if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE, mImageDigest, &mCertType, mImageDigestSize)) {
+ DbStatus = IsSignatureFoundInDatabase (
+ EFI_IMAGE_SECURITY_DATABASE,
+ mImageDigest,
+ &mCertType,
+ mImageDigestSize,
+ &IsFound
+ );
+ if (!EFI_ERROR (DbStatus) && IsFound) {
IsVerified = TRUE;
} else {
DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but signature is not allowed by DB and %s hash of image is not found in DB/DBX.\n", mHashTypeStr));
--
2.24.0.windows.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [edk2-devel] [PATCH 9/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsSignatureFoundInDatabase(CVE-2019-14575)
2020-02-06 14:19 ` [PATCH 9/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsSignatureFoundInDatabase(CVE-2019-14575) Wang, Jian J
@ 2020-02-13 9:02 ` Zhang, Chao B
2020-02-13 10:20 ` Yao, Jiewen
1 sibling, 0 replies; 27+ messages in thread
From: Zhang, Chao B @ 2020-02-13 9:02 UTC (permalink / raw)
To: devel@edk2.groups.io, Wang, Jian J; +Cc: Yao, Jiewen
Ack-by : Chao Zhang <chao.b.zhang@intel.com>
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wang, Jian J
Sent: Thursday, February 6, 2020 10:20 PM
To: devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
Subject: [edk2-devel] [PATCH 9/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsSignatureFoundInDatabase(CVE-2019-14575)
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
To avoid false-negative issue in check hash against dbx, both error condition (as return value) and check result (as out parameter) of
IsSignatureFoundInDatabase() are added. So the caller of this function will know exactly if a failure is caused by a black list hit or other error happening, and enforce a more secure operation to prevent secure boot from being bypassed. For a white list check (db), there's no such necessity.
All intermediate results inside this function will be checked and returned immediately upon any failure or error, like out-of-resource, hash calculation error or certificate retrieval failure.
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>
---
.../DxeImageVerificationLib.c | 77 ++++++++++++++-----
1 file changed, 58 insertions(+), 19 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 5b7a67f811..8e599ca0be 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLi
+++ b.c
@@ -955,17 +955,19 @@ Done:
@param[in] Signature Pointer to signature that is searched for. @param[in] CertType Pointer to hash algorithm. @param[in] SignatureSize Size of Signature.+ @param[out] IsFound Search result. Only valid if EFI_SUCCESS returned - @return TRUE Found the signature in the variable database.- @return FALSE Not found the signature in the variable database.+ @retval EFI_SUCCESS Finished the search without any error.+ @retval Others Error occurred in the search of database. **/-BOOLEAN+EFI_STATUS IsSignatureFoundInDatabase (- IN CHAR16 *VariableName,- IN UINT8 *Signature,- IN EFI_GUID *CertType,- IN UINTN SignatureSize+ IN CHAR16 *VariableName,+ IN UINT8 *Signature,+ IN EFI_GUID *CertType,+ IN UINTN SignatureSize,+ OUT BOOLEAN *IsFound ) { EFI_STATUS Status;@@ -975,22 +977,28 @@ IsSignatureFoundInDatabase (
UINT8 *Data; UINTN Index; UINTN CertCount;- BOOLEAN IsFound; // // Read signature database variable. //- IsFound = FALSE;+ *IsFound = FALSE; Data = NULL; DataSize = 0; Status = gRT->GetVariable (VariableName, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, NULL); if (Status != EFI_BUFFER_TOO_SMALL) {- return FALSE;+ if (Status == EFI_NOT_FOUND) {+ //+ // No database, no need to search.+ //+ Status = EFI_SUCCESS;+ }++ return Status; } Data = (UINT8 *) AllocateZeroPool (DataSize); if (Data == NULL) {- return FALSE;+ return EFI_OUT_OF_RESOURCES; } Status = gRT->GetVariable (VariableName, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, Data);@@ -1010,7 +1018,7 @@ IsSignatureFoundInDatabase (
// // Find the signature in database. //- IsFound = TRUE;+ *IsFound = TRUE; // // Entries in UEFI_IMAGE_SECURITY_DATABASE that are used to validate image should be measured //@@ -1023,7 +1031,7 @@ IsSignatureFoundInDatabase (
Cert = (EFI_SIGNATURE_DATA *) ((UINT8 *) Cert + CertList->SignatureSize); } - if (IsFound) {+ if (*IsFound) { break; } }@@ -1037,7 +1045,7 @@ Done:
FreePool (Data); } - return IsFound;+ return Status; } /**@@ -1642,6 +1650,8 @@ DxeImageVerificationHandler (
CHAR16 *NameStr; RETURN_STATUS PeCoffStatus; EFI_STATUS HashStatus;+ EFI_STATUS DbStatus;+ BOOLEAN IsFound; SignatureList = NULL; SignatureListSize = 0;@@ -1650,7 +1660,7 @@ DxeImageVerificationHandler (
PkcsCertData = NULL; Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED; IsVerified = FALSE;-+ IsFound = FALSE; // // Check the image type and get policy setting.@@ -1792,7 +1802,14 @@ DxeImageVerificationHandler (
goto Failed; } - if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1, mImageDigest, &mCertType, mImageDigestSize)) {+ DbStatus = IsSignatureFoundInDatabase (+ EFI_IMAGE_SECURITY_DATABASE1,+ mImageDigest,+ &mCertType,+ mImageDigestSize,+ &IsFound+ );+ if (EFI_ERROR (DbStatus) || IsFound) { // // Image Hash is in forbidden database (DBX). //@@ -1800,7 +1817,14 @@ DxeImageVerificationHandler (
goto Failed; } - if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE, mImageDigest, &mCertType, mImageDigestSize)) {+ DbStatus = IsSignatureFoundInDatabase (+ EFI_IMAGE_SECURITY_DATABASE,+ mImageDigest,+ &mCertType,+ mImageDigestSize,+ &IsFound+ );+ if (!EFI_ERROR (DbStatus) && IsFound) { // // Image Hash is in allowed database (DB). //@@ -1888,14 +1912,29 @@ DxeImageVerificationHandler (
// // Check the image's hash value. //- if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1, mImageDigest, &mCertType, mImageDigestSize)) {+ DbStatus = IsSignatureFoundInDatabase (+ EFI_IMAGE_SECURITY_DATABASE1,+ mImageDigest,+ &mCertType,+ mImageDigestSize,+ &IsFound+ );+ if (EFI_ERROR (DbStatus) || IsFound) { Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND; DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but %s hash of image is found in DBX.\n", mHashTypeStr)); IsVerified = FALSE; break; }+ if (!IsVerified) {- if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE, mImageDigest, &mCertType, mImageDigestSize)) {+ DbStatus = IsSignatureFoundInDatabase (+ EFI_IMAGE_SECURITY_DATABASE,+ mImageDigest,+ &mCertType,+ mImageDigestSize,+ &IsFound+ );+ if (!EFI_ERROR (DbStatus) && IsFound) { IsVerified = TRUE; } else { DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but signature is not allowed by DB and %s hash of image is not found in DB/DBX.\n", mHashTypeStr));--
2.24.0.windows.2
-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53875): https://edk2.groups.io/g/devel/message/53875
Mute This Topic: https://groups.io/mt/71023427/1790519
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [chao.b.zhang@intel.com] -=-=-=-=-=-=
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 9/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsSignatureFoundInDatabase(CVE-2019-14575)
2020-02-06 14:19 ` [PATCH 9/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsSignatureFoundInDatabase(CVE-2019-14575) Wang, Jian J
2020-02-13 9:02 ` [edk2-devel] " Zhang, Chao B
@ 2020-02-13 10:20 ` Yao, Jiewen
1 sibling, 0 replies; 27+ messages in thread
From: Yao, Jiewen @ 2020-02-13 10:20 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io; +Cc: Zhang, Chao B
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:20 PM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>
> Subject: [PATCH 9/9] SecurityPkg/DxeImageVerificationLib: Differentiate error
> and search result in IsSignatureFoundInDatabase(CVE-2019-14575)
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
>
> To avoid false-negative issue in check hash against dbx, both error
> condition (as return value) and check result (as out parameter) of
> IsSignatureFoundInDatabase() are added. So the caller of this function
> will know exactly if a failure is caused by a black list hit or
> other error happening, and enforce a more secure operation to prevent
> secure boot from being bypassed. For a white list check (db), there's
> no such necessity.
>
> All intermediate results inside this function will be checked and
> returned immediately upon any failure or error, like out-of-resource,
> hash calculation error or certificate retrieval failure.
>
> 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>
> ---
> .../DxeImageVerificationLib.c | 77 ++++++++++++++-----
> 1 file changed, 58 insertions(+), 19 deletions(-)
>
> diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 5b7a67f811..8e599ca0be 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -955,17 +955,19 @@ Done:
> @param[in] Signature Pointer to signature that is searched for.
>
> @param[in] CertType Pointer to hash algorithm.
>
> @param[in] SignatureSize Size of Signature.
>
> + @param[out] IsFound Search result. Only valid if EFI_SUCCESS returned
>
>
>
> - @return TRUE Found the signature in the variable database.
>
> - @return FALSE Not found the signature in the variable database.
>
> + @retval EFI_SUCCESS Finished the search without any error.
>
> + @retval Others Error occurred in the search of database.
>
>
>
> **/
>
> -BOOLEAN
>
> +EFI_STATUS
>
> IsSignatureFoundInDatabase (
>
> - IN CHAR16 *VariableName,
>
> - IN UINT8 *Signature,
>
> - IN EFI_GUID *CertType,
>
> - IN UINTN SignatureSize
>
> + IN CHAR16 *VariableName,
>
> + IN UINT8 *Signature,
>
> + IN EFI_GUID *CertType,
>
> + IN UINTN SignatureSize,
>
> + OUT BOOLEAN *IsFound
>
> )
>
> {
>
> EFI_STATUS Status;
>
> @@ -975,22 +977,28 @@ IsSignatureFoundInDatabase (
> UINT8 *Data;
>
> UINTN Index;
>
> UINTN CertCount;
>
> - BOOLEAN IsFound;
>
>
>
> //
>
> // Read signature database variable.
>
> //
>
> - IsFound = FALSE;
>
> + *IsFound = FALSE;
>
> Data = NULL;
>
> DataSize = 0;
>
> Status = gRT->GetVariable (VariableName, &gEfiImageSecurityDatabaseGuid,
> NULL, &DataSize, NULL);
>
> if (Status != EFI_BUFFER_TOO_SMALL) {
>
> - return FALSE;
>
> + if (Status == EFI_NOT_FOUND) {
>
> + //
>
> + // No database, no need to search.
>
> + //
>
> + Status = EFI_SUCCESS;
>
> + }
>
> +
>
> + return Status;
>
> }
>
>
>
> Data = (UINT8 *) AllocateZeroPool (DataSize);
>
> if (Data == NULL) {
>
> - return FALSE;
>
> + return EFI_OUT_OF_RESOURCES;
>
> }
>
>
>
> Status = gRT->GetVariable (VariableName, &gEfiImageSecurityDatabaseGuid,
> NULL, &DataSize, Data);
>
> @@ -1010,7 +1018,7 @@ IsSignatureFoundInDatabase (
> //
>
> // Find the signature in database.
>
> //
>
> - IsFound = TRUE;
>
> + *IsFound = TRUE;
>
> //
>
> // Entries in UEFI_IMAGE_SECURITY_DATABASE that are used to validate
> image should be measured
>
> //
>
> @@ -1023,7 +1031,7 @@ IsSignatureFoundInDatabase (
> Cert = (EFI_SIGNATURE_DATA *) ((UINT8 *) Cert + CertList->SignatureSize);
>
> }
>
>
>
> - if (IsFound) {
>
> + if (*IsFound) {
>
> break;
>
> }
>
> }
>
> @@ -1037,7 +1045,7 @@ Done:
> FreePool (Data);
>
> }
>
>
>
> - return IsFound;
>
> + return Status;
>
> }
>
>
>
> /**
>
> @@ -1642,6 +1650,8 @@ DxeImageVerificationHandler (
> CHAR16 *NameStr;
>
> RETURN_STATUS PeCoffStatus;
>
> EFI_STATUS HashStatus;
>
> + EFI_STATUS DbStatus;
>
> + BOOLEAN IsFound;
>
>
>
> SignatureList = NULL;
>
> SignatureListSize = 0;
>
> @@ -1650,7 +1660,7 @@ DxeImageVerificationHandler (
> PkcsCertData = NULL;
>
> Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
>
> IsVerified = FALSE;
>
> -
>
> + IsFound = FALSE;
>
>
>
> //
>
> // Check the image type and get policy setting.
>
> @@ -1792,7 +1802,14 @@ DxeImageVerificationHandler (
> goto Failed;
>
> }
>
>
>
> - if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1,
> mImageDigest, &mCertType, mImageDigestSize)) {
>
> + DbStatus = IsSignatureFoundInDatabase (
>
> + EFI_IMAGE_SECURITY_DATABASE1,
>
> + mImageDigest,
>
> + &mCertType,
>
> + mImageDigestSize,
>
> + &IsFound
>
> + );
>
> + if (EFI_ERROR (DbStatus) || IsFound) {
>
> //
>
> // Image Hash is in forbidden database (DBX).
>
> //
>
> @@ -1800,7 +1817,14 @@ DxeImageVerificationHandler (
> goto Failed;
>
> }
>
>
>
> - if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE,
> mImageDigest, &mCertType, mImageDigestSize)) {
>
> + DbStatus = IsSignatureFoundInDatabase (
>
> + EFI_IMAGE_SECURITY_DATABASE,
>
> + mImageDigest,
>
> + &mCertType,
>
> + mImageDigestSize,
>
> + &IsFound
>
> + );
>
> + if (!EFI_ERROR (DbStatus) && IsFound) {
>
> //
>
> // Image Hash is in allowed database (DB).
>
> //
>
> @@ -1888,14 +1912,29 @@ DxeImageVerificationHandler (
> //
>
> // Check the image's hash value.
>
> //
>
> - if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1,
> mImageDigest, &mCertType, mImageDigestSize)) {
>
> + DbStatus = IsSignatureFoundInDatabase (
>
> + EFI_IMAGE_SECURITY_DATABASE1,
>
> + mImageDigest,
>
> + &mCertType,
>
> + mImageDigestSize,
>
> + &IsFound
>
> + );
>
> + if (EFI_ERROR (DbStatus) || IsFound) {
>
> Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;
>
> DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but %s
> hash of image is found in DBX.\n", mHashTypeStr));
>
> IsVerified = FALSE;
>
> break;
>
> }
>
> +
>
> if (!IsVerified) {
>
> - if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE,
> mImageDigest, &mCertType, mImageDigestSize)) {
>
> + DbStatus = IsSignatureFoundInDatabase (
>
> + EFI_IMAGE_SECURITY_DATABASE,
>
> + mImageDigest,
>
> + &mCertType,
>
> + mImageDigestSize,
>
> + &IsFound
>
> + );
>
> + if (!EFI_ERROR (DbStatus) && IsFound) {
>
> IsVerified = TRUE;
>
> } else {
>
> DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but
> signature is not allowed by DB and %s hash of image is not found in DB/DBX.\n",
> mHashTypeStr));
>
> --
> 2.24.0.windows.2
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler
2020-02-06 14:19 [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler Wang, Jian J
` (8 preceding siblings ...)
2020-02-06 14:19 ` [PATCH 9/9] SecurityPkg/DxeImageVerificationLib: Differentiate error and search result in IsSignatureFoundInDatabase(CVE-2019-14575) Wang, Jian J
@ 2020-02-13 1:53 ` Liming Gao
9 siblings, 0 replies; 27+ messages in thread
From: Liming Gao @ 2020-02-13 1:53 UTC (permalink / raw)
To: devel@edk2.groups.io, Wang, Jian J, Yao, Jiewen, Zhang, Chao B
Jian, Jiewen and Chao:
Does this patch catch to edk2 Q1 stable tag? If yes, can this patch be reviewed this week, because Q1 stable tag soft feature freeze is 2020-02-14.
Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wang, Jian J
> 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: [edk2-devel] [PATCH 0/9] Fix false negative issue in DxeImageVerificationHandler
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1608
>
> Patch branch: https://github.com/jwang36/edk2/tree/fix-bz1608-bypass-blacklist-check-via-signature
>
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
>
>
> Jian J Wang (8):
> SecurityPkg/DxeImageVerificationLib: Fix memory leaks(CVE-2019-14575)
> SecurityPkg/DxeImageVerificationLib: reject CertStack.CertNumber==0
> per DBX(CVE-2019-14575)
> SecurityPkg/DxeImageVerificationLib: fix wrong fetching dbx in
> IsAllowedByDb(CVE-2019-14575)
> SecurityPkg/DxeImageVerificationLib: avoid bypass in fetching dbx in
> IsAllowedByDb(CVE-2019-14575)
> SecurityPkg/DxeImageVerificationLib: refactor db/dbx fetching code in
> IsAllowedByDb(CVE-2019-14575)
> SecurityPkg/DxeImageVerificationLib: Differentiate error and search
> result in IsCertHashFoundInDatabase(CVE-2019-14575)
> SecurityPkg/DxeImageVerificationLib: tighten default result of
> IsForbiddenByDbx()(CVE-2019-14575)
> SecurityPkg/DxeImageVerificationLib: Differentiate error and search
> result in IsSignatureFoundInDatabase(CVE-2019-14575)
>
> Laszlo Ersek (1):
> SecurityPkg/DxeImageVerificationLib: plug Data leak in
> IsForbiddenByDbx()(CVE-2019-14575)
>
> .../DxeImageVerificationLib.c | 283 ++++++++++++------
> 1 file changed, 191 insertions(+), 92 deletions(-)
>
> --
> 2.24.0.windows.2
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#53866): https://edk2.groups.io/g/devel/message/53866
> Mute This Topic: https://groups.io/mt/71023416/1759384
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [liming.gao@intel.com]
> -=-=-=-=-=-=
^ permalink raw reply [flat|nested] 27+ messages in thread