preinstall: Add checks for platform firmware protections #316

chrisccoulson · 2024-07-10T23:04:55Z

This adds checks for how the platform firmware is protected, with
specific tests for whether CPU silicon debugging features are enabled or
not locked off, whether a firmware debugging endpoint (such as UDK) is
enabled, whether the kernel IOMMU is available, whether the firmware
indicates that DMA protection was disabled at some point, and whether
the firmware is protected by a hardware root-of-trust (ie, keys fused
into the silicon during manufacturing).

The main entry point to this is checkPlatformFirmwareProtections.
There is an implementation for amd64 go builds (x86-64) and then a null
implementation for everything that is !amd64 which just returns an error.

Some of these checks are based on the HSI checks in fwupd with some minor
adaptions, particularly for the BootGuard checks where there are some tests
in fwupd that disagree with other publicly available documentation which
better aligns with hardware that is assumed to be good (note that Intel do
not publicly document how any of this works).

This has a significant limitation today - it only works on Intel based
systems. AMD support will happen when we figure out how the platform
firmware is protected on AMD platforms with the dedicated PSP.

There are some other limitations, eg, whilst we detect that the IOMMU
functionality in the kernel is enabled, we don't check that external
facing DMA capable ports or user accessible internal DMA capable ports
are protected by one of the domains (and this feature could be made
optional if there aren't external facing or user-accessible internal
ports that are capable of DMA). Microsoft say that externally accessible
ports can be identified using ACPI tables (https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports), but we don't
do anything like this yet. I'm going to create a follow-on epic to expand support
for this feature, and this check will likely end up in there.

This does perform a check of the secure boot PCR for an event that is
not documented anywhere by the TCG, isn't in EDK2 or any of Microsoft's
public Project Mu repositories, but is a Windows requirement (measuring
a EV_EFI_ACTION event with the string "DMA Protection Disabled" to PCR7
if DMA protection is disabled by any settings in the firmware). Note
that strings associated with EV_EFI_ACTION events aren't meant to be
NULL terminated, although the one on my Dell XPS15 is so the code
handles the case where the event data is NULL terminated too. I don't
know if this is a Dell bug or if Microsoft generally expects this to be a NULL
terminated, but this contradicts the TCG spec for this event type. The only
places where NULL terminated strings are used are for EV_S_CRTM_CONTENTS
when measured by a H-CRTM event, and a NULL terminated UCS2 string in
the data associated with EV_S_CRTM_VERSION events (yay for consistency!)

This adds checks for how the platform firmware is protected, with specific tests for whether CPU silicon debugging features are enabled or not locked off, whether a firmware debugging endpoint (such as UDK) is enabled, whether the kernel IOMMU is available, whether the firmware indicates that DMA protection was disabled at some point, and whether the firmware is protected by a hardware root-of-trust (ie, keys fused into the silicon during manufacturing). Some of these checks are based on the HSI checks in fwupd with some minor adaptions, particularly for the BootGuard checks where there are some tests in fwupd that disagree with other publicly available documentation which better aligns with hardware that is assumed to be good (note that Intel do not publicly document how any of this works). This has a significant limitation today - it only works on Intel based systems. AMD support will happen when we figure out how the platform firmware is protected on AMD platforms with the dedicated PSP. There are some other limitations, eg, whilst we detect that the IOMMU functionality in the kernel is enabled, we don't check that external facing DMA capable ports or user accessible internal DMA capable ports are protected by one of the domains (and this feature could be made optional if there aren't external facing or user-accessible internal ports that aren't capable of DMA). This does perform a check of the secure boot PCR for an event that is not documented anywhere by the TCG, isn't in EDK2 or any of Microsoft's public Project Mu repositories, but is a Windows requirement (measuring a EV_EFI_ACTION event with the string "DMA Protection Disabled" to PCR7 if DMA protection is disabled by any settings in the firmware). Note that strings associated with EV_EFI_ACTION events aren't meant to be NULL terminated, although the one on my Dell XPS15 is so the code handles the case where the event data is NULL terminated too.

pedronis

this looks reasonable to me, did Mate review this?

pedronis · 2024-08-09T15:32:10Z

