[v4,10/11] PM: hibernate: Verify the digest encryption key

  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@google.com>
Signed-off-by: Matthew Garrett <mjg59@google.com>
Signed-off-by: Evan Green <evgreen@chromium.org>

---
Matthew's original version of this patch is here:
https://patchwork.kernel.org/project/linux-pm/patch/20210220013255.1083202-9-matthewgarrett@google.com/

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(-)
  

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@google.com>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Signed-off-by: Evan Green <evgreen@chromium.org>
> 
> ---
> Matthew's original version of this patch is here:
> https://patchwork.kernel.org/project/linux-pm/patch/20210220013255.1083202-9-matthewgarrett@google.com/
> 
> 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
>
  

On Fri, Nov 4, 2022 at 12:00 PM Kees Cook <keescook@chromium.org> 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@google.com>
> > Signed-off-by: Matthew Garrett <mjg59@google.com>
> > Signed-off-by: Evan Green <evgreen@chromium.org>
> >
> > ---
> > Matthew's original version of this patch is here:
> > https://patchwork.kernel.org/project/linux-pm/patch/20210220013255.1083202-9-matthewgarrett@google.com/
> >
> > 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