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

Wip/hughsie/tpm event log replay #1735

Merged
merged 2 commits into from Feb 4, 2020
Merged

Conversation

@hughsie
Copy link
Collaborator

hughsie commented Jan 31, 2020

No description provided.

@hughsie hughsie added the enhancement label Jan 31, 2020
@hughsie hughsie requested a review from superm1 Jan 31, 2020
@hughsie hughsie self-assigned this Jan 31, 2020
@superm1

This comment has been minimized.

Copy link
Collaborator

superm1 commented Jan 31, 2020

It's probably worth taking this the step further of comparing that the calculated hash actually matches and making some noise if it doesn't.

@hughsie

This comment has been minimized.

Copy link
Collaborator Author

hughsie commented Jan 31, 2020

comparing that the calculated hash actually matches

I guess we should do that in tpm-eventlog rather than tpm as the latter might be neutered in favour of the dell on, right? The uefi plugin isn't logically enough connected to the TPM, although maybe we can use the registered() event? Ideas welcome.

@superm1

This comment has been minimized.

Copy link
Collaborator

superm1 commented Jan 31, 2020

Looks like one of the CI problems is probably this fix:
https://gitlab.gnome.org/GNOME/glib/merge_requests/1307

We can probably turn off -Wtypedef-redefinition for the failing builds for it for now.

@superm1

This comment has been minimized.

Copy link
Collaborator

superm1 commented Jan 31, 2020

I guess we should do that in tpm-eventlog rather than tpm as the latter might be neutered in favour of the dell on, right? The uefi plugin isn't logically enough connected to the TPM, although maybe we can use the registered() event? Ideas welcome.

I think it's most interesting in the uefi plugin. Let it get calculated in tpm event log plugin and let uefi pick up a signal and make noise when there's a problem then and they don't add up. That means that if you can't calculate event log (such as on Dell system that has secure boot turned off) you just don't send the signal to uefi plugin.

@hughsie

This comment has been minimized.

Copy link
Collaborator Author

hughsie commented Jan 31, 2020

I think it's most interesting in the uefi plugin.

I think I implemented it the other way around, but I don't think it matters much.

@hughsie

This comment has been minimized.

Copy link
Collaborator Author

hughsie commented Jan 31, 2020

With how scary this situation actually is

I don't know how scary it is. I guess the TPM could be doing the wrong thing in a non-scary way, but @mjg59 is the expert here.

I think it's worth also adding this into the daemon log too perhaps.

I don't think anyone looks at the daemon log in reality. If it's really that serious we ought to add a new FWUPD_DEVICE_FLAG_IS_BROKEN and provide a translation in the various UIs.

@mjg59

This comment has been minimized.

Copy link

mjg59 commented Jan 31, 2020

There are machines that only produce sha256 hashes, so I think you need to handle that case. I also haven't checked the backend code, so I don't know whether you handle TCG2 format event logs (which would be a prerequisite for the first point).

There are definitely firmwares out there that generate incomplete event logs and I think it's worth logging that, but right now I wouldn't recommend expressing that to the user until we have a reasonable idea of how common it is.

@hughsie

This comment has been minimized.

Copy link
Collaborator Author

hughsie commented Feb 1, 2020

There are machines that only produce sha256 hashes, so I think you need to handle that case.

Right, will fix.

so I don't know whether you handle TCG2 format event logs

We do!

There are definitely firmwares out there that generate incomplete event logs

Do you know why? e.g. out of space etc?

@hughsie hughsie force-pushed the wip/hughsie/tpm-event-log-replay branch from 5b07721 to f49eb69 Feb 1, 2020
@mjg59

This comment has been minimized.

Copy link

mjg59 commented Feb 1, 2020

Do you know why? e.g. out of space etc?

The most common example I've seen is measuring the TXT ACM but failing to include it in the event log.

@hughsie

This comment has been minimized.

Copy link
Collaborator Author

hughsie commented Feb 2, 2020

@superm1 do you want to re-review given i've rewritten a lot of the string handling?

@superm1
superm1 approved these changes Feb 3, 2020
Copy link
Collaborator

superm1 left a comment

I haven't tested it on an actual system that is populating an event log, but I think things look good right now, thanks.

hughsie added 2 commits Jan 31, 2020
This means we use half the amount of memory to store the event hashes, and also
means we can process the raw data in future patches without parsing back out
of ASCII format.
In theory, these should always match the reported PCRx values from the TPM.

If the reconstructed event log checksum does not match the TPM value then
something is either implemented wrongly, or something bad has happened.
@hughsie hughsie force-pushed the wip/hughsie/tpm-event-log-replay branch from f49eb69 to 2b967ae Feb 4, 2020
@hughsie hughsie merged commit 59d947a into master Feb 4, 2020
8 checks passed
8 checks passed
LGTM analysis: Python No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
ci/circleci: build-snap Your tests passed on CircleCI!
Details
ci/circleci: build-ubuntu-x86_64 Your tests passed on CircleCI!
Details
ci/circleci: build-windows Your tests passed on CircleCI!
Details
ci/circleci: check-abi Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@hughsie hughsie deleted the wip/hughsie/tpm-event-log-replay branch Feb 4, 2020
@superm1

This comment has been minimized.

Copy link
Collaborator

superm1 commented Feb 4, 2020

Just as an FYI, I did confirm on an XPS 7390 with secure boot enabled that this is working properly:

17:39:30:0409 FuPluginTpmEventlog  TPM reconstructed event log matched PCR0 reading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.