Re: [PATCH v2 03/10] security: keys: trusted: Include TPM2 creation data

From: Evan Green
Date: Fri Sep 23 2022 - 18:23:10 EST

Next message: Evan Green: "Re: [PATCH v2 05/10] security: keys: trusted: Verify creation data"
Previous message: Segher Boessenkool: "Re: [PATCH] powerpc/irq: Modernise inline assembly in irq_soft_mask_{set,return}"
In reply to: Kees Cook: "Re: [PATCH v2 03/10] security: keys: trusted: Include TPM2 creation data"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Sep 20, 2022 at 4:04 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Tue, Aug 23, 2022 at 03:25:19PM -0700, Evan Green wrote:
> > In addition to the private key and public key, the TPM2_Create
> > command may also return creation data, a creation hash, and a creation
> > ticket. These fields allow the TPM to attest to the contents of a
> > specified set of PCRs at the time the trusted key was created. Encrypted
> > hibernation will use this to ensure that PCRs settable only by the
> > kernel were set properly at the time of creation, indicating this is an
> > authentic hibernate key.
> >
> > Encode these additional parameters into the ASN.1 created to represent
> > the key blob. The new fields are made optional so that they don't bloat
> > key blobs which don't need them, and to ensure interoperability with
> > old blobs.
> >
> > ---
> >
> > (no changes since v1)
> >
> > This is a replacement for Matthew's original patch here:
> > https://patchwork.kernel.org/patch/12096489/
> >
> > That patch was written before the exported key format was switched to
> > ASN.1. This patch accomplishes the same thing (saving, loading, and
> > getting pointers to the creation data) while utilizing the new ASN.1
> > format.
>
> This part (between your S-o-b and the "---") should got below the "---"
> after your S-o-b, otherwise tooling will include it in the commit log
> (or lose your S-o-b).

Will fix.

