Attention is currently required from: Jakub Czapiga. Hello Jakub Czapiga,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/59982
to review the following change.
Change subject: cbfs: Enable CBFS verification Kconfigs ......................................................................
cbfs: Enable CBFS verification Kconfigs
With the elimination of remaining non-verifying CBFS APIs in CB:59682, CBFS verification is now ready to be used in its simplest form, so enable the respective Kconfig options in menuconfig. Add a few more restrictions to the TOCTOU_SAFETY option for problems that haven't been solved yet, and transform a comment in cbfs.c into a die() to make sure we don't accidentally forget implementing it once vboot integration gets added.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Ifeba5c962c943856ab79bc6c4cb90a60c1de4a60 --- M src/lib/Kconfig.cbfs_verification M src/lib/cbfs.c 2 files changed, 33 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/59982/1
diff --git a/src/lib/Kconfig.cbfs_verification b/src/lib/Kconfig.cbfs_verification index fa90d9d..33e5458 100644 --- a/src/lib/Kconfig.cbfs_verification +++ b/src/lib/Kconfig.cbfs_verification @@ -2,33 +2,41 @@ # # This file is sourced from src/security/Kconfig for menuconfig convenience.
-#menu "CBFS verification" # TODO: enable once it works +menu "CBFS verification"
config CBFS_VERIFICATION - bool # TODO: make user selectable once it works + bool "Enable CBFS verification" depends on !VBOOT_STARTS_BEFORE_BOOTBLOCK # this is gonna get tricky... select VBOOT_LIB help - Work in progress. Do not use (yet). + Say yes here to enable code that cryptographically verifies each CBFS + file as it gets loaded by chaining it to a trust anchor that is + embedded in the bootblock. This only makes sense if you use some + out-of-band mechanism to guarantee the integrity of the bootblock + itself, such as Intel BootGuard or flash write-protection. + + If a CBFS image was created with this option enabled, cbfstool will + automatically update the hash embedded in the bootblock whenever it + modifies the CBFS. + +if CBFS_VERIFICATION
config TOCTOU_SAFETY - bool - depends on CBFS_VERIFICATION + bool "Protect against time-of-check vs. time-of-use vulnerabilities" depends on !NO_FMAP_CACHE depends on !NO_CBFS_MCACHE depends on !USE_OPTION_TABLE && !FSP_CAR # Known to access CBFS before CBMEM init + depends on !VBOOT # TODO: can only allow this once vboot fully integrated + depends on NO_XIP_EARLY_STAGES help - Work in progress. Not actually TOCTOU safe yet. Do not use. + Say yes here to eliminate time-of-check vs. time-of-use vulnerabilities + for CBFS verification. This means that data from flash must be verified + every time it is loaded (not just the first time), which requires a bit + more overhead and is incompatible with certain configurations.
- Design idea here is that mcache overflows in this mode are only legal - for the RW CBFS, because it's relatively easy to retrieve the RW - metadata hash from persistent vboot context at any time, but the RO - metadata hash is lost after the bootblock is unloaded. This avoids the - need to carry yet another piece forward through the stages. Mcache - overflows are mostly a concern for RW updates (if an update adds more - files than originally planned for), for the RO section it should - always be possible to dimension the mcache correctly beforehand, so - this should be an acceptable limitation. + Using this option only makes sense when the mechanism securing the + bootblock is also safe against these vulnerabilities (i.e. there's no + point in enabling this when you just rely on flash write-protection).
config CBFS_HASH_ALGO int @@ -37,9 +45,13 @@ default 3 if CBFS_HASH_SHA512
choice - prompt "--> hash type" - depends on CBFS_VERIFICATION + prompt "Hash algorithm" default CBFS_HASH_SHA256 + help + Select the hash algorithm used in CBFS verification. Note that SHA-1 is + generally considered insecure today and should not be used without good + reason. When using CBFS verification together with measured boot, using + the same hash algorithm (usually SHA-256) for both is more efficient.
config CBFS_HASH_SHA1 bool "SHA-1" @@ -52,4 +64,6 @@
endchoice
-#endmenu +endif + +endmenu diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 991c9fe..3a044f7 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -57,7 +57,7 @@ RO CBFS would have been caught when building the mcache in cbfs_get boot_device(). (Note that TOCTOU_SAFETY implies !NO_CBFS_MCACHE.) */ assert(cbd == vboot_get_cbfs_boot_device()); - /* TODO: set metadata_hash to RW metadata hash here. */ + die("TODO: set metadata_hash to RW metadata hash here.\n"); } err = cbfs_lookup(&cbd->rdev, name, mdata, &data_offset, metadata_hash); }