preinstall: Add initial checks for TCG log consistency with PCR values #319

chrisccoulson · 2024-07-31T23:08:07Z

This adds a function that verifies that the TCG log is well formed and
is consistent with the TPM's PCR values for at least one supported PCR
bank. On success, it returns the best PCR bank to use and the startup
locality, and any errors that occurred for non-mandatory PCRs. A PCR
might not be mandatory because we don't have a way to generate profiles
for it, or because the input flags to RunChecks marked it as optional.
The startup locality is used later on with the discrete TPM indication
returned from the earlier TPM check to customize the PCR profile.

It performs some other tests, notably that:

the TCG log contains a sequence of expected phases.
the TCG defined PCRs contain a EV_SEPARATOR event between the pre-OS
and OS-present environment (although the one in PCR7 separates secure
boot policy from secure boot authentication).
none of the EV_SEPARATORs in the TCG defined PCRs indicated that an
error occurred.
there are no pre-OS measurements to non-TCG defined PCRs (8-).

Future PRs will add more specific checks for each supported PCR.

This pulls in a newer go-tpm2 for TPM2_PCR_Allocate support in the
library and test harness. This is a function that's normally ever
called from firmware, but we need to be able to do what the firmware
does to enable/disable PCR banks here for unit testing to emulate
different firmware configurations.

This adds a function that verifies that the TCG log is well formed and is consistent with the TPM's PCR values for at least one supported PCR bank. On success, it returns the best PCR bank to use and the startup locality, and any errors that occurred for non-mandatory PCRs. A PCR might not be mandatory because we don't have a way to generate profiles for it, or because the input flags to RunChecks marked it as optional. The startup locality is used later on with the discrete TPM indication returned from the earlier TPM check to customize the PCR profile. It performs some other tests, notably that: - the TCG log contains a sequence of expected phases. - the TCG defined PCRs contain a EV_SEPARATOR event between the pre-OS and OS-present environment (although the one in PCR7 separates secure boot policy from secure boot authentication). - none of the EV_SEPARATORs in the TCG defined PCRs indicated that an error occurred. - there are no pre-OS measurements to non-TCG defined PCRs (8-). Future PRs will add more specific checks for each supported PCR. This pulls in a newer go-tpm2 for TPM2_PCR_Allocate support in the library and test harness. This is a function that's normally ever called from firmware, but we need to be able to do what the firmware does to enable/disable PCR banks here.

pedronis

did an initial pass

pedronis · 2024-09-05T15:46:37Z

internal/efitest/log.go