efi/preinstall/check_fw_protections_null.go

+)
+
+func checkPlatformFirmwareProtections(env HostEnvironment, log *tcglog.Log) (result firmwareProtectionResultFlags, err error) {
+	return 0, errors.New("checking platform firmware protections is not implemented on this platform")


shouldn't the not implemented error be recognisable?

I've added a new error type UnsupportedPlatformError for this.

pedronis · 2024-08-09T15:32:32Z

efi/preinstall/check_fw_protections_amd64.go

+			result |= platformFirmwareProtectionsTPMLocality3IsProtected
+		}
+	case cpuVendorAMD:
+		return 0, errors.New("TODO: checking platform firmware protections is not yet implemented for AMD")


See previous response.

chrisccoulson · 2024-08-13T14:20:00Z

@kukrimate

chrisccoulson · 2024-08-13T21:36:31Z

I've addressed the minor comments about distinguishing errors, so I've popped it back in your queue @pedronis. We can wait for some comments from Mate, although a lot of this was developed in consultation with him anyway (particularly the BootGuard bits - thanks Mate!) so none of what is in this PR should be too much of a surprise.

This only had a single value. Return a protected localities result instead.

chrisccoulson · 2024-08-14T07:25:47Z

I pushed a small change to this to remove the flags returned from the main entry point, checkPlatformFirmwareProtections, as there was only a single flag. It returns a mask of
protected TPM localities now (based on my assumptions from reading the Intel TXT
documentation), the value of which will be used later on as part of a discrete TPM reset
mitigation.

kukrimate

Note that I haven't tested this code on my own hardware, this review is only based on reading it.

Left a couple notes, but overall seems good to me and implements what Chris was telling me about earlier.

kukrimate · 2024-08-14T09:42:28Z

efi/preinstall/check_fw_protections.go

+// On Intel devices, the domains are defined by the DMAR ACPI table. The check is quite
+// simple, and based on the fwupd HSI checks.
+// XXX: Figure out whether this is genuinely sufficient, eg:
+//   - Should we only mandate this if there are externally facing ports, or internal ports


If we believe that attacks against TPM FDE involving physical access are relevant to our threat model, then we must by definition consider DMA via internal ports a problem.
Based on our advocacy for BootGuard, this very much seems to be the case, replacing the contents of an SPI flash device isn't really a lower barrier that needs more protection than DMA via an internal PCIe connector.

kukrimate · 2024-08-14T09:57:48Z

efi/preinstall/check_fw_protections_intel.go

+		return &NoHardwareRootOfTrustError{errors.New("BootGuard is disabled")}
+	}
+
+	// Verify the BootGuard profile


BootGuard FVME profile is okay from a security perspective so +1 on that.

I think supporting measured boot would be nice too, but this is okay, and most devices use this.

I was thinking about adding support for a measured boot profile as well (the consequence of this is that we bring PCR0 into the profile), but perhaps only on firmware TPMs?

In light of the GPIO problem and Intel doing nothing about it software-wise, yes that would make sense to me, at least temporarily.
I have heard (unverified) claims that some very new systems might have TPM reset wired in such a way that it isn't shared with GPIO.
The usual hardware attacks against discrete TPMs exist as always, but those are not mitigated by verified boot either.

Thanks. If we get a generation of devices with discrete TPMs with reset signals that are tied to the host CPU reset and can't be controlled via GPIO, then we can add extra checks in the future to turn off the reset mitigation on those platforms (which is just adding PCR0 to the profile if TPM2_Startup came from locality 3 or if there was a H-CRTM event sequence, which requires locality 4 access, both of which change the initial value of PCR0).

pedronis

thanks, some small comments and a comment about errors/error definitions vs future handling needs for them

pedronis · 2024-08-16T11:08:43Z

efi/preinstall/check_fw_protections_amd64.go