>
> >
> > Signed-off-by: Evan Green <evgreen@xxxxxxxxxxxx>
> > ---
> > include/keys/trusted-type.h | 8 +
> > security/keys/trusted-keys/tpm2key.asn1 | 5 +-
> > security/keys/trusted-keys/trusted_tpm2.c | 202 +++++++++++++++++++---
> > 3 files changed, 190 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
> > index 4eb64548a74f1a..209086fed240a5 100644
> > --- a/include/keys/trusted-type.h
> > +++ b/include/keys/trusted-type.h
> > @@ -22,15 +22,23 @@
> > #define MAX_BLOB_SIZE 512
> > #define MAX_PCRINFO_SIZE 64
> > #define MAX_DIGEST_SIZE 64
> > +#define MAX_CREATION_DATA 412
> > +#define MAX_TK 76
> >
> > struct trusted_key_payload {
> > struct rcu_head rcu;
> > unsigned int key_len;
> > unsigned int blob_len;
> > + unsigned int creation_len;
> > + unsigned int creation_hash_len;
> > + unsigned int tk_len;
> > unsigned char migratable;
> > unsigned char old_format;
> > unsigned char key[MAX_KEY_SIZE + 1];
> > unsigned char blob[MAX_BLOB_SIZE];
> > + unsigned char *creation;
> > + unsigned char *creation_hash;
> > + unsigned char *tk;
> > };
> >
> > struct trusted_key_options {
> > diff --git a/security/keys/trusted-keys/tpm2key.asn1 b/security/keys/trusted-keys/tpm2key.asn1
> > index f57f869ad60068..1bfbf290e523a3 100644
> > --- a/security/keys/trusted-keys/tpm2key.asn1
> > +++ b/security/keys/trusted-keys/tpm2key.asn1
> > @@ -7,5 +7,8 @@ TPMKey ::= SEQUENCE {
> > emptyAuth [0] EXPLICIT BOOLEAN OPTIONAL,
> > parent INTEGER ({tpm2_key_parent}),
> > pubkey OCTET STRING ({tpm2_key_pub}),
> > - privkey OCTET STRING ({tpm2_key_priv})
> > + privkey OCTET STRING ({tpm2_key_priv}),
> > + creationData [1] EXPLICIT OCTET STRING OPTIONAL ({tpm2_key_creation_data}),
> > + creationHash [2] EXPLICIT OCTET STRING OPTIONAL ({tpm2_key_creation_hash}),
> > + creationTk [3] EXPLICIT OCTET STRING OPTIONAL ({tpm2_key_creation_tk})
> > }
>
> Maybe include a link (or named reference) to these fields from the TPM
> spec?

Sure. The TPM spec names their structure types (TPM2B_CREATION_DATA,
TPM2B_DIGEST, etc), so I'll add comments with the names of the types
as well as the command they came out of.

>
> > [...]
> > @@ -46,6 +49,26 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
> >
> > pub_len = get_unaligned_be16(src) + 2;
> > pub = src;
> > + src += pub_len;
> > +
> > + creation_data_len = get_unaligned_be16(src);
> > + if (creation_data_len) {
> > + creation_data_len += 2;
> > + creation_data = src;
> > + src += creation_data_len;
> > +
> > + creation_hash_len = get_unaligned_be16(src) + 2;
> > + creation_hash = src;
> > + src += creation_hash_len;
> > +
> > + /*
> > + * The creation ticket (TPMT_TK_CREATION) consists of a 2 byte
> > + * tag, 4 byte handle, and then a TPM2B_DIGEST, which is a 2
> > + * byte length followed by data.
> > + */
> > + creation_tk_len = get_unaligned_be16(src + 6) + 8;
> > + creation_tk = src;
> > + }
> >
> > if (!scratch)
> > return -ENOMEM;
>
> I don't see anything in this code (even before your patch) actually
> checking length against the "len" argument to tpm2_key_encode(). I think
> that needs to be fixed so proper bounds checking can be done here.
> Otherwise how do we know if we're running off the end of "src"?
>
> Yes, I realize if we have a malicious TPM everything goes out the
> window, but TPMs don't always behave -- this code should likely be more
> defensive. Or, I've misunderstood where "src" is coming from.
> Regardless, my question stands: what is checking "len"?

Sure, will add checks of len to my hunk and the bit above.

>
> > @@ -63,26 +86,81 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
> > }
> >
> > /*
> > - * Assume both octet strings will encode to a 2 byte definite length
> > + * Assume each octet string will encode to a 2 byte definite length.
> > + * Each optional octet string consumes one extra byte.
> > *
> > - * Note: For a well behaved TPM, this warning should never
> > - * trigger, so if it does there's something nefarious going on
> > + * Note: For a well behaved TPM, this warning should never trigger, so
> > + * if it does there's something nefarious going on
> > */
> > - if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
> > - "BUG: scratch buffer is too small"))
> > - return -EINVAL;
> > + if (WARN(work - scratch + pub_len + priv_len + creation_data_len +
> > + creation_hash_len + creation_tk_len + (7 * 5) + 3 >
> > + SCRATCH_SIZE,
> > + "BUG: scratch buffer is too small")) {
> > + rc = -EINVAL;
> > + goto err;
> > + }
> >
> > work = asn1_encode_integer(work, end_work, options->keyhandle);
> > work = asn1_encode_octet_string(work, end_work, pub, pub_len);
> > work = asn1_encode_octet_string(work, end_work, priv, priv_len);
> > + if (creation_data_len) {
> > + u8 *scratch2 = kmalloc(SCRATCH_SIZE, GFP_KERNEL);
> > + u8 *work2;
> > + u8 *end_work2 = scratch2 + SCRATCH_SIZE;
> > +
> > + if (!scratch2) {
> > + rc = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + work2 = asn1_encode_octet_string(scratch2,
> > + end_work2,
> > + creation_data,
> > + creation_data_len);
> > +
> > + work = asn1_encode_tag(work,
> > + end_work,
> > + 1,
> > + scratch2,
> > + work2 - scratch2);
> > +
> > + work2 = asn1_encode_octet_string(scratch2,
> > + end_work2,
> > + creation_hash,
> > + creation_hash_len);
> > +
> > + work = asn1_encode_tag(work,
> > + end_work,
> > + 2,
> > + scratch2,
> > + work2 - scratch2);
> > +
> > + work2 = asn1_encode_octet_string(scratch2,
> > + end_work2,
> > + creation_tk,
> > + creation_tk_len);
> > +
> > + work = asn1_encode_tag(work,
> > + end_work,
> > + 3,
> > + scratch2,
> > + work2 - scratch2);
> > +
> > + kfree(scratch2);
> > + }
> >
> > work1 = payload->blob;
> > work1 = asn1_encode_sequence(work1, work1 + sizeof(payload->blob),
> > scratch, work - scratch);
> > - if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
> > - return PTR_ERR(work1);
> > + if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed")) {
> > + rc = PTR_ERR(work1);
> > + goto err;
>
> I find the addition of the word "BUG" in a WARN() to be confusing. :) I
> realize this is just copying the existing style, though.

