| Message ID | 20250323140911.226137-11-nstange@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ima: get rid of hard dependency on SHA-1 | expand |
On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote: > PCR reads aren't currently authenticated even with > CONFIG_TCG_TPM2_HMAC=y yet. The reason being TPM2_PCR_Read can only support an audit session, so it has even more overhead than the usual HMAC session for something you don't care about and because no-one relies on plain reads anyway, relying entities use quotes. > It is probably desirable though, as e.g. IMA does some PCR reads to > form the cumulative boot digest subsequently extended into PCR 10 (an > operation which *is* authenticated). Could you elaborate on what security properties this adds? I can't see any form of attack that could be done by altering the boot aggregate: either the relying party cares, in which case it will quote the boot log and arrive at its own value, or it doesn't, in which case the value in the log is superfluous. > + /* > + * Exclusivity is not needed, but set in the > response. > + * Set it here too, so that the HMAC verification > + * won't fail. > + */ > + tpm_buf_append_hmac_session(chip, &buf, > TPM2_SA_AUDIT > + | > TPM2_SA_AUDIT_EXCLUSIVE, > + NULL, 0); Exclusivity here requires no other command be unaudited between the session starting and now. That means that with the lazy flush scheme you have a reasonable chance of this being violated and triggering an error on the command. Additionally, the response will only have the exclusive flag set if the above condition (no other unaudited command since session start) is true, which it might not be. The problem you're having is that tpm2_auth_check_hmac_response() uses the command session flags to calculate the rpHash, which is a useful short cut because for non-audit sessions they're always the same. If you want to use audit sessions, you have to teach it to dig the response session flags out of the header and use them instead. Regards, James
On Sun, Mar 23, 2025 at 03:09:08PM +0100, Nicolai Stange wrote: > PCR reads aren't currently authenticated even with CONFIG_TCG_TPM2_HMAC=y > yet. > > It is probably desirable though, as e.g. IMA does some PCR reads to form > the cumulative boot digest subsequently extended into PCR 10 (an operation > which *is* authenticated). > > Furthermore, a subsequent patch will make IMA to skip certain PCR bank > re-invalidations (which are implemented with extensions) upon kexec based > on the value read back at boot. In order to not weaken the overall > security posture in this case, it will be required to establish the same > level of trust into PCR reads as there is already for the extensions. > > Make tpm2_pcr_read() to protect the command with a HMAC auth session, > using the already existing infrastructure. > > As the TPM2_PCR_Read command doesn't have any authorizations defined, and > neither of TPM2_SA_ENCRYPT/TPM2_SA_DECRYPT is needed, use TPM2_SA_AUDIT, > even though no auditing functionality is actually being required. Since > the TPM will set TPM2_SA_AUDIT_EXCLUSIVE in its response with this > single-use session, set it upfront so that tpm_buf_check_hmac_response() > would expect it for the HMAC verification. > > Now that tpm2_pcr_read() depends on the driver's session infrastructure, > note that the first call to tpm2_pcr_read() at init time gets issued from > tpm_chip_bootstrap() -> tpm_get_pcr_allocation() > -> tpm2_get_pcr_allocation() -> tpm2_init_bank_info() > -> tpm2_pcr_read() > after > tpm_chip_bootstrap() -> tpm_auto_startup() -> tpm2_auto_startup() > -> tpm2_sessions_init(), > so there won't be any issues with that. > > Signed-off-by: Nicolai Stange <nstange@suse.de> Please write a better commit message. There's extra words like 'yet'. And e.g., subsequent patch means nothing in the commit log. Please don't use such terminology. Not going to waste my life reading this. BR, Jarkko
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index 23ded8ea47dc..e16772bbc5c8 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -168,6 +168,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, int i; int rc; struct tpm_buf buf; + struct tpm_header *head; + int offset_p; struct tpm2_pcr_read_out *out; u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0}; u16 digest_size; @@ -187,9 +189,30 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, expected_digest_size = chip->allocated_banks[i].digest_size; } - rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ); - if (rc) - return rc; + if (IS_ENABLED(CONFIG_TCG_TPM2_HMAC) && !disable_pcr_integrity) { + rc = tpm2_start_auth_session(chip); + if (rc) + return rc; + + rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_READ); + if (rc) { + tpm2_end_auth_session(chip); + return rc; + } + + /* + * Exclusivity is not needed, but set in the response. + * Set it here too, so that the HMAC verification + * won't fail. + */ + tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_AUDIT + | TPM2_SA_AUDIT_EXCLUSIVE, + NULL, 0); + } else { + rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ); + if (rc) + return rc; + } pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7); @@ -199,11 +222,24 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, tpm_buf_append(&buf, (const unsigned char *)pcr_select, sizeof(pcr_select)); + if (!disable_pcr_integrity) + tpm_buf_fill_hmac_session(chip, &buf); rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to read a pcr value"); if (rc) goto out; + if (!disable_pcr_integrity) { + rc = tpm_buf_check_hmac_response(chip, &buf, rc); + if (rc) + goto out; + } + + head = (struct tpm_header *)buf.data; + offset_p = TPM_HEADER_SIZE; + /* Skip the parameter size field: */ + if (be16_to_cpu(head->tag) == TPM2_ST_SESSIONS) + offset_p += 4; + out = (struct tpm2_pcr_read_out *)&buf.data[offset_p]; - out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE]; digest_size = be16_to_cpu(out->digest_size); if (digest_size > sizeof(digest->digest) || (!digest_size_ptr && digest_size != expected_digest_size)) { @@ -216,6 +252,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, memcpy(digest->digest, out->digest, digest_size); out: + if (!disable_pcr_integrity) + tpm2_end_auth_session(chip); tpm_buf_destroy(&buf); return rc; }
PCR reads aren't currently authenticated even with CONFIG_TCG_TPM2_HMAC=y yet. It is probably desirable though, as e.g. IMA does some PCR reads to form the cumulative boot digest subsequently extended into PCR 10 (an operation which *is* authenticated). Furthermore, a subsequent patch will make IMA to skip certain PCR bank re-invalidations (which are implemented with extensions) upon kexec based on the value read back at boot. In order to not weaken the overall security posture in this case, it will be required to establish the same level of trust into PCR reads as there is already for the extensions. Make tpm2_pcr_read() to protect the command with a HMAC auth session, using the already existing infrastructure. As the TPM2_PCR_Read command doesn't have any authorizations defined, and neither of TPM2_SA_ENCRYPT/TPM2_SA_DECRYPT is needed, use TPM2_SA_AUDIT, even though no auditing functionality is actually being required. Since the TPM will set TPM2_SA_AUDIT_EXCLUSIVE in its response with this single-use session, set it upfront so that tpm_buf_check_hmac_response() would expect it for the HMAC verification. Now that tpm2_pcr_read() depends on the driver's session infrastructure, note that the first call to tpm2_pcr_read() at init time gets issued from tpm_chip_bootstrap() -> tpm_get_pcr_allocation() -> tpm2_get_pcr_allocation() -> tpm2_init_bank_info() -> tpm2_pcr_read() after tpm_chip_bootstrap() -> tpm_auto_startup() -> tpm2_auto_startup() -> tpm2_sessions_init(), so there won't be any issues with that. Signed-off-by: Nicolai Stange <nstange@suse.de> --- drivers/char/tpm/tpm2-cmd.c | 46 +++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-)