Attention is currently required from: Patrick Georgi.
Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79095?usp=email )
Change subject: Documentation: Describe how SMMSTORE can be used safely ......................................................................
Patch Set 2: Code-Review+1
(5 comments)
Patchset:
PS2: Looks pretty good to me. From "the boot process ... needs to be trusted," it generally seems like the intention is to use the root of trust for security, rather than isolation inside SMRAM. It's certainly not a bad idea, I like it for vboot, which has a fixed set of keys.
(Whether we can make that work for secure boot, I'm not sure. Physical attackers are often left out of the attack model, so overwriting native UEFI's variable store offline is ignored, I suppose. But just provisioning keys into a cleared variable store is not enough, because an attacker could *patch*, instead of clear, the variable store).
File Documentation/drivers/smmstore.md:
https://review.coreboot.org/c/coreboot/+/79095/comment/9d330d91_8a0faa93 : PS2, Line 145: - while doing so, process authentication data and reject invalid blocks; So, I think that UEFI's variable store is authenticated for writes, not reads. On initialisation, I can see UEFI code attempting to determine if secure boot is enabled by looking for the existence of a variable, but not validating it. I suppose self-validation is a tricky question: if something can write over this, it can also make sure it's valid (of course, the key will be different, that's where measurement helps a bit). Perhaps I missed something, UEFI's variable stack is complicated.
Anyways, not an issue with this document.
https://review.coreboot.org/c/coreboot/+/79095/comment/011d8668_b3398132 : PS2, Line 171: An attacker could mess with future calls into the APIs, but they : can already do so: Other common APIs for boot level variable are : implemented in RAM as well, so they can easily be defused. nit: variables? Also, are you thinking of coreboot CMOS or UEFI variables here? Or both, I suppose.
UEFI's native implementation in SMM is secure against this, at least for writes. If only SMM can write to SPI flash, and verification && write happen together in SMRAM, then verification can't be bypassed (at least online. A physically present attacker can write to this region offline).
Perhaps a mention that this point depends on addressing the next one would be helpful?
https://review.coreboot.org/c/coreboot/+/79095/comment/441f3e77_35e02a81 : PS2, Line 174: An attacker could flush the store with the CLEAR command. This is a : potential problem if the "empty state" is somehow less secure than : a fully configured system. This is the case for secure boot. For a simple boolean (like BIOS lock), I have and would default to secure settings when variables can't be found (at least for production code), but secure boot can't enable itself without keys.
We'd solve part of the problem by storing default keys in SPI flash to use when the store is empty, but that wouldn't stop an attacker from overwriting keys. Users are allowed to do this. There would be a change in the data measured into the TPM, though whether that's good enough security... I have no idea. Servers and business PCs may have attestation, but clients do not.
Again, not a issue here.
https://review.coreboot.org/c/coreboot/+/79095/comment/48f07427_a462cf6f : PS2, Line 177: As a remedy, CLEAR could be disabled after the initial repacking, : within the boot process, so that SMMSTORE becomes an append-only : store. In this case, the attacker could fill up the buffer, leading : to a DoS of the variable store until it's repacked. As described : earlier, once there's an attacker on the system, the variable store : lost its function until the attacker has been evicted, anyway. Raw WRITE has to be protected to, because writing 0xFF is equivalent to clearing. If someone were to do this, they might want to pass some range of memory that should be protected against writes. "Append-only" probably hints at that to future readers.