Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
From: David Woodhouse <dwmw2@infradead.org>
To: James Bottomley <James.Bottomley@HansenPartnership.com>,
	linux-integrity@vger.kernel.org,
	David Howells <dhowells@redhat.com>
Cc: Mimi Zohar <zohar@linux.ibm.com>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Subject: Re: [PATCH 4/8] security: keys: trusted: use ASN.1 tpm2 key format for the blobs
Date: Mon, 09 Dec 2019 10:04:45 +0000
Message-ID: <3b035027b934eb253143dca66ebae4356b386efe.camel@infradead.org> (raw)
In-Reply-To: <1575781831.14069.13.camel@HansenPartnership.com>


[-- Attachment #1: Type: text/plain, Size: 10717 bytes --]

On Sat, 2019-12-07 at 21:10 -0800, James Bottomley wrote:
> Modify the tpm2 key format blob output to export and import in the
> ASN.1 form for tpm2 sealed object keys.  For compatibility with prior
> trusted keys, the importer will also accept two tpm2b quantities
> representing the public and private parts of the key.  However, the
> export via keyctl pipe will only output the ASN.1 format.

You still have a tpm2_key_encode() function which spits out the raw
private/public blobs each prefixed with a length word. What's that
still used for?

> The benefit of the ASN.1 format is that it's a standard

We should probably make that true. Did we even get as far as writing up
an RFC-style description of the ASN.1? 

>  and thus the
> exported key can be used by userspace tools.  The format includes
> policy specifications, thus it gets us out of having to construct
> policy handles in userspace and the format includes the parent meaning
> you don't have to keep passing it in each time.
> 
> This patch only implements basic handling for the ASN.1 format, so
> keys with passwords but no policy.

... but doesn't bail out with an error when it sees something it
doesn't yet understand? Including the 'secret' field which is only
relevant for importable keys, etc.

> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  security/keys/trusted-keys/Makefile       |   2 +-
>  security/keys/trusted-keys/tpm2key.asn1   |  23 ++++
>  security/keys/trusted-keys/trusted_tpm1.c |   2 +-
>  security/keys/trusted-keys/trusted_tpm2.c | 170 +++++++++++++++++++++++++++++-
>  4 files changed, 190 insertions(+), 7 deletions(-)
>  create mode 100644 security/keys/trusted-keys/tpm2key.asn1
> 
> diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
> index 7b73cebbb378..e0198641eff2 100644
> --- a/security/keys/trusted-keys/Makefile
> +++ b/security/keys/trusted-keys/Makefile
> @@ -5,4 +5,4 @@
>  
>  obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
>  trusted-y += trusted_tpm1.o
> -trusted-y += trusted_tpm2.o
> +trusted-y += trusted_tpm2.o tpm2key.asn1.o
> diff --git a/security/keys/trusted-keys/tpm2key.asn1 b/security/keys/trusted-keys/tpm2key.asn1
> new file mode 100644
> index 000000000000..1851b7c80f08
> --- /dev/null
> +++ b/security/keys/trusted-keys/tpm2key.asn1
> @@ -0,0 +1,23 @@
> +---
> +--- Note: This isn't quite the definition in the standard
> +---       However, the Linux asn.1 parser doesn't understand
> +---       [2] EXPLICIT SEQUENCE OF OPTIONAL
> +---       So there's an extra intermediate TPMPolicySequence
> +---       definition to work around this

At the very least we should prod David with a pointy stick on that
topic, rather than quietly working around it.


> +
> +TPMKey ::= SEQUENCE {
> +	type		OBJECT IDENTIFIER ({tpmkey_type}),
> +	emptyAuth	[0] EXPLICIT BOOLEAN OPTIONAL,
> +	policy		[1] EXPLICIT TPMPolicySequence OPTIONAL,
> +	secret		[2] EXPLICIT OCTET STRING OPTIONAL,
> +	parent		INTEGER ({tpmkey_parent}),
> +	pubkey		OCTET STRING ({tpmkey_pub}),
> +	privkey		OCTET STRING ({tpmkey_priv})
> +	}
> +
> +TPMPolicySequence ::= SEQUENCE OF TPMPolicy
> +
> +TPMPolicy ::= SEQUENCE {
> +	commandCode		[0] EXPLICIT INTEGER,
> +	commandPolicy		[1] EXPLICIT OCTET STRING
> +	}
> diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
> index d2c5ec1e040b..d744a0d1cb89 100644
> --- a/security/keys/trusted-keys/trusted_tpm1.c
> +++ b/security/keys/trusted-keys/trusted_tpm1.c
> @@ -991,7 +991,7 @@ static int trusted_instantiate(struct key *key,
>  		goto out;
>  	}
>  
> -	if (!options->keyhandle) {
> +	if (!options->keyhandle && !tpm2) {
>  		ret = -EINVAL;
>  		goto out;
>  	}
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 08ec7f48f01d..4efc7b64d1cd 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -4,6 +4,8 @@
>   * Copyright (C) 2014 Intel Corporation
>   */
>  
> +#include <linux/asn1_encoder.h>
> +#include <linux/oid_registry.h>
>  #include <linux/string.h>
>  #include <linux/err.h>
>  #include <linux/tpm.h>
> @@ -12,6 +14,10 @@
>  #include <keys/trusted-type.h>
>  #include <keys/trusted_tpm.h>
>  
> +#include <asm/unaligned.h>
> +
> +#include "tpm2key.asn1.h"
> +
>  static struct tpm2_hash tpm2_hash_map[] = {
>  	{HASH_ALGO_SHA1, TPM_ALG_SHA1},
>  	{HASH_ALGO_SHA256, TPM_ALG_SHA256},
> @@ -20,6 +26,141 @@ static struct tpm2_hash tpm2_hash_map[] = {
>  	{HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
>  };
>  
> +static u32 tpm2key_oid[] = { 2,23,133,10,1,5 };
> +
> +static int tpm2_key_encode(struct trusted_key_payload *payload,
> +			   struct trusted_key_options *options,
> +			   u8 *src, u32 len)
> +{
> +	u8 *scratch = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +	u8 *work = scratch, *work1;
> +	u8 *priv, *pub;
> +	u16 priv_len, pub_len;
> +
> +	priv_len = get_unaligned_be16(src);
> +	src += 2;
> +	priv = src;
> +	src += priv_len;
> +	pub_len = get_unaligned_be16(src);
> +	src += 2;
> +	pub = src;
> +
> +	if (!scratch)
> +		return -ENOMEM;
> +
> +	asn1_encode_oid(&work, tpm2key_oid, asn1_oid_len(tpm2key_oid));
> +	if (options->blobauth[0] == 0) {
> +		unsigned char bool[3], *w = bool;
> +		/* tag 0 is emptyAuth */
> +		asn1_encode_boolean(&w, true);
> +		asn1_encode_tag(&work, 0, bool, w - bool);
> +	}
> +	asn1_encode_integer(&work, options->keyhandle);
> +	asn1_encode_octet_string(&work, pub, pub_len);
> +	asn1_encode_octet_string(&work, priv, priv_len);
> +
> +	work1 = payload->blob;
> +	asn1_encode_sequence(&work1, scratch, work - scratch);
> +
> +	return work1 - payload->blob;
> +}

I still don't like the lack of overflow protection here, one layer up
from the underlying encoding APIs I already commented on.


> +struct tpm2key_context {
> +	u32 parent;
> +	const u8 *pub;
> +	u32 pub_len;
> +	const u8 *priv;
> +	u32 priv_len;
> +};
> +
> +static int tpm2_key_decode(struct trusted_key_payload *payload,
> +			   struct trusted_key_options *options,
> +			   u8 **buf)
> +{
> +	int ret;
> +	struct tpm2key_context ctx;
> +	u8 *blob;
> +
> +	ret = asn1_ber_decoder(&tpm2key_decoder, &ctx, payload->blob,
> +			       payload->blob_len);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
> +		return -EINVAL;
> +
> +	blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);
> +	if (!blob)
> +		return -ENOMEM;
> +
> +	*buf = blob;
> +	options->keyhandle = ctx.parent;
> +	put_unaligned_be16(ctx.priv_len, blob);
> +	blob += 2;
> +	memcpy(blob, ctx.priv, ctx.priv_len);
> +	blob += ctx.priv_len;
> +	put_unaligned_be16(ctx.pub_len, blob);
> +	blob += 2;
> +	memcpy(blob, ctx.pub, ctx.pub_len);
> 

Hm, do we really have to create this legacy form here and pass it
around? Can't we change whatever consumes this?

> +	return 0;
> +}
> +
> +int tpmkey_parent(void *context, size_t hdrlen,
> +		  unsigned char tag,
> +		  const void *value, size_t vlen)
> +{
> +	struct tpm2key_context *ctx = context;
> +	const u8 *v = value;
> +	int i;
> +
> +	ctx->parent = 0;
> +	for (i = 0; i < vlen; i++) {
> +		ctx->parent <<= 8;
> +		ctx->parent |= v[i];
> +	}
> +	return 0;
> +}
> +
> +int tpmkey_type(void *context, size_t hdrlen,
> +		unsigned char tag,
> +		const void *value, size_t vlen)
> +{
> +	enum OID oid = look_up_OID(value, vlen);
> +
> +	if (oid != OID_TPMSealedData) {
> +		char buffer[50];
> +
> +		sprint_oid(value, vlen, buffer, sizeof(buffer));
> +		pr_debug("OID is \"%s\" which is not TPMSealedData\n",
> +			 buffer);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +int tpmkey_pub(void *context, size_t hdrlen,
> +	       unsigned char tag,
> +	       const void *value, size_t vlen)
> +{
> +	struct tpm2key_context *ctx = context;
> +
> +	ctx->pub = value;
> +	ctx->pub_len = vlen;
> +	return 0;
> +}
> +
> +int tpmkey_priv(void *context, size_t hdrlen,
> +		unsigned char tag,
> +		const void *value, size_t vlen)
> +{
> +	struct tpm2key_context *ctx = context;
> +
> +	ctx->priv = value;
> +	ctx->priv_len = vlen;
> +	return 0;
> +}
> +
>  /**
>   * tpm_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
>   *
> @@ -79,6 +220,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
>  	if (i == ARRAY_SIZE(tpm2_hash_map))
>  		return -EINVAL;
>  
> +	if (!options->keyhandle)
> +		return -EINVAL;
> +
>  	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
>  	if (rc)
>  		return rc;
> @@ -144,8 +288,10 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
>  		goto out;
>  	}
>  
> -	memcpy(payload->blob, &buf.data[TPM_HEADER_SIZE + 4], blob_len);
> -	payload->blob_len = blob_len;
> +	payload->blob_len =
> +		tpm2_key_encode(payload, options,
> +				&buf.data[TPM_HEADER_SIZE + 4],
> +				blob_len);
>  
>  out:
>  	tpm_buf_destroy(&buf);
> @@ -156,6 +302,8 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
>  		else
>  			rc = -EPERM;
>  	}
> +	if (payload->blob_len < 0)
> +		return payload->blob_len;
>  
>  	return rc;
>  }
> @@ -182,13 +330,23 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
>  	unsigned int private_len;
>  	unsigned int public_len;
>  	unsigned int blob_len;
> +	u8 *blob;
>  	int rc;
>  
> -	private_len = be16_to_cpup((__be16 *) &payload->blob[0]);
> +	rc = tpm2_key_decode(payload, options, &blob);
> +	if (rc)
> +		/* old form */
> +		blob = payload->blob;
> +
> +	/* new format carries keyhandle but old format doesn't */
> +	if (!options->keyhandle)
> +		return -EINVAL;
> +
> +	private_len = be16_to_cpup((__be16 *) &blob[0]);
>  	if (private_len > (payload->blob_len - 2))
>  		return -E2BIG;
>  
> -	public_len = be16_to_cpup((__be16 *) &payload->blob[2 + private_len]);
> +	public_len = be16_to_cpup((__be16 *) &blob[2 + private_len]);
>  	blob_len = private_len + public_len + 4;
>  	if (blob_len > payload->blob_len)
>  		return -E2BIG;
> @@ -204,7 +362,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
>  			     options->keyauth /* hmac */,
>  			     TPM_DIGEST_SIZE);
>  
> -	tpm_buf_append(&buf, payload->blob, blob_len);
> +	tpm_buf_append(&buf, blob, blob_len);
>  
>  	if (buf.flags & TPM_BUF_OVERFLOW) {
>  		rc = -E2BIG;
> @@ -217,6 +375,8 @@ 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)


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-08  5:06 [PATCH 0/8] Fix TPM 2.0 trusted keys James Bottomley
2019-12-08  5:07 ` [PATCH 1/8] security: keys: trusted: flush the key handle after use James Bottomley
2019-12-09  8:31   ` David Woodhouse
2019-12-09 15:38     ` James Bottomley
2019-12-08  5:08 ` [PATCH 2/8] lib: add asn.1 encoder James Bottomley
2019-12-09  8:50   ` David Woodhouse
2019-12-09 15:46     ` James Bottomley
2019-12-09 22:05   ` Matthew Garrett
2019-12-09 22:43     ` James Bottomley
2019-12-08  5:09 ` [PATCH 3/8] oid_registry: Add TCG defined OIDS for TPM keys James Bottomley
2019-12-09  8:55   ` David Woodhouse
2019-12-09 16:21     ` James Bottomley
2020-06-19 20:45     ` Wiseman, Monty (GE Research, US)
2020-06-19 22:50       ` Jerry Snitselaar
2020-06-20 15:36       ` James Bottomley
2020-06-23  1:17       ` Jarkko Sakkinen
2019-12-08  5:10 ` [PATCH 4/8] security: keys: trusted: use ASN.1 tpm2 key format for the blobs James Bottomley
2019-12-09 10:04   ` David Woodhouse [this message]
2019-12-09 16:31     ` James Bottomley
2019-12-08  5:11 ` [PATCH 5/8] security: keys: trusted: Make sealed key properly interoperable James Bottomley
2019-12-09 10:09   ` David Woodhouse
2019-12-09 17:23     ` James Bottomley
2019-12-08  5:12 ` [PATCH 6/8] security: keys: trusted: add PCR policy to TPM2 keys James Bottomley
2019-12-09 10:18   ` David Woodhouse
2019-12-09 18:03     ` James Bottomley
2019-12-09 18:44       ` David Woodhouse
2019-12-09 19:11         ` James Bottomley
2019-12-25 17:08           ` Ken Goldman
2019-12-08  5:13 ` [PATCH 7/8] security: keys: trusted: add ability to specify arbitrary policy James Bottomley
2019-12-08  5:14 ` [PATCH 8/8] security: keys: trusted: implement counter/timer policy James Bottomley
2019-12-09 20:20 ` [PATCH 0/8] Fix TPM 2.0 trusted keys Jarkko Sakkinen
2019-12-09 20:57   ` James Bottomley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3b035027b934eb253143dca66ebae4356b386efe.camel@infradead.org \
    --to=dwmw2@infradead.org \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=dhowells@redhat.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=linux-integrity@vger.kernel.org \
    --cc=zohar@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git