Andrey Pronin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46772 )
Change subject: security/vboot: fix policy digest for nvmem spaces ......................................................................
security/vboot: fix policy digest for nvmem spaces
This CL fixes the policy digest that restricts deleting the nvmem spaces to specific PCR0 states for Chrome OS case. For the general case, the CL captures the issues with the currently used digest in the comment. Fixing the general case is only possible after understanding in which states the nvmem spaces should be deletable, so this CL doesn't attempt to fix it.
BRANCH=none BUG=b:140958855 TEST=verified that nvmem spaces created with this digest can be deleted in the intended states, and cannot be deleted in other states (test details for ChromeOS - in BUG comments).
Change-Id: I3cb7d644fdebda71cec3ae36de1dc76387e61ea7 --- M src/security/vboot/secdata_tpm.c 1 file changed, 37 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/46772/1
diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c index 65d9c83..bc3d982 100644 --- a/src/security/vboot/secdata_tpm.c +++ b/src/security/vboot/secdata_tpm.c @@ -108,14 +108,47 @@ .TPMA_NV_PLATFORMCREATE = 1, };
+#if CONFIG(CHROMEOS) +/* + * This policy digest was obtained using TPM2_PolicyOR on 3 digests + * corresponding to a sequence of + * -) TPM2_PolicyCommandCode(TPM_CC_NV_UndefineSpaceSpecial), + * -) TPM2_PolicyPCR(PCR0, <extended_value>). + * where <extended value> is + * 1) all zeros = initial, unextended state, + * 2) result of extending (SHA1(0x00|0x01|0x00) | 00s to SHA256 size), + * 3) result of extending (SHA1(0x01|0x01|0x00) | 00s to SHA256 size). + * Values #2 and #3 correspond to two forms of recovery mode as extended by + * vb2api_get_pcr_digest(). + * As a result, the digest allows deleting the space with UndefineSpaceSpecial + * at early RO stages (before extending PCR0) or from recovery mode. + */ +static const uint8_t pcr0_allowed_policy[] = { + 0x44, 0x44, 0x79, 0x00, 0xCB, 0xB8, 0x3F, 0x5B, 0x15, 0x76, 0x56, + 0x50, 0xEF, 0x96, 0x98, 0x0A, 0x2B, 0x96, 0x6E, 0xA9, 0x09, 0x04, + 0x4A, 0x01, 0xB8, 0x5F, 0xA5, 0x4A, 0x96, 0xFC, 0x59, 0x84}; +#else /* * This policy digest was obtained using TPM2_PolicyPCR * selecting only PCR_0 with a value of all zeros. + * + * TODO: Fixed for ChromeOS above, need to fix for the general case depending + * on the actual use. Issues with the current digest: + * 1) The actual digest for a single PolicyPCR(PCR0, 00..00) + * is 0x09, 0x3C, 0xEB, ... + * and not 0x09, 0x93, 0x3C, 0xCE, 0xEB, ... + * 2) UndefineSpaceSpecial uses ADMIN auth role for the index and thus + * requires that the used policy contains a command that restricts command + * code (to TPM_CC_NV_UndefineSpaceSpecial in this case). + * The currently used digest works as arbitrary value that doesn't have a known + * sequence of policy commands leading to it, essentially preventing undefining + * the space. */ -static const uint8_t pcr0_unchanged_policy[] = { +static const uint8_t pcr0_allowed_policy[] = { 0x09, 0x93, 0x3C, 0xCE, 0xEB, 0xB4, 0x41, 0x11, 0x18, 0x81, 0x1D, 0xD4, 0x47, 0x78, 0x80, 0x08, 0x88, 0x86, 0x62, 0x2D, 0xD7, 0x79, 0x94, 0x46, 0x62, 0x26, 0x68, 0x8E, 0xEE, 0xE6, 0x6A, 0xA1}; +#endif /* CONFIG(CHROMEOS) */
/* Nothing special in the TPM2 path yet. */ static uint32_t safe_write(uint32_t index, const void *data, uint32_t length) @@ -154,7 +187,7 @@ { return set_space("firmware", FIRMWARE_NV_INDEX, firmware_blob, VB2_SECDATA_FIRMWARE_SIZE, ro_space_attributes, - pcr0_unchanged_policy, sizeof(pcr0_unchanged_policy)); + pcr0_allowed_policy, sizeof(pcr0_allowed_policy)); }
static uint32_t set_kernel_space(const void *kernel_blob) @@ -167,8 +200,8 @@ { if (index == MRC_REC_HASH_NV_INDEX) { return set_space("RO MRC Hash", index, data, HASH_NV_SIZE, - ro_space_attributes, pcr0_unchanged_policy, - sizeof(pcr0_unchanged_policy)); + ro_space_attributes, pcr0_allowed_policy, + sizeof(pcr0_allowed_policy)); } else { return set_space("RW MRC Hash", index, data, HASH_NV_SIZE, rw_space_attributes, NULL, 0);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46772 )
Change subject: security/vboot: fix policy digest for nvmem spaces ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46772/1/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46772/1/src/security/vboot/secdata_... PS1, Line 114: * corresponding to a sequence of trailing whitespace
https://review.coreboot.org/c/coreboot/+/46772/1/src/security/vboot/secdata_... PS1, Line 134: * trailing whitespace
https://review.coreboot.org/c/coreboot/+/46772/1/src/security/vboot/secdata_... PS1, Line 137: * 1) The actual digest for a single PolicyPCR(PCR0, 00..00) trailing whitespace
https://review.coreboot.org/c/coreboot/+/46772/1/src/security/vboot/secdata_... PS1, Line 139: * and not 0x09, 0x93, 0x3C, 0xCE, 0xEB, ... trailing whitespace
Hello build bot (Jenkins), Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46772
to look at the new patch set (#2).
Change subject: security/vboot: fix policy digest for nvmem spaces ......................................................................
security/vboot: fix policy digest for nvmem spaces
This CL fixes the policy digest that restricts deleting the nvmem spaces to specific PCR0 states for Chrome OS case. For the general case, the CL captures the issues with the currently used digest in the comment. Fixing the general case is only possible after understanding in which states the nvmem spaces should be deletable, so this CL doesn't attempt to fix it.
BRANCH=none BUG=b:140958855 TEST=verified that nvmem spaces created with this digest can be deleted in the intended states, and cannot be deleted in other states (test details for ChromeOS - in BUG comments).
Change-Id: I3cb7d644fdebda71cec3ae36de1dc76387e61ea7 --- M src/security/vboot/secdata_tpm.c 1 file changed, 37 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/46772/2
Hello build bot (Jenkins), Julius Werner, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46772
to look at the new patch set (#3).
Change subject: security/vboot: fix policy digest for nvmem spaces ......................................................................
security/vboot: fix policy digest for nvmem spaces
This CL fixes the policy digest that restricts deleting the nvmem spaces to specific PCR0 states for Chrome OS case. For the general case, the CL captures the issues with the currently used digest in the comment. Fixing the general case is only possible after understanding in which states the nvmem spaces should be deletable, so this CL doesn't attempt to fix it.
BRANCH=none BUG=b:140958855 TEST=verified that nvmem spaces created with this digest can be deleted in the intended states, and cannot be deleted in other states (test details for ChromeOS - in BUG comments).
Change-Id: I3cb7d644fdebda71cec3ae36de1dc76387e61ea7 Signed-off-by: Andrey Pronin apronin@chromium.org --- M src/security/vboot/secdata_tpm.c 1 file changed, 37 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/46772/3
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46772 )
Change subject: security/vboot: fix policy digest for nvmem spaces ......................................................................
Patch Set 3:
(2 comments)
Thanks!
https://review.coreboot.org/c/coreboot/+/46772/3/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46772/3/src/security/vboot/secdata_... PS3, Line 111: #if CONFIG(CHROMEOS) You don't need to add this, just update the code with the new version. Recovery mode is a general vboot concept and the digests used are the same with and without CONFIG(CHROMEOS). If anyone ever has a need to use a different policy here we can add a new Kconfig for that later, but for now we should reflect the policy that was originally intended here (and that's still used by the TPM1 code), which is to allow deleting this space in any flavor of recovery mode. (This is also consistent with the fact that we don't lock the spaces in recovery mode and don't disable platform hierarchy, regardless of whether CONFIG(CHROMEOS) is enabled or not.)
https://review.coreboot.org/c/coreboot/+/46772/3/src/security/vboot/secdata_... PS3, Line 118: * 1) all zeros = initial, unextended state, nit: Just to make things easier to reconstruct for people maybe also write each result digest here (because those are the inputs you need for trunks_client and presumably most similar tools)?
Hello build bot (Jenkins), Julius Werner, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46772
to look at the new patch set (#4).
Change subject: security/vboot: fix policy digest for nvmem spaces ......................................................................
security/vboot: fix policy digest for nvmem spaces
This CL fixes the policy digest that restricts deleting the nvmem spaces to specific PCR0 states for Chrome OS case. For the general case, the CL captures the issues with the currently used digest in the comment. Fixing the general case is only possible after understanding in which states the nvmem spaces should be deletable, so this CL doesn't attempt to fix it.
BRANCH=none BUG=b:140958855 TEST=verified that nvmem spaces created with this digest can be deleted in the intended states, and cannot be deleted in other states (test details for ChromeOS - in BUG comments).
Change-Id: I3cb7d644fdebda71cec3ae36de1dc76387e61ea7 Signed-off-by: Andrey Pronin apronin@chromium.org --- M src/security/vboot/secdata_tpm.c 1 file changed, 37 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/46772/4
Andrey Pronin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46772 )
Change subject: security/vboot: fix policy digest for nvmem spaces ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46772/3/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/46772/3/src/security/vboot/secdata_... PS3, Line 111: #if CONFIG(CHROMEOS)
You don't need to add this, just update the code with the new version. […]
Done
https://review.coreboot.org/c/coreboot/+/46772/3/src/security/vboot/secdata_... PS3, Line 118: * 1) all zeros = initial, unextended state,
nit: Just to make things easier to reconstruct for people maybe also write each result digest here ( […]
Done
Hello build bot (Jenkins), Julius Werner, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46772
to look at the new patch set (#5).
Change subject: security/vboot: fix policy digest for nvmem spaces ......................................................................
security/vboot: fix policy digest for nvmem spaces
This CL fixes the policy digest that restricts deleting the nvmem spaces to specific PCR0 states.
BRANCH=none BUG=b:140958855 TEST=verified that nvmem spaces created with this digest can be deleted in the intended states, and cannot be deleted in other states (test details for ChromeOS - in BUG comments).
Change-Id: I3cb7d644fdebda71cec3ae36de1dc76387e61ea7 Signed-off-by: Andrey Pronin apronin@chromium.org --- M src/security/vboot/secdata_tpm.c 1 file changed, 37 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/46772/5
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46772 )
Change subject: security/vboot: fix policy digest for nvmem spaces ......................................................................
Patch Set 5: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46772 )
Change subject: security/vboot: fix policy digest for nvmem spaces ......................................................................
security/vboot: fix policy digest for nvmem spaces
This CL fixes the policy digest that restricts deleting the nvmem spaces to specific PCR0 states.
BRANCH=none BUG=b:140958855 TEST=verified that nvmem spaces created with this digest can be deleted in the intended states, and cannot be deleted in other states (test details for ChromeOS - in BUG comments).
Change-Id: I3cb7d644fdebda71cec3ae36de1dc76387e61ea7 Signed-off-by: Andrey Pronin apronin@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/46772 Reviewed-by: Julius Werner jwerner@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/security/vboot/secdata_tpm.c 1 file changed, 37 insertions(+), 9 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c index 65d9c83..0e14575 100644 --- a/src/security/vboot/secdata_tpm.c +++ b/src/security/vboot/secdata_tpm.c @@ -109,13 +109,41 @@ };
/* - * This policy digest was obtained using TPM2_PolicyPCR - * selecting only PCR_0 with a value of all zeros. + * This policy digest was obtained using TPM2_PolicyOR on 3 digests + * corresponding to a sequence of + * -) TPM2_PolicyCommandCode(TPM_CC_NV_UndefineSpaceSpecial), + * -) TPM2_PolicyPCR(PCR0, <extended_value>). + * where <extended value> is + * 1) all zeros = initial, unextended state: + * - Value to extend to initial PCR0: + * <none> + * - Resulting PCR0: + * 0000000000000000000000000000000000000000000000000000000000000000 + * - Policy digest for PolicyCommandCode + PolicyPCR: + * 4B44FC4192DB5AD7167E0135708FD374890A06BFB56317DF01F24F2226542A3F + * 2) result of extending (SHA1(0x00|0x01|0x00) | 00s to SHA256 size) + * - Value to extend to initial PCR0: + * 62571891215b4efc1ceab744ce59dd0b66ea6f73000000000000000000000000 + * - Resulting PCR0: + * 9F9EA866D3F34FE3A3112AE9CB1FBABC6FFE8CD261D42493BC6842A9E4F93B3D + * - Policy digest for PolicyCommandCode + PolicyPCR: + * CB5C8014E27A5F7586AAE42DB4F9776A977BCBC952CA61E33609DA2B2C329418 + * 3) result of extending (SHA1(0x01|0x01|0x00) | 00s to SHA256 size) + * - Value to extend to initial PCR0: + * 47ec8d98366433dc002e7721c9e37d5067547937000000000000000000000000 + * - Resulting PCR0: + * 2A7580E5DA289546F4D2E0509CC6DE155EA131818954D36D49E027FD42B8C8F8 + * - Policy digest for PolicyCommandCode + PolicyPCR: + * E6EF4F0296AC3EF0F53906480985B1BE8058E0E517E5F74A5B8A415EFE339D87 + * Values #2 and #3 correspond to two forms of recovery mode as extended by + * vb2api_get_pcr_digest(). + * As a result, the digest allows deleting the space with UndefineSpaceSpecial + * at early RO stages (before extending PCR0) or from recovery mode. */ -static const uint8_t pcr0_unchanged_policy[] = { - 0x09, 0x93, 0x3C, 0xCE, 0xEB, 0xB4, 0x41, 0x11, 0x18, 0x81, 0x1D, - 0xD4, 0x47, 0x78, 0x80, 0x08, 0x88, 0x86, 0x62, 0x2D, 0xD7, 0x79, - 0x94, 0x46, 0x62, 0x26, 0x68, 0x8E, 0xEE, 0xE6, 0x6A, 0xA1}; +static const uint8_t pcr0_allowed_policy[] = { + 0x44, 0x44, 0x79, 0x00, 0xCB, 0xB8, 0x3F, 0x5B, 0x15, 0x76, 0x56, + 0x50, 0xEF, 0x96, 0x98, 0x0A, 0x2B, 0x96, 0x6E, 0xA9, 0x09, 0x04, + 0x4A, 0x01, 0xB8, 0x5F, 0xA5, 0x4A, 0x96, 0xFC, 0x59, 0x84};
/* Nothing special in the TPM2 path yet. */ static uint32_t safe_write(uint32_t index, const void *data, uint32_t length) @@ -154,7 +182,7 @@ { return set_space("firmware", FIRMWARE_NV_INDEX, firmware_blob, VB2_SECDATA_FIRMWARE_SIZE, ro_space_attributes, - pcr0_unchanged_policy, sizeof(pcr0_unchanged_policy)); + pcr0_allowed_policy, sizeof(pcr0_allowed_policy)); }
static uint32_t set_kernel_space(const void *kernel_blob) @@ -167,8 +195,8 @@ { if (index == MRC_REC_HASH_NV_INDEX) { return set_space("RO MRC Hash", index, data, HASH_NV_SIZE, - ro_space_attributes, pcr0_unchanged_policy, - sizeof(pcr0_unchanged_policy)); + ro_space_attributes, pcr0_allowed_policy, + sizeof(pcr0_allowed_policy)); } else { return set_space("RW MRC Hash", index, data, HASH_NV_SIZE, rw_space_attributes, NULL, 0);
-
Andrey Pronin (Code Review) -
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)