-		data := tcglog.StringEventData("1.0")
-		builder.hashLogExtendEvent(c, data, &logEvent{
+	if opts.StartupLocality == 4 {
+		ev := &tcglog.Event{


a brief comment describing what is happing here could be useful

I've added a comment to this.

pedronis · 2024-09-05T15:48:52Z

efi/fw_load_handler_test.go

+			{pcr: 0, eventType: mockPcrBranchExtendEvent, digest: testutil.DecodeHexString(c, "cd1137dfa2bfba51973100d73d78d9f496e089fd246fe980fadc668b4efc9443")},
+			{pcr: 0, eventType: mockPcrBranchExtendEvent, digest: testutil.DecodeHexString(c, "5ca5e6acb83d626a42b53ddc5a2fe04d6a4b2f045bb07f6d9baf0e82900d7bbe")},


are these because of the changes in efitest/log.go ?

pedronis · 2024-09-05T15:49:53Z

efi/preinstall/errors.go

+	return w.String()
+}
+
+// UNwrapBankError returns the error associated with the specified PCR bank if one


pedronis · 2024-09-05T16:00:43Z

efi/preinstall/check_tcglog.go

+				results.Lookup(ev.PCRIndex).setErr(errors.New("unexpected StartupLocality event (should be in PCR0)"))
+				continue
+			}
+			if !bytes.Equal(results.Lookup(0).logValue, results.Lookup(0).initialValue) {


shouldn't this check be a predicate on the struct?

Yes, they could be

pedronis · 2024-09-05T16:49:04Z

efi/preinstall/check_tcglog.go

+	return &tcglogPhaseTracker{
+		phase:             tcglogPhasePreOSBeforeMeasureSecureBootConfig,
+		numSeparatorsSeen: 0,
+		pcrSeparatorsSeen: make(map[tpm2.Handle]struct{}),


nitpick: we tend to just use bool and not empty struct for this kind of set structures

I can change that.

pedronis · 2024-09-05T16:54:14Z

efi/preinstall/check_tcglog.go

+			// An EV_SEPARATOR event in PCRs 0-6 after measuring the secure boot configuration begins the
+			// transition to OS-present, skipping the part of the pre-OS phase after the secure boot config
+			// has been measured.
+			t.phase = tcglogPhaseTransitioningToOSPresent


this case is not tested

Aha, I'll see if there's something I can do to test that. At the moment I'm just doing ad-hoc modifications to logs to break things.

Note that the next PR currently has quite a low coverage because it's got a switch statement with a lot of branches to cover all possible event types. We have tests for the most common types that we're likelly to see, but testing all of the others is going to require significant work in internal/efitest to customize the log and generate events with different types. I'm not sure whether that will be a blocker for the first iteration, especially as Oliver is keen to get this landed.

pedronis · 2024-09-05T16:57:37Z

efi/preinstall/check_tcglog.go

+		}
+	}
+	if !phaseTracker.reachedOSPresent() {
+		return nil, errors.New("reached the end of the log without seeing EV_SEPARATOR events in all TCG defined PCRs")


this error case is not tested

This one should be easy to figure out how to test.

I've added a test for this one now.

This reduces some code that's duplicated in a few places.

pedronis

thank you, should Mate or somebody else familiar with boot give a look a this?

chrisccoulson · 2024-09-23T14:38:13Z

thank you, should Mate or somebody else familiar with boot give a look a this?

Sure, I can ask @kukrimate if he would like to take a look at this too before we land it.

chrisccoulson force-pushed the preinstall-add-initial-tcglog-and-pcr-bank-select branch 4 times, most recently from 3b23972 to 28e0454 Compare August 7, 2024 15:35

chrisccoulson mentioned this pull request Aug 7, 2024

preinstall: Add simple checks for PCRs 0 and 2 #321

Closed

chrisccoulson force-pushed the preinstall-add-initial-tcglog-and-pcr-bank-select branch 6 times, most recently from 0c803fc to bbe4284 Compare August 17, 2024 12:12

chrisccoulson force-pushed the preinstall-add-initial-tcglog-and-pcr-bank-select branch 3 times, most recently from 2147496 to 64dc595 Compare August 21, 2024 07:50

chrisccoulson force-pushed the preinstall-add-initial-tcglog-and-pcr-bank-select branch from 64dc595 to e080181 Compare August 29, 2024 22:31

chrisccoulson force-pushed the preinstall-add-initial-tcglog-and-pcr-bank-select branch from e080181 to 9d73305 Compare August 29, 2024 22:33

chrisccoulson assigned pedronis Aug 29, 2024

chrisccoulson marked this pull request as ready for review September 2, 2024 14:09

chrisccoulson unassigned pedronis Sep 4, 2024

chrisccoulson requested a review from pedronis September 4, 2024 12:45

pedronis reviewed Sep 5, 2024

View reviewed changes

chrisccoulson added 4 commits September 20, 2024 14:25

internal/efitest: provide some extra comments

ba806f1

preinstall: fix a typo

7f13b02

preinstall: add pcrResults.extended helper

9012f92

This reduces some code that's duplicated in a few places.

preinstall: use bool rather than struct{} as a map value

17a9ff7

chrisccoulson requested a review from pedronis September 20, 2024 14:32

preinstall: improve test coverage

33e31fa

chrisccoulson mentioned this pull request Sep 20, 2024

preinstall: Add minimal checks for PCR 2 #334

Merged

pedronis approved these changes Sep 23, 2024

View reviewed changes

chrisccoulson merged commit a6c3ed5 into canonical:master Oct 1, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

preinstall: Add initial checks for TCG log consistency with PCR values #319

preinstall: Add initial checks for TCG log consistency with PCR values #319

chrisccoulson commented Jul 31, 2024 •

edited

Loading

pedronis left a comment

pedronis Sep 5, 2024

chrisccoulson Sep 20, 2024

pedronis Sep 5, 2024

chrisccoulson Sep 20, 2024

pedronis Sep 5, 2024

chrisccoulson Sep 20, 2024

pedronis Sep 5, 2024

chrisccoulson Sep 11, 2024

pedronis Sep 5, 2024

chrisccoulson Sep 11, 2024

pedronis Sep 5, 2024

chrisccoulson Sep 11, 2024

pedronis Sep 5, 2024

chrisccoulson Sep 11, 2024

chrisccoulson Sep 20, 2024

pedronis left a comment

chrisccoulson commented Sep 23, 2024

		{pcr: 0, eventType: mockPcrBranchExtendEvent, digest: testutil.DecodeHexString(c, "cd1137dfa2bfba51973100d73d78d9f496e089fd246fe980fadc668b4efc9443")},
		{pcr: 0, eventType: mockPcrBranchExtendEvent, digest: testutil.DecodeHexString(c, "5ca5e6acb83d626a42b53ddc5a2fe04d6a4b2f045bb07f6d9baf0e82900d7bbe")},

preinstall: Add initial checks for TCG log consistency with PCR values #319

preinstall: Add initial checks for TCG log consistency with PCR values #319

Conversation

chrisccoulson commented Jul 31, 2024 • edited Loading

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

chrisccoulson commented Sep 23, 2024

chrisccoulson commented Jul 31, 2024 •

edited

Loading