Message-ID: <202209201552.DF8C511D23@keescook>
Date:   Tue, 20 Sep 2022 16:04:25 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Evan Green <evgreen@...omium.org>
Cc:     linux-kernel@...r.kernel.org, gwendal@...omium.org,
        Eric Biggers <ebiggers@...nel.org>,
        Matthew Garrett <mgarrett@...ora.tech>, jarkko@...nel.org,
        zohar@...ux.ibm.com, linux-integrity@...r.kernel.org,
        Pavel Machek <pavel@....cz>, apronin@...omium.org,
        dlunev@...gle.com, rjw@...ysocki.net, linux-pm@...r.kernel.org,
        corbet@....net, jejb@...ux.ibm.com,
        David Howells <dhowells@...hat.com>,
        James Morris <jmorris@...ei.org>,
        Paul Moore <paul@...l-moore.com>,
        "Serge E. Hallyn" <serge@...lyn.com>, keyrings@...r.kernel.org,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH v2 03/10] security: keys: trusted: Include TPM2 creation
 data

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

> 
> Signed-off-by: Evan Green <evgreen@...omium.org>
> ---
>  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?

> [...]
> @@ -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"?

> @@ -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.

> +	}
>  
>  	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?

> +
> +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!

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists