Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
From: James Bottomley <James.Bottomley@HansenPartnership.com>
To: David Woodhouse <dwmw2@infradead.org>, linux-integrity@vger.kernel.org
Cc: Mimi Zohar <zohar@linux.ibm.com>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	David Howells <dhowells@redhat.com>
Subject: Re: [PATCH 2/8] lib: add asn.1 encoder
Date: Mon, 09 Dec 2019 07:46:12 -0800
Message-ID: <1575906372.3340.14.camel@HansenPartnership.com> (raw)
In-Reply-To: <8391d7c97e2897ec7e0ba2a30de272f7a0dd1ec3.camel@infradead.org>

On Mon, 2019-12-09 at 08:50 +0000, David Woodhouse wrote:
> On Sat, 2019-12-07 at 21:08 -0800, James Bottomley wrote:
> > +/**
> > + * asn1_encode_integer - encode positive integer to ASN.1
> > + * @_data: pointer to the pointer to the data
> > + * @integer: integer to be encoded
> > + *
> > + * This is a simplified encoder: since we know the integer is
> > + * positive we don't have to bother with twos complement and since
> > we
> > + * know the largest integer is a u64, we know the max length is 8.
> > + */
> > +void asn1_encode_integer(unsigned char **_data, u64 integer)
> > +{
> > +	unsigned char *data = *_data, *d = &data[2];
> > +	int i;
> > +	bool found = false;
> > +
> > +	data[0] = _tag(UNIV, PRIM, INT);
> > +	if (integer == 0) {
> > +		*d++ = 0;
> > +		goto out;
> > +	}
> > +	for (i = sizeof(integer); i > 0 ; i--) {
> > +		int byte = integer >> (8*(i-1));
> > +
> > +		if (!found && byte == 0)
> > +			continue;
> > +		found = true;
> > +		if (byte & 0x80)
> > +			*d++ = 0;
> > +		*d++ = byte;
> > +	}
> > + out:
> > +	data[1] = d - data - 2;
> > +	*_data = d;
> > +}
> 
> I'd be a lot happier to see a 'buffer length' argument here. This API
> is just one accidental u64 underflow away from a caller which "knows"
> its <128 integer is only three bytes long, actually taking eleven and
> overflowing its buffer. Especially since  you are actively
> encouraging people to create fragments on the stack and then assemble
> them into SEQUENCES later (qv¹).

OK, I'll add a length argument.

> Also: is documenting it as taking a 'positive integer' enough? Making
> that explicit in the function name might be more likely to prevent
> future users from assuming i actually encodes an arbitrary INTEGER.

Well, I have no use case for a signed integer at the moment, but it
shouldn't be too hard to add, so if that use case came along someone
could fill in the code ... it just involves stripping off leading
0xff's.

How about I make the input an s64 and return EINVAL on negative?  That
way the API is ready for the negative case.

> > +static void asn1_encode_definite_length(unsigned char **data, u32
> > len)
> > +{
> > +	if (len <= 0x7f) {
> > +		*((*data)++) = len;
> > +		return;
> > +	}
> > +	if (len <= 0xff) {
> > +		*((*data)++) = 0x81;
> > +		*((*data)++) = len & 0xff;
> > +		return;
> > +	}
> > +	if (len <= 0xffff) {
> > +		*((*data)++) = 0x82;
> > +		*((*data)++) = (len >> 8) & 0xff;
> > +		*((*data)++) = len & 0xff;
> > +		return;
> > +	}
> > +
> > +	if (WARN(len > 0xffffff, "ASN.1 length can't be >
> > 0xffffff"))
> > +		return;
> > +
> > +	*((*data)++) = 0x83;
> > +	*((*data)++) = (len >> 16) & 0xff;
> > +	*((*data)++) = (len >> 8) & 0xff;
> > +	*((*data)++) = len & 0xff;
> > +}
> 
> (¹)
> 
> That's nice when you know the length in advance. Less so when you
> don't, because you have to either calculate it first or actually
> create
> the whole of the content in a separate buffer and copy it around.
> 
> It would be useful to permit sequences with indeterminate length. You
> could even return a pointer which allows them to be changed to
> definite
> length if they are <128 bytes at the end.
> 
> I note that later in this series in tpm2_encode_policy() you are
> eschewing your own API for this, and doing just what I said above —
> going back and filling in the length later.

Yes, that bit bothers me.  I'll fix it to do indefinite followed by
updateable sequence lengths and the same for the tag code.

James


> > +/**
> > + * asn1_encode_tag - add a tag for optional or explicit value
> > + * @data: pointer to place tag at
> > + * @tag: tag to be placed
> > + * @string: the data to be tagged
> > + * @len: the length of the data to be tagged
> > + *
> > + * Note this currently only handles short form tags < 31
> > + */
> > +void asn1_encode_tag(unsigned char **data, u32 tag,
> > +		     const unsigned char *string, u32 len)
> > +{
> > +	if (WARN(tag > 30, "ASN.1 tag can't be > 30"))
> > +		return;
> > +
> > +	*((*data)++) = _tagn(CONT, CONS, tag);
> > +	asn1_encode_definite_length(data, len);
> > +	memcpy(*data, string, len);
> > +	*data += len;
> > +}
> > +EXPORT_SYMBOL(asn1_encode_tag);
> 
> EXPORT_SYMBOL() again when everything else here uses
> EXPORT_SYMBOL_GPL().
> 
> 


  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 [this message]
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
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=1575906372.3340.14.camel@HansenPartnership.com \
    --to=james.bottomley@hansenpartnership.com \
    --cc=dhowells@redhat.com \
    --cc=dwmw2@infradead.org \
    --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