Re: [edk2-devel] [PATCH] SecurityPkg/MeasureBootLib: Return EFI_ACCESS_DENIED after image check fail

Re: [edk2-devel] [PATCH] SecurityPkg/MeasureBootLib: Return EFI_ACCESS_DENIED after image check fail

[Guomin Jiang]

Hi Jiewen, Jiang, Chao,

Could you help review the change.

Best Regards
Guomin

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin
> Jiang
> Sent: Wednesday, April 1, 2020 9:11 AM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
> Subject: [edk2-devel] [PATCH] SecurityPkg/MeasureBootLib: Return
> EFI_ACCESS_DENIED after image check fail
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
> 
> If check the File at the begin of function, it will only allow the File is present
> and forbid image from buffer.
> It is possible that image come from the memory buffer, so make it can run
> and check the File after it.
> It is improvement for 4b026f0d5af36faf3a3629a3ad49c51b5b3be12f.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> ---
>  .../DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c  | 14 +++++++-----
> --
>  .../DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c    | 14 +++++++------
> -
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git
> a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.
> c
> b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.
> c
> index f0e95e5ec0..fdb4758cbe 100644
> ---
> a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.
> c
> +++
> b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.
> c
> @@ -435,13 +435,6 @@ DxeTpm2MeasureBootHandler (
>    EFI_PHYSICAL_ADDRESS                FvAddress;   UINT32                              Index; -
> //-  // Check for invalid parameters.-  //-  if (File == NULL) {-    return
> EFI_ACCESS_DENIED;-  }-   Status = gBS->LocateProtocol
> (&gEfiTcg2ProtocolGuid, NULL, (VOID **) &Tcg2Protocol);   if (EFI_ERROR
> (Status)) {     //@@ -615,6 +608,13 @@ DxeTpm2MeasureBootHandler (
>    //   Status = PeCoffLoaderGetImageInfo (&ImageContext);   if (EFI_ERROR
> (Status)) {+    //+    // Check for invalid parameters.+    //+    if (File == NULL) {+
> Status = EFI_ACCESS_DENIED;+    }+     //     // The information can't be got
> from the invalid PeImage     //diff --git
> a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
> b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
> index d499371e7a..20f7d94d6b 100644
> ---
> a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
> +++
> b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
> @@ -732,13 +732,6 @@ DxeTpmMeasureBootHandler (
>    EFI_PHYSICAL_ADDRESS                FvAddress;   UINT32                              Index; -
> //-  // Check for invalid parameters.-  //-  if (File == NULL) {-    return
> EFI_ACCESS_DENIED;-  }-   Status = gBS->LocateProtocol
> (&gEfiTcgProtocolGuid, NULL, (VOID **) &TcgProtocol);   if (EFI_ERROR
> (Status)) {     //@@ -912,6 +905,13 @@ DxeTpmMeasureBootHandler (
>    //   Status = PeCoffLoaderGetImageInfo (&ImageContext);   if (EFI_ERROR
> (Status)) {+    //+    // Check for invalid parameters.+    //+    if (File == NULL) {+
> return EFI_ACCESS_DENIED;+    }+     //     // The information can't be got from
> the invalid PeImage     //--
> 2.25.1.windows.1
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#56805): https://edk2.groups.io/g/devel/message/56805
> Mute This Topic: https://groups.io/mt/72691331/4399222
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [guomin.jiang@intel.com] -=-=-=-=-=-=

Re: [edk2-devel] [PATCH] SecurityPkg/MeasureBootLib: Return EFI_ACCESS_DENIED after image check fail

[Laszlo Ersek]

Hi,

On 04/01/20 03:11, Guomin Jiang wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
> 
> If check the File at the begin of function, it will only allow the File is
> present and forbid image from buffer.
> It is possible that image come from the memory buffer, so make it can run
> and check the File after it.

Unfortunately, this commit message is unhelpful.

The use case you are trying to describe is presumably the following:

the DxeTpm2MeasureBootHandler() and DxeTpmMeasureBootHandler() functions
may receive a FileBuffer argument that is not associated with any
particular device path (e.g., because the UEFI image has not been loaded
from any particular device path). Therefore rejecting (File==NULL) at
the top of the function is invalid.

