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

Secure boot rollback fixes #3422

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Secure boot rollback fixes #3422

wants to merge 4 commits into from

Conversation

jakogut
Copy link
Contributor

@jakogut jakogut commented May 13, 2024


Contributor checklist

Reviewer Guidelines

  • When submitting a review, please pick:
    • 'Approve' if this change would be acceptable in the codebase (even if there are minor or cosmetic tweaks that could be improved).
    • 'Request Changes' if this change would not be acceptable in our codebase (e.g. bugs, changes that will make development harder in future, security/performance issues, etc).
    • 'Comment' if you don't feel you have enough information to decide either way (e.g. if you have major questions, or you don't understand the context of the change sufficiently to fully review yourself, but want to make a comment)

@flowzone-app flowzone-app bot enabled auto-merge May 13, 2024 16:33
@jakogut jakogut force-pushed the sb-rollback-fixes branch 3 times, most recently from 65df818 to 2e4b0ad Compare May 16, 2024 17:37
@jakogut jakogut requested review from alexgg and mtoman May 16, 2024 21:11
Copy link
Contributor

@alexgg alexgg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of high level comments, let's wait for Michal to comment too.

In commit 1c19ebb, we append digests from the TPM event log
corresponding to events that are logged before EV_SEPARATOR. For
instance, parsing the event log on a typical system for event types, the
output looks like this:

EV_EFI_VARIABLE_DRIVER_CONFIG (SecureBoot)
EV_EFI_VARIABLE_DRIVER_CONFIG (PK)
EV_EFI_VARIABLE_DRIVER_CONFIG (KEK)
EV_EFI_VARIABLE_DRIVER_CONFIG (db)
EV_EFI_VARIABLE_DRIVER_CONFIG (dbx)
EV_SEPARATOR

This system requires no merging of event log digests.

On systems that measure EFI binaries (mostly only QEMU w/ edk2), we also
get this:

EV_EFI_VARIABLE_DRIVER_CONFIG (SecureBoot)
EV_EFI_VARIABLE_DRIVER_CONFIG (PK)
EV_EFI_VARIABLE_DRIVER_CONFIG (KEK)
EV_EFI_VARIABLE_DRIVER_CONFIG (db)
EV_EFI_VARIABLE_DRIVER_CONFIG (dbx)
EV_SEPARATOR
EV_EFI_VARIABLE_AUTHORITY (bootx64.efi)
EV_EFI_VARIABLE_AUTHORITY (bzImage)

Again, this requires no merging. We compute the signature of the
relevant EFI binaries ourselves, as they're subject to change during
hostapp-update.

However, we've also seen event logs like this:

EV_EFI_VARIABLE_DRIVER_CONFIG (SecureBoot)
EV_EFI_VARIABLE_DRIVER_CONFIG (PK)
EV_EFI_VARIABLE_DRIVER_CONFIG (KEK)
EV_EFI_VARIABLE_DRIVER_CONFIG (db)
EV_EFI_VARIABLE_DRIVER_CONFIG (dbx)
EV_EFI_ACTION (DMA Protection Disabled)
EV_SEPARATOR

This case is the one we've handled previously, by reading from the event
log and appending event digests before EV_SEPARATOR. We stopped at
EV_SEPARATOR because we weren't parsing event types previously, and this
digest is a constant that's easily recognized. However, we've since
encountered systems that have unexpected events *after* EV_SEPARATOR, as
shown below.

EV_EFI_VARIABLE_DRIVER_CONFIG (SecureBoot)
EV_EFI_VARIABLE_DRIVER_CONFIG (PK)
EV_EFI_VARIABLE_DRIVER_CONFIG (KEK)
EV_EFI_VARIABLE_DRIVER_CONFIG (db)
EV_EFI_VARIABLE_DRIVER_CONFIG (dbx)
EV_SEPARATOR
Unknown event type (?!)

In order to handle this, parse digests and event types into temporary
files and iterate through them together. We only stop appending digests
from the event log when the next event type is EV_EFI_VARIABLE_AUTHORITY
(EFI binary signature) or we hit the end of the list. This should
account for all possible variations.

Change-type: patch
Signed-off-by: Joseph Kogut <joseph@balena.io>
When rollback-health runs, a failing healthcheck causes the
hostapp-update hooks to be run from the inactive partition, to make the
inactive system bootable again.

The 0-signed-update hook, which updates the sealing policy for secure
boot enabled systems, reads from the securityfs mounted at
/sys/kernel/security in order to parse the TPM event log.

If this filesystem isn't mounted, the hook will improperly detect that
the TPM event log isn't available, and unneccessarily create a combined
policy when a single PCR policy would suffice.

Mount this filesystem in old_rootfs before chrooting to fix this.

Change-type: patch
Signed-off-by: Joseph Kogut <joseph@balena.io>
Some hooks, such as 0-signed-update, will attempt to read files from the
EFI system partition, such as combined policy binaries.

Bind mount the EFI partition into old_rootfs before running hooks to
ensure this is available.

Change-type: patch
Signed-off-by: Joseph Kogut <joseph@balena.io>
When reading from efi variables in hostapp-update hooks during rollback,
tcgtool will improperly read zero bytes from efivar files. This results
in an improper calculation of the PCR 7 digest, and an unbootable
system.

Read the file contents, skipping the first four bytes that are
attributes, and pipe the data directly to tcgtool to work around this.

Change-type: patch
Signed-off-by: Joseph Kogut <joseph@balena.io>
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

Successfully merging this pull request may close these issues.

None yet

3 participants