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

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

Laszlo,

I think a new BZ is a good idea.  I am sure there is more 
history here and more discussion required on this invalid
policy PCD setting case.

I would also like to see a DEBUG() message or even better
a REPORT_STATUS_CODE() for an invalid policy PCD setting
and I would like platform policy to decide if the platform
should deadloop or continue with EFI_ACCESS_DENIED.  By
putting the deadloop in this function, it takes away the
option for the platform to make that decision.

I also find ASSERT(FALSE) harder to triage.  I prefer the
debug log to provide some indication of the cause of the
assert.  Then I can go look up the file/line number for
more context.

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, January 31, 2020 12:13 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> 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: Re: [edk2-devel] [PATCH 00/11]
> SecurityPkg/DxeImageVerificationHandler: fix retval for
> "deny" policy
> 
> 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/DxeImageV
> erificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageV
> erificationLib.c
> > index ff79e30ef83e..1d41161abedc 100644
> > ---
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageV
> erificationLib.c
> > +++
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageV
> erificationLib.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