On Thu, Nov 03, 2022 at 11:01:18AM -0700, Evan Green wrote: > We want to ensure that the key used to encrypt the digest was created by > the kernel during hibernation. To do this we request that the TPM > include information about the value of PCR 23 at the time of key > creation in the sealed blob. On resume, we can make sure that the PCR > information in the creation data blob (already certified by the TPM to > be accurate) corresponds to the expected value. Since only > the kernel can touch PCR 23, if an attacker generates a key themselves > the value of PCR 23 will have been different, allowing us to reject the > key and boot normally instead of resuming. > > Co-developed-by: Matthew Garrett <mjg59@xxxxxxxxxx> > Signed-off-by: Matthew Garrett <mjg59@xxxxxxxxxx> > Signed-off-by: Evan Green <evgreen@xxxxxxxxxxxx> > > --- > Matthew's original version of this patch is here: > https://patchwork.kernel.org/project/linux-pm/patch/20210220013255.1083202-9-matthewgarrett@xxxxxxxxxx/ > > I moved the TPM2_CC_CERTIFYCREATION code into a separate change in the > trusted key code because the blob_handle was being flushed and was no > longer valid for use in CC_CERTIFYCREATION after the key was loaded. As > an added benefit of moving the certification into the trusted keys code, > we can drop the other patch from the original series that squirrelled > the blob_handle away. > > Changes in v4: > - Local variable reordering (Jarkko) > > Changes in v3: > - Changed funky tag to Co-developed-by (Kees). Matthew, holler if you > want something different. > > Changes in v2: > - Fixed some sparse warnings > - Use CRYPTO_LIB_SHA256 to get rid of sha256_data() (Eric) > - Adjusted offsets due to new ASN.1 format, and added a creation data > length check. > > kernel/power/snapenc.c | 67 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 65 insertions(+), 2 deletions(-) > > diff --git a/kernel/power/snapenc.c b/kernel/power/snapenc.c > index 50167a37c5bf23..2f421061498246 100644 > --- a/kernel/power/snapenc.c > +++ b/kernel/power/snapenc.c > @@ -22,6 +22,12 @@ static struct tpm_digest known_digest = { .alg_id = TPM_ALG_SHA256, > 0xf1, 0x22, 0x38, 0x6c, 0x33, 0xb1, 0x14, 0xb7, 0xec, 0x05, > 0x5f, 0x49}}; > > +/* sha256(sha256(empty_pcr | known_digest)) */ > +static const char expected_digest[] = {0x2f, 0x96, 0xf2, 0x1b, 0x70, 0xa9, 0xe8, > + 0x42, 0x25, 0x8e, 0x66, 0x07, 0xbe, 0xbc, 0xe3, 0x1f, 0x2c, 0x84, 0x4a, > + 0x3f, 0x85, 0x17, 0x31, 0x47, 0x9a, 0xa5, 0x53, 0xbb, 0x23, 0x0c, 0x32, > + 0xf3}; > + > /* Derive a key from the kernel and user keys for data encryption. */ > static int snapshot_use_user_key(struct snapshot_data *data) > { > @@ -486,7 +492,7 @@ static int snapshot_setup_encryption_common(struct snapshot_data *data) > static int snapshot_create_kernel_key(struct snapshot_data *data) > { > /* Create a key sealed by the SRK. */ > - char *keyinfo = "new\t32\tkeyhandle=0x81000000"; > + char *keyinfo = "new\t32\tkeyhandle=0x81000000\tcreationpcrs=0x00800000"; > const struct cred *cred = current_cred(); > struct tpm_digest *digests = NULL; > struct key *key = NULL; > @@ -613,6 +619,8 @@ static int snapshot_load_kernel_key(struct snapshot_data *data, > > char *keytemplate = "load\t%s\tkeyhandle=0x81000000"; > const struct cred *cred = current_cred(); > + struct trusted_key_payload *payload; > + char certhash[SHA256_DIGEST_SIZE]; > struct tpm_digest *digests = NULL; > char *blobstring = NULL; > struct key *key = NULL; > @@ -635,8 +643,10 @@ static int snapshot_load_kernel_key(struct snapshot_data *data, > > digests = kcalloc(chip->nr_allocated_banks, sizeof(struct tpm_digest), > GFP_KERNEL); > - if (!digests) > + if (!digests) { > + ret = -ENOMEM; > goto out; > + } > > for (i = 0; i < chip->nr_allocated_banks; i++) { > digests[i].alg_id = chip->allocated_banks[i].alg_id; > @@ -676,6 +686,59 @@ static int snapshot_load_kernel_key(struct snapshot_data *data, > if (ret != 0) > goto out; > > + /* Verify the creation hash matches the creation data. */ > + payload = key->payload.data[0]; > + if (!payload->creation || !payload->creation_hash || > + (payload->creation_len < 3) || Later accesses are reaching into indexes, 6, 8, 12, 14, etc. Shouldn't this test be: (payload->creation_len < 14 + SHA256_DIGEST_SIZE) || > + (payload->creation_hash_len < SHA256_DIGEST_SIZE)) { > + ret = -EINVAL; > + goto out; > + } > + > + sha256(payload->creation + 2, payload->creation_len - 2, certhash); Why +2 offset? > + if (memcmp(payload->creation_hash + 2, certhash, SHA256_DIGEST_SIZE) != 0) { And if this is +2 also, shouldn't the earlier test be: (payload->creation_hash_len - 2 != SHA256_DIGEST_SIZE)) { ? > + if (be32_to_cpu(*(__be32 *)&payload->creation[2]) != 1) { > + ret = -EINVAL; > + goto out; > + } > + > + if (be16_to_cpu(*(__be16 *)&payload->creation[6]) != TPM_ALG_SHA256) { > + ret = -EINVAL; > + goto out; > + } > + > + if (*(char *)&payload->creation[8] != 3) { > + ret = -EINVAL; > + goto out; > + } > + > + /* PCR 23 selected */ > + if (be32_to_cpu(*(__be32 *)&payload->creation[8]) != 0x03000080) { > + ret = -EINVAL; > + goto out; > + } > + > + if (be16_to_cpu(*(__be16 *)&payload->creation[12]) != > + SHA256_DIGEST_SIZE) { > + ret = -EINVAL; > + goto out; > + } > + > + /* Verify PCR 23 contained the expected value when the key was created. */ > + if (memcmp(&payload->creation[14], expected_digest, > + SHA256_DIGEST_SIZE) != 0) { These various literals (2, 6, 8, 3, 8, 0x03000080, 12, 14) should be explicit #defines so their purpose/meaning is more clear. I can guess at it, but better to avoid the guessing. :) > + > + ret = -EINVAL; > + goto out; > + } > + > data->key = key; > key = NULL; > > -- > 2.38.1.431.g37b22c650d-goog > -- Kees Cook
