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

["Laszlo Ersek" <lersek@redhat.com>]

On 01/31/20 03:59, Kinney, Michael D wrote:
> 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>

Thank you very much!

(more below)

>
> 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.

That's right. I seem to recall that I noticed it too, and I wrote it off
with "you can do damage with a bunch of other PCDs as well, if you
misconfigure them".

> This means an invalid PCD value will fall through to the
> EFI_ACCESS_DENIED case.

Yes. I find that the safest (silent) fallback for an undefined (per DEC)
PCD value.

> 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?

Hmm, good point.

I think these scenarios are different, historically. In one case we have
a plain misconfigured platform. In the other case we have a platform
that used to have correct configuration (considering edk2 in itself),
but then for spec conformance reasons, it got invalidated
*retroactively*. And I guess we wanted to be as loud as possible about
the second case. There's a difference between "you didn't do your
homework" and "you did your homework but we've changed the curriculum
meanwhile".

So the main question is if we want to remain "silent" about "plain
misconfig", vs. "loud" about "obsolete config".

We could unify the handling of both cases ("loudly") like this, for
example:

> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index ff79e30ef83e..1d41161abedc 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1617,23 +1617,32 @@ DxeImageVerificationHandler (
>      Policy = DENY_EXECUTE_ON_SECURITY_VIOLATION;
>      break;
>    }
> +
> +  switch (Policy) {
>    //
>    // If policy is always/never execute, return directly.
>    //
> -  if (Policy == ALWAYS_EXECUTE) {
> +  case ALWAYS_EXECUTE:
>      return EFI_SUCCESS;
> -  }
> -  if (Policy == NEVER_EXECUTE) {
> +  case NEVER_EXECUTE:
>      return EFI_ACCESS_DENIED;
> -  }
>
>    //
> -  // The policy QUERY_USER_ON_SECURITY_VIOLATION and ALLOW_EXECUTE_ON_SECURITY_VIOLATION
> -  // violates the UEFI spec and has been removed.
> +  // "Defer" and "deny" are valid policies that require actual verification.
>    //
> -  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) {
> +  case DEFER_EXECUTE_ON_SECURITY_VIOLATION:
> +  case DENY_EXECUTE_ON_SECURITY_VIOLATION:
> +    break;
> +
> +  //
> +  // QUERY_USER_ON_SECURITY_VIOLATION and ALLOW_EXECUTE_ON_SECURITY_VIOLATION
> +  // violate the UEFI spec; they now indicate platform misconfig. Hang here if
> +  // we find those policies. Handle undefined policy values the same way.
> +  //
> +  default:
> +    ASSERT (FALSE);
>      CpuDeadLoop ();
> +    return EFI_ACCESS_DENIED;
>    }
>
>    GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID**)&SecureBoot, NULL);

Continuing:

On 01/31/20 03:59, Kinney, Michael D wrote:
> I am ok if you consider this to be a different issue.

Yes, TianoCore#2129 is about mistaking DENY for DEFER, while this other
topic is "complain loudly, rather than silently, about invalid PCD
settings".

So let me push this series as-is for TianoCore#2129, with your R-b
applied. Then I will submit the above patch for separate review -- I'll
put something like "hang on undefined image verification policy PCDs" in
the subject.

Would you like me to file a separate BZ too, for that patch?

Thanks!
Laszlo