Date:   Wed, 9 Nov 2022 16:30:30 -0800
From:   Evan Green <evgreen@...omium.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-kernel@...r.kernel.org, corbet@....net,
        linux-pm@...r.kernel.org, rjw@...ysocki.net, gwendal@...omium.org,
        apronin@...omium.org, Pavel Machek <pavel@....cz>,
        Matthew Garrett <mgarrett@...ora.tech>,
        linux-integrity@...r.kernel.org, jejb@...ux.ibm.com,
        zohar@...ux.ibm.com, dlunev@...gle.com,
        Eric Biggers <ebiggers@...nel.org>,
        Ben Boeckel <me@...boeckel.net>, jarkko@...nel.org,
        Matthew Garrett <mjg59@...gle.com>,
        Len Brown <len.brown@...el.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v4 10/11] PM: hibernate: Verify the digest encryption key

On Fri, Nov 4, 2022 at 12:00 PM Kees Cook <keescook@...omium.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@...gle.com>
> > Signed-off-by: Matthew Garrett <mjg59@...gle.com>
> > Signed-off-by: Evan Green <evgreen@...omium.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

Powered by blists - more mailing lists