[coreboot-gerrit] Change in coreboot[master]: security: vboot: Clarify PCR extension algorithms/sizes

[Julius Werner (Code Review)]

Attention is currently required from: Philipp Deppenwiese, Joel Kitching, Andrey Pronin, Werner Zeh.
Hello Philipp Deppenwiese, Joel Kitching, Andrey Pronin, Werner Zeh,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/51720

to review the following change.

Change subject: security: vboot: Clarify PCR extension algorithms/sizes
......................................................................

security: vboot: Clarify PCR extension algorithms/sizes

The PCR algorithms used for vboot are frequently causing confusion (e.g.
see CB:35645) because depending on the circumstances sometimes a
(zero-extended) SHA1 value is interpreted as a SHA256, and sometimes a
SHA256 is interpreted as a SHA1. We can't really "fix" anything here
because the resulting digests are hardcoded in many generations of
Chromebooks, but we can document and isolate it better to reduce
confusion. This patch adds an explanatory comment and fixes both
algorithms and size passed into the lower-level TPM APIs to their actual
values (whereas it previously still relied on the TPM 1.2 TSS not
checking the algorithm type, and the TPM 2.0 TSS only using the size
value for the TCPA log and not the actual TPM operation).

Signed-off-by: Julius Werner 
Change-Id: Ib0b6ecb8c7e9a405ae966f1049158f1d3820f7e2
---
M src/security/vboot/tpm_common.c
1 file changed, 18 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/51720/1

diff --git a/src/security/vboot/tpm_common.c b/src/security/vboot/tpm_common.c
index 783392c..7fb2a9d 100644
--- a/src/security/vboot/tpm_common.c
+++ b/src/security/vboot/tpm_common.c
@@ -31,15 +31,30 @@
    if (size < TPM_PCR_MINIMUM_DIGEST_SIZE)
    	return VB2_ERROR_UNKNOWN;

+	/*
+	 * On TPM 1.2, all PCRs are intended for use with SHA1. We truncate our
+	 * SHA256 HWID hash to 20 bytes to make it fit. On TPM 2.0, we always
+	 * want to use the SHA256 banks, even for the boot mode which is
+	 * technically a SHA1 value for historical reasons. vboot has already
+	 * zero-extended the buffer to 32 bytes for us, so we just take it like
+	 * that and pretend it's a SHA256. In practice, this means we never care
+	 * about the (*size) value returned from vboot (which indicates how many
+	 * significant bytes vboot wrote, although it always extends zeroes up
+	 * to the end of the buffer), we always use a hardcoded size instead.
+	 */
+	_Static_assert(sizeof(buffer) >= VB2_SHA256_DIGEST_SIZE,
+		       "Buffer needs to be able to fit at least a SHA256");
+	enum vb2_hash_algorithm algo = CONFIG(TPM1) ? VB2_HASH_SHA1 : VB2_HASH_SHA256;
+
    switch (which_digest) {
    /* SHA1 of (devmode|recmode|keyblock) bits */
    case BOOT_MODE_PCR:
-		return tpm_extend_pcr(pcr, VB2_HASH_SHA256, buffer, size,
+		return tpm_extend_pcr(pcr, algo, buffer, vb2_digest_size(algo),
    			      TPM_PCR_BOOT_MODE);
     /* SHA256 of HWID */
    case HWID_DIGEST_PCR:
-		return tpm_extend_pcr(pcr, VB2_HASH_SHA256, buffer,
-					  size, TPM_PCR_GBB_HWID_NAME);
+		return tpm_extend_pcr(pcr, algo, buffer, vb2_digest_size(algo),
+				      TPM_PCR_GBB_HWID_NAME);
    default:
    	return VB2_ERROR_UNKNOWN;
    }