(1) So please state this clearly in the commit message.

The subject line is similarly incomprehensible. It should state the
*goal* of the patch. Something like:

SecurityPkg/TPM: measure UEFI images without associated device paths again

(74 characters)

(2) However, this is a use case that currently does not fully conform to
the SECURITY2_FILE_AUTHENTICATION_HANDLER specification in
"MdeModulePkg/Include/Library/SecurityManagementLib.h":

- The documentation for "@param[in] File" does not explain what happens
when File is NULL. The description says "This will optionally be used
for logging" -- dependent on what? The statement means, "Maybe we'll use
File for logging, maybe we won't". It doesn't explain what the decision
depends on, and it also doesn't explain what *else* the File parameter
could be used to. The generic description is "The pointer to the device
path of the file that is being dispatched", and passing NULL for that is
not valid.

- In the actual parameter list, "File" is not annotated as OPTIONAL.

So please fix up the lib class header *first*. If the documentation had
been complete when Phil was working on 4b026f0d5af3 ("SecurityPkg: Fix
incorrect return value when File is NULL", 2020-02-10), then the use
case in question would not have been regressed. (Because then we'd have
understood that (File == NULL) is valid, under certain circumstances.)

> It is improvement for 4b026f0d5af36faf3a3629a3ad49c51b5b3be12f.

In fact 4b026f0d5af3 introduced a regression, so this patch is a
regression fix. Please mark TianoCore#2652 with the Regression keyword,
and add the following to the commit message here:

Fixes: 4b026f0d5af36faf3a3629a3ad49c51b5b3be12f

Regarding the logic change, I'll let the SecurityPkg maintainers /
reviewers verify that.

Thanks
Laszlo


> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> ---
>  .../DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c  | 14 +++++++-------
>  .../DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c    | 14 +++++++-------
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
> index f0e95e5ec0..fdb4758cbe 100644
> --- a/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
> +++ b/SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.c
> @@ -435,13 +435,6 @@ DxeTpm2MeasureBootHandler (
>    EFI_PHYSICAL_ADDRESS                FvAddress;
>    UINT32                              Index;
>  
> -  //
> -  // Check for invalid parameters.
> -  //
> -  if (File == NULL) {
> -    return EFI_ACCESS_DENIED;
> -  }
> -
>    Status = gBS->LocateProtocol (&gEfiTcg2ProtocolGuid, NULL, (VOID **) &Tcg2Protocol);
>    if (EFI_ERROR (Status)) {
>      //
> @@ -615,6 +608,13 @@ DxeTpm2MeasureBootHandler (
>    //
>    Status = PeCoffLoaderGetImageInfo (&ImageContext);
>    if (EFI_ERROR (Status)) {
> +    //
> +    // Check for invalid parameters.
> +    //
> +    if (File == NULL) {
> +      Status = EFI_ACCESS_DENIED;
> +    }
> +
>      //
>      // The information can't be got from the invalid PeImage
>      //
> diff --git a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
> index d499371e7a..20f7d94d6b 100644
> --- a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
> +++ b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
> @@ -732,13 +732,6 @@ DxeTpmMeasureBootHandler (
>    EFI_PHYSICAL_ADDRESS                FvAddress;
>    UINT32                              Index;
>  
> -  //
> -  // Check for invalid parameters.
> -  //
> -  if (File == NULL) {
> -    return EFI_ACCESS_DENIED;
> -  }
> -
>    Status = gBS->LocateProtocol (&gEfiTcgProtocolGuid, NULL, (VOID **) &TcgProtocol);
>    if (EFI_ERROR (Status)) {
>      //
> @@ -912,6 +905,13 @@ DxeTpmMeasureBootHandler (
>    //
>    Status = PeCoffLoaderGetImageInfo (&ImageContext);
>    if (EFI_ERROR (Status)) {
> +    //
> +    // Check for invalid parameters.
> +    //
> +    if (File == NULL) {
> +      return EFI_ACCESS_DENIED;
> +    }
> +
>      //
>      // The information can't be got from the invalid PeImage
>      //
>