It wasn't my favorite either, but I felt an urge to be consistent. So
far I've lft this as-is, holler if I should change it.

>
> > + }
> >
> > return work1 - payload->blob;
> > +err:
> > + kfree(scratch);
> > + return rc;
> > }
> >
> > struct tpm2_key_context {
> > @@ -91,15 +169,21 @@ struct tpm2_key_context {
> > u32 pub_len;
> > const u8 *priv;
> > u32 priv_len;
> > + const u8 *creation_data;
> > + u32 creation_data_len;
> > + const u8 *creation_hash;
> > + u32 creation_hash_len;
> > + const u8 *creation_tk;
> > + u32 creation_tk_len;
> > };
> >
> > static int tpm2_key_decode(struct trusted_key_payload *payload,
> > - struct trusted_key_options *options,
> > - u8 **buf)
> > + struct trusted_key_options *options)
> > {
> > + u64 data_len;
> > int ret;
> > struct tpm2_key_context ctx;
> > - u8 *blob;
> > + u8 *blob, *buf;
> >
> > memset(&ctx, 0, sizeof(ctx));
> >
> > @@ -108,21 +192,57 @@ static int tpm2_key_decode(struct trusted_key_payload *payload,
> > if (ret < 0)
> > return ret;
> >
> > - if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
> > + data_len = ctx.priv_len + ctx.pub_len + ctx.creation_data_len +
> > + ctx.creation_hash_len + ctx.creation_tk_len;
> > +
> > + if (data_len > MAX_BLOB_SIZE)
> > return -EINVAL;
> >
> > - blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);
> > - if (!blob)
> > + buf = kmalloc(data_len + 4, GFP_KERNEL);
> > + if (!buf)
> > return -ENOMEM;
> >
> > - *buf = blob;
> > + blob = buf;
> > options->keyhandle = ctx.parent;
> >
> > memcpy(blob, ctx.priv, ctx.priv_len);
> > blob += ctx.priv_len;
> >
> > memcpy(blob, ctx.pub, ctx.pub_len);
> > + blob += ctx.pub_len;
> > + if (ctx.creation_data_len) {
> > + memcpy(blob, ctx.creation_data, ctx.creation_data_len);
> > + blob += ctx.creation_data_len;
> > + }
> > +
> > + if (ctx.creation_hash_len) {
> > + memcpy(blob, ctx.creation_hash, ctx.creation_hash_len);
> > + blob += ctx.creation_hash_len;
> > + }
> >
> > + if (ctx.creation_tk_len) {
> > + memcpy(blob, ctx.creation_tk, ctx.creation_tk_len);
> > + blob += ctx.creation_tk_len;
> > + }
> > +
> > + /*
> > + * Copy the buffer back into the payload blob since the creation
> > + * info will be used after loading.
> > + */
> > + payload->blob_len = blob - buf;
> > + memcpy(payload->blob, buf, payload->blob_len);
> > + if (ctx.creation_data_len) {
> > + payload->creation = payload->blob + ctx.priv_len + ctx.pub_len;
> > + payload->creation_len = ctx.creation_data_len;
> > + payload->creation_hash = payload->creation + ctx.creation_data_len;
> > + payload->creation_hash_len = ctx.creation_hash_len;
> > + payload->tk = payload->creation_hash +
> > + payload->creation_hash_len;
> > +
> > + payload->tk_len = ctx.creation_tk_len;
> > + }
> > +
> > + kfree(buf);
> > return 0;
> > }
> >
> > @@ -185,6 +305,42 @@ int tpm2_key_priv(void *context, size_t hdrlen,
> > return 0;
> > }
> >
> > +int tpm2_key_creation_data(void *context, size_t hdrlen,
> > + unsigned char tag,
> > + const void *value, size_t vlen)
> > +{
> > + struct tpm2_key_context *ctx = context;
> > +
> > + ctx->creation_data = value;
> > + ctx->creation_data_len = vlen;
> > +
> > + return 0;
> > +}
>
> What is hdrlen here? Or rather, what kinds of bounds checking is needed
> here?

