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

[Kees Cook]

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