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
