Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Laszlo,

I have reviewed this patch series.  All the patches look
good.  The detailed description of each change made it 
easy to review.

Series Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

I have one question about that is not directly related to
this logic change.

I see this logic that checks for invalid policy settings and
ASSERT()s and halts in a deadloop if either of 2 specific values
are used that have been removed from the UEFI Spec.

  //
  // The policy QUERY_USER_ON_SECURITY_VIOLATION and ALLOW_EXECUTE_ON_SECURITY_VIOLATION
  // violates the UEFI spec and has been removed.
  //
  ASSERT (Policy != QUERY_USER_ON_SECURITY_VIOLATION && Policy != ALLOW_EXECUTE_ON_SECURITY_VIOLATION);
  if (Policy == QUERY_USER_ON_SECURITY_VIOLATION || Policy == ALLOW_EXECUTE_ON_SECURITY_VIOLATION) {
    CpuDeadLoop ();
  }

There are 3 conditions where the Policy comes from a PCD.

  //
  // Check the image type and get policy setting.
  //
  switch (GetImageType (File)) {

  case IMAGE_FROM_FV:
    Policy = ALWAYS_EXECUTE;
    break;

  case IMAGE_FROM_OPTION_ROM:
    Policy = PcdGet32 (PcdOptionRomImageVerificationPolicy);
    break;

  case IMAGE_FROM_REMOVABLE_MEDIA:
    Policy = PcdGet32 (PcdRemovableMediaImageVerificationPolicy);
    break;

  case IMAGE_FROM_FIXED_MEDIA:
    Policy = PcdGet32 (PcdFixedMediaImageVerificationPolicy);
    break;

  default:
    Policy = DENY_EXECUTE_ON_SECURITY_VIOLATION;
    break;
  }

However, there are no checks in this function to verify that 
Policy is set to one of the allowed values.  This means
an invalid PCD value will fall through to the EFI_ACCESS_DENIED
case.  Is this the expected behavior for an invalid PCD setting?
If so, then why is there a check for the retired values from
the UEFI Spec and a deadloop performed.  That seems inconsistent
and not a good idea to deadloop if we do not have to.  Would
it be better to return EFI_ACCESS_DENIED for these 2 retired
Policy values as well?

I am ok if you consider this to be a different issue. 

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Laszlo Ersek
> Sent: Thursday, January 16, 2020 11:07 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: [edk2-devel] [PATCH 00/11]
> SecurityPkg/DxeImageVerificationHandler: fix retval for
> "deny" policy
> 
> Ref:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2129
> Repo:   https://github.com/lersek/edk2.git
> Branch: deny_execute_2129
> 
> The DxeImageVerificationHandler() function does not
> handle the
> DENY_EXECUTE_ON_SECURITY_VIOLATION policy correctly.
> When an image is
> rejected, and the platform sets this policy for the
> corresponding image
> source, the function should return EFI_ACCESS_DENIED.
> Instead, the
> function currently returns EFI_SECURITY_VIOLATION. The
> consequence is
> that gBS->LoadImage() will keep the image loaded (in
> untrusted state),
> rather than unloading it immediately. If the platform
> sets the
> DENY_EXECUTE_ON_SECURITY_VIOLATION policy for all image
> sources, then
> the platform may not expect EFI_SECURITY_VIOLATION at
> all. Then,
> rejected images may linger in RAM, in untrusted state,
> and may be leaked
> forever.
> 
> This series refactors the DxeImageVerificationHandler()
> function,
> simplifying the control flow. The series also improves
> the conformance
> of the return values to the
> SECURITY2_FILE_AUTHENTICATION_HANDLER
> prototype. The last two patches are actual bugfixes,
> with the last one
> fixing the problem laid out above.
> 
> The patches in this posting have been formatted with
> "--function-context", for easier review.
> 
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (11):
>   SecurityPkg/DxeImageVerificationHandler: simplify
> "VerifyStatus"
>   SecurityPkg/DxeImageVerificationHandler: remove
> "else" after
>     return/break
>   SecurityPkg/DxeImageVerificationHandler: keep PE/COFF
> info status
>     internal
>   SecurityPkg/DxeImageVerificationHandler: narrow down
> PE/COFF hash
>     status
>   SecurityPkg/DxeImageVerificationHandler: fix retval
> on memalloc
>     failure
>   SecurityPkg/DxeImageVerificationHandler: remove
> superfluous Status
>     setting
>   SecurityPkg/DxeImageVerificationHandler: unnest
> AddImageExeInfo() call
>   SecurityPkg/DxeImageVerificationHandler: eliminate
> "Status" variable
>   SecurityPkg/DxeImageVerificationHandler: fix retval
> for
>     (FileBuffer==NULL)
>   SecurityPkg/DxeImageVerificationHandler: fix imgexec
> info on memalloc
>     fail
>   SecurityPkg/DxeImageVerificationHandler: fix "defer"
> vs. "deny"
>     policies
> 
> 
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVer
> ificationLib.c | 118 ++++++++++----------
>  1 file changed, 59 insertions(+), 59 deletions(-)
> 
> --
> 2.19.1.3.g30247aa5d201
> 
> 
>