Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PCR7 change in 15.7+ due to the switch to using MokListRT instead of MokList #714

Open
vittyvk opened this issue Jan 10, 2025 · 6 comments

Comments

@vittyvk
Copy link
Contributor

vittyvk commented Jan 10, 2025

Shim 15.7 switched to checking MokListRT instead of MokList to support cases when the former is extended by someone else, e.g. grub:

commit 092c2b2bbed950727e41cf450b61c794881c33e7
Author: Eric Snowberg <[email protected]>
Date:   Fri Jun 17 12:37:28 2022 -0400

    Reference MokListRT instead of MokList

Unfortunately, this commit also changed what's measured in PCR7 for shim builds using 'vendor_cert' (and not 'vendor_db'). Pre-change:

- EventNum: 28
  PCRIndex: 7
  EventType: EV_EFI_VARIABLE_AUTHORITY
  ...
  Event:
    VariableName: 605dab50-e046-4300-abb6-3dd810dd8b23
    UnicodeNameLength: 4
    VariableDataLength: 1119
    UnicodeName: Shim
    VariableData: ...

Post-change:

- EventNum: 28
  PCRIndex: 7
  EventType: EV_EFI_VARIABLE_AUTHORITY
  ...
  Event:
    VariableName: 605dab50-e046-4300-abb6-3dd810dd8b23
    UnicodeNameLength: 9
    VariableDataLength: 1135
    UnicodeName: MokListRT
    VariableData: ...

The reason for the change is that MokListRT gets 'vendor_cert'/'vendor_db' mirrored in it, however, shim checks MokListRT before it gets to checking 'vendor_cert' (but after 'vendor_db' so there's no change for these builds, see verify_one_signature()/check_allowlist()).

Now, the question is what can (or should) be done to this. I see the following options:

  • We change the order of the checks, namely move SHIM_CERT/VENDOR_CERT check to check_allowlist() before it gets to checking MokListRT. Unfortunately, this will result in PCR7 change again.
  • We do nothing and stick to the new behavior. It would probably be nice to have this documented somewhere as the discrepancy between 'vendor_db' and 'vendor_cert' is quite un-obvious.
  • We eradicate 'vendor_cert' completely and make everyone use 'vendor_db' even for a single cert.
@vittyvk
Copy link
Contributor Author

vittyvk commented Jan 10, 2025

Cc @esnowberg

@chrisccoulson
Copy link
Collaborator

Huh, I thought I'd already opened an issue for this because this is something we had to add a workaround for in Ubuntu Core some time ago, but it seems like I didn't.

@vittyvk
Copy link
Contributor Author

vittyvk commented Jan 14, 2025

It was brought to my attention that there's #616 already which basically describes the same issue but as it only mentions 'dead code' and not PCR7 measurements change, this went unnoticed.

@trungams
Copy link

We also noticed this PCR7 value change in Azure Linux after switching from shim 15.3 to shim 15.8. We depend on PCR7 for our confidential VM image so it would be nice to have the behaviour stable.

@chrisccoulson
Copy link
Collaborator

In addition to this change of "Shim" to "MokListRT" for the value of the UnicodeName field for EV_EFI_VARIABLE_AUTHORITY measurements of the built in CA when using a vendor cert, as a side effect of 092c2b2, there's another inconsistency that I think should be addressed.

For builds that are using a vendor DB rather than a single vendor cert, EV_EFI_VARIABLE_AUTHORITY measurements of CAs from it use "vendor_db" for the UnicodeName when I think it should probably also be "Shim", and they use EFI_IMAGE_SECURITY_DATABASE_GUID (which is reserved by UEFI for the UEFI defined signature databases) for the GUID when this should almost certainly be SHIM_GUID.

@vittyvk
Copy link
Contributor Author

vittyvk commented Jan 29, 2025

If we decide to change what/how gets measured into PCRs in the future versions of shim, it would be great to have an easy way to figure out how the particular shim binary behaves by observing something in shim binary. I would suggest we have a section with feature flags and every change in the measurements must have a corresponding one. This is important as there are tools out there (e.g. https://github.com/canonical/encrypt-cloud-image, https://gitlab.com/vkuznets/encrypt-rhel-image) which try to predict PCR value[s] and shim is an important part of the equation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants