Adventures with the UEFI shim

[Paul Moore]

On Fri, Nov 13, 2020 at 11:25 AM Javier Martinez Canillas
<fmartine at redhat.com> wrote:
>
> [adding more people that I think will have a say on this topic]
>
> On 10/29/20 11:24 PM, Paul Moore wrote:
> > On Sun, Oct 11, 2020 at 8:39 PM Paul Moore <paul at paul-moore.com> wrote:
> >> On Sun, Oct 11, 2020 at 2:50 PM Peter Jones <pjones at redhat.com> wrote:
> >>> On Thu, Oct 08, 2020 at 11:42:33AM -0400, Paul Moore wrote:
> >>>> On Wed, Oct 7, 2020 at 3:17 PM Peter Jones <pjones at redhat.com> wrote:
> >>>>> On Thu, Oct 01, 2020 at 03:16:08PM -0400, Paul Moore wrote:
> >
> > ...
> >
> >>>>> If it weren't for that, I would say:
> >>>>> - put a pubkey in shim like normal
> >>>>> - sign your kernel with it
> >>>>> - make systemd-boot call thezi shim lock protocol on the embedded kernel
> >>>>
> >>>> Yes, I'm already planning on steps 1 and 2 (those are needed pretty
> >>>> much regardless of the design), I was just wondering if the UEFI shim
> >>>> folks would be open to working on solution that doesn't require the
> >>>> "loader_is_participating" check in shim's ExitBootServices hook.
> >>>> Conceptually if shim has already verified the signature on the next
> >>>> stage of the boot process that would seem to imply a certain level of
> >>>> trust in that binary.  Would you be open to a mechanism for post-shim
> >>>> loaders to signal shim that they do not need additional verification
> >>>> via the shim protocol?
> >>>
> >>> I think that sounds like a reasonable trade-off, yes, though I do think
> >>> we'd want to see the code during review, just to be sure.
> >>
> >> That's fair.  Do you have any suggestions about how to skip the
> >> "loader_is_participating" check that you would find acceptable
> >> upstream?  As I said above, I think just removing it should be
> >> reasonable and in keeping with the transitive trust of UEFI SB, but
> >> I'm happy taking a different approach if that is what you guys want.
> >
> > In an effort to move this discussion forward I posted my current code
> > as a PR here:
> >
> > * https://github.com/rhboot/shim/pull/233
> >
>
> As far as I can tell you are trying to overcome the following limitations
> that exist with the current PCR measurements in shim and GRUB:
>
> 1) PCR7 can suffer of PCR fragility because it measures the db and dbx
>    variables and this could change due an EFI firmware update.
>
> 2) PCR9 only contains measurements of the kernel if GRUB is used.

For my selfish use case, I don't particularly care about GRUB; I'm
booting a kernel image directly from shim.  The PCR8 extensions are to
work around the problems with PCR7 and the PCR9 extensions are simply
there for those who want a measurement of the kernel+loadoptions.  I
currently do use PCR9, but I can see using in the future if/when I
start to tackle remote attestation.

> I wonder if what is measured and the PCR extended could be changed in
> shim and GRUB to have a more flexible policy that could solve both.
>
> That is, maybe using a different PCR for db and dbx? That way PCR7
> will have stable measurements across firmware updates, and if db/dbx
> should be part of a PCR policy, then this other PCR could be used in
> conjunction with PCR7.

The problem with the PCR7 db/dbx extensions is that they are done in
the firmware according to spec, shim only extends the relevant entries
from the db/dbx.  At least that is my understanding from reading the
UEFI/TPM/TCG specs and sifting through the TPM event logs from a
handful of systems.

I chose PCR8 and PCR9 because they are unused if a second stage loader
is not used, and I like to think that the extensions I proposed are at
least in keeping with the spirit of the existing GRUB PC8 and PCR9
usage.

> Same question about PCR9, maybe the loader_is_participating variable
> can be used to tell shim if should extend PCR9 with the loaded image?

Earlier versions of my patch keyed of a default vs specified second
stage loader for the PCR8 and PCR9 extensions in shim.  If a default
second stage loader was booted then shim refrained from making the
PCR8 and PCR9 extensions, but if a loader was specified in the shim's
loadoptions then the PCR8 and PCR9 extensions would be done.  The idea
being that it would be extremely unlikely to boot directly into the
kernel if you were relying on the default loader; if you are using the
default, it is very likely that the default is a bootloader itself.

Ultimately I ditched that both for simplicity in the patch/PR and to
remove any ambiguity of the PCR extensions in a given shim binary.  It
seems like having a single shim binary extend PCRs differently based
on the type of second stage loader used is a problem waiting to
happen.

> Because a problem I see with your patches is that it seems to me that
> are adding a bunch of build time options that could be mix and matched
> to have ad-hoc TPM measurements policies.

I personally think build time adjustments to the shim's PCR extension
policy are reasonable so long as it is documented.  There is enough
process and cost involved in building, and signing, a shim that only a
limited number of people (almost all distros, yes?) are ever going to
bother with such an exercise.  With that in mind, and working under
the assumption that these distros will not change their shim PCR
policy on a whim, having this flexibility doesn't seem like a
significant problem to me.  Anyone attempting to do remote attestation
with these PCR values should be going through the event logs and
understanding what is being measured and extended anyway, so this
should not be a problem or a surprise.  If anyone is doing attestation
without doing their homework then it really doesn't matter anyway;
they are simply going to use the PCR values they get from their
(hw/distro/etc.) vendors and move on.

> We could add defaults for these, but I guess that will be confusing and
> also the attestation services will need to be aware of what was enabled
> or disabled in shim for a given setup.

I think the attestation services need to do that anyway.  I'm also not
advocating for these to be enabled by default.

> A third thing that that your patches tackle is shim's assumption that
> a second stage bootloader using the shim_lock protocol will be part of
> the boot path.
>
> I've to admit that I don't fully understand why the ExitBootServices()
> is checking for the loader_is_participating variable, since it already
> trusts the binary and could just assume that would do the right thing.

Agreed, I've been very confused by why this is necessary, but I
recognize that I haven't looked at all the various fallback scenarios
that shim/friends have to deal with.

-- 
paul moore
www.paul-moore.com