@@ -98,7 +98,19 @@ func checkPlatformFirmwareProtections(env internal_efi.HostEnvironment, log *tcg
 			return 0, fmt.Errorf("encountered an error when determining platform firmware protections using Intel MEI: %w", err)
 		}
 		if amd64Env.HasCPUIDFeature(cpuid.SMX) {
-			// The Intel TXT spec says that locality 3 is only available to ACMs
+			// The Intel TXT spec says that locality 4 is basically only avilable


typo: available

I've fixed this now.

pedronis · 2024-08-16T11:08:52Z

efi/preinstall/check_fw_protections_amd64.go

+			// has access to locality 3. Access to this is meant to be locked at the
+			// hardware level before running non-Intel code, although I'm not sure if
+			// this is only relevant in the D-CRTM case where the SINIT ACM has access
+			//to locality 3, and it locks access to it, leaving access to localities 2


strange formatting

It looks like this one was already fixed in a recent push I made

pedronis · 2024-08-16T11:11:35Z

efi/preinstall/check_fw_protections.go

+	internal_efi "github.com/snapcore/secboot/internal/efi"
+)
+
+// NoHardwareRootOfTrustError is returned wrapped from [RunChecks] if the platform


as mentioned in London my preference would be for all the errors that RunChecks can return to live in a single errors.go file as we will have to work out at a later point a mechanism to expose them to clients of snapd, and we would also like an easy way to point also people to the full list

I've moved errors to a new errors.go now.

pedronis

thank you

chrisccoulson force-pushed the preinstall-add-check-fw-protection branch from 8b04c23 to fc155fd Compare July 26, 2024 12:31

chrisccoulson marked this pull request as ready for review July 26, 2024 12:35

chrisccoulson requested a review from pedronis July 26, 2024 12:35

chrisccoulson mentioned this pull request Jul 30, 2024

preinstall: Add checks for TPM2 device #318

Merged

chrisccoulson force-pushed the preinstall-add-check-fw-protection branch from fc155fd to de39165 Compare August 7, 2024 15:29

pedronis reviewed Aug 9, 2024

View reviewed changes

preinstall: Add an UnsupportedPlatformError type

2c66c8e

chrisccoulson requested a review from pedronis August 13, 2024 21:34

chrisccoulson added 2 commits August 13, 2024 22:51

preinstall: explain the check for SMX on Intel

893f028

preinstall: remove the flags result from checkPlatformFirmwareProtection

ea45bbc

This only had a single value. Return a protected localities result instead.

kukrimate approved these changes Aug 14, 2024

View reviewed changes

pedronis reviewed Aug 16, 2024

View reviewed changes

preinstall: fix a typo

4cafa96

chrisccoulson requested a review from pedronis August 16, 2024 17:11

preinstall: move errors to their own file

b353807

pedronis approved these changes Aug 19, 2024

View reviewed changes

chrisccoulson merged commit b48732e into canonical:master Aug 20, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

preinstall: Add checks for platform firmware protections #316

preinstall: Add checks for platform firmware protections #316

chrisccoulson commented Jul 10, 2024 •

edited

Loading

pedronis left a comment

pedronis Aug 9, 2024

chrisccoulson Aug 13, 2024

pedronis Aug 9, 2024

chrisccoulson Aug 13, 2024

chrisccoulson commented Aug 13, 2024

chrisccoulson commented Aug 13, 2024

chrisccoulson commented Aug 14, 2024

kukrimate left a comment

kukrimate Aug 14, 2024

kukrimate Aug 14, 2024

chrisccoulson Aug 14, 2024

kukrimate Aug 14, 2024 •

edited

Loading

chrisccoulson Aug 14, 2024

pedronis left a comment

pedronis Aug 16, 2024

chrisccoulson Aug 16, 2024

pedronis Aug 16, 2024

chrisccoulson Aug 16, 2024

pedronis Aug 16, 2024

chrisccoulson Aug 16, 2024

pedronis left a comment

preinstall: Add checks for platform firmware protections #316

preinstall: Add checks for platform firmware protections #316

Conversation

chrisccoulson commented Jul 10, 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

chrisccoulson commented Aug 13, 2024

chrisccoulson commented Aug 13, 2024

chrisccoulson commented Aug 14, 2024

kukrimate 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

kukrimate Aug 14, 2024 • edited Loading

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

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 Jul 10, 2024 •

edited

Loading

kukrimate Aug 14, 2024 •

edited

Loading