The prototype of asn1_action_t looks like this:

typedef int (*asn1_action_t)(void *context,
size_t hdrlen, /* In case of ANY type */
unsigned char tag, /* In case of ANY type */
const void *value, size_t vlen);

I'm not an ASN.1 expert, but from studying asn1_ber_decoder(), it
looks like the core unit of an ASN.1 thing is a TLV, and hdrlen
represents the number of bytes in the datastream that make up the {TL}
portion. Based on the ANY comment above this seems to maybe be useful
for certain generic/undefined object types, but I think it's not
relevant to the types we're using. The vlen arg is the L value of the
TLV, which is what we save away in tpm2_key_context. In
asn1_ber_decoder(), I do see checks that what they pass for vlen stays
within the bounds of the source data buffer (since I knew you'd ask
:)).

>
> > +
> > +int tpm2_key_creation_hash(void *context, size_t hdrlen,
> > + unsigned char tag,
> > + const void *value, size_t vlen)
> > +{
> > + struct tpm2_key_context *ctx = context;
> > +
> > + ctx->creation_hash = value;
> > + ctx->creation_hash_len = vlen;
> > +
> > + return 0;
> > +}
> > +
> > +int tpm2_key_creation_tk(void *context, size_t hdrlen,
> > + unsigned char tag,
> > + const void *value, size_t vlen)
> > +{
> > + struct tpm2_key_context *ctx = context;
> > +
> > + ctx->creation_tk = value;
> > + ctx->creation_tk_len = vlen;
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * tpm_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
> > *
> > @@ -229,6 +385,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> > struct trusted_key_options *options)
> > {
> > int blob_len = 0;
> > + unsigned int offset;
> > struct tpm_buf buf;
> > u32 hash;
> > u32 flags;
> > @@ -317,13 +474,14 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> > rc = -E2BIG;
> > goto out;
> > }
> > - if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 4 + blob_len) {
> > + offset = TPM_HEADER_SIZE + 4;
> > + if (tpm_buf_length(&buf) < offset + blob_len) {
> > rc = -EFAULT;
> > goto out;
> > }
> >
> > blob_len = tpm2_key_encode(payload, options,
> > - &buf.data[TPM_HEADER_SIZE + 4],
> > + &buf.data[offset],
> > blob_len);
> >
> > out:
> > @@ -370,13 +528,11 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> > int rc;
> > u32 attrs;
> >
> > - rc = tpm2_key_decode(payload, options, &blob);
> > - if (rc) {
> > - /* old form */
> > - blob = payload->blob;
> > + rc = tpm2_key_decode(payload, options);
> > + if (rc)
> > payload->old_format = 1;
> > - }
> >
> > + blob = payload->blob;
> > /* new format carries keyhandle but old format doesn't */
> > if (!options->keyhandle)
> > return -EINVAL;
> > @@ -433,8 +589,6 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> > (__be32 *) &buf.data[TPM_HEADER_SIZE]);
> >
> > out:
> > - if (blob != payload->blob)
> > - kfree(blob);
> > tpm_buf_destroy(&buf);
> >
> > if (rc > 0)
> > --
> > 2.31.0
> >
>
> Otherwise looks good!

Thank you for reviewing it!
-Evan

Next message: Evan Green: "Re: [PATCH v2 05/10] security: keys: trusted: Verify creation data"
Previous message: Segher Boessenkool: "Re: [PATCH] powerpc/irq: Modernise inline assembly in irq_soft_mask_{set,return}"
In reply to: Kees Cook: "Re: [PATCH v2 03/10] security: keys: trusted: Include TPM2 creation data"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]