On Fri, Nov 4, 2022 at 12:00 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote: > > 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) || > Yikes, you're right. > > > + (payload->creation_hash_len < SHA256_DIGEST_SIZE)) { > > + ret = -EINVAL; > > + goto out; > > + } > > + > > + sha256(payload->creation + 2, payload->creation_len - 2, certhash); > > Why +2 offset? The first two bytes are a __be16 size that isn't part of what the TPM hashes. > > > + 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)) { Oops, yes. > > ? > > > + 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. :) Ok, agreed it's a bit too hairy to manage this way. I can define a struct specific to this form of the response I'm expecting, then use struct fields like a proper C developer. > > > + > > + ret = -EINVAL; > > + goto out; > > + } > > + > > data->key = key; > > key = NULL; > > > > -- > > 2.38.1.431.g37b22c650d-goog > > > > -- > Kees Cook
