Re: [PATCH 2/8] lib: add asn.1 encoder

[David Woodhouse <dwmw2@infradead.org>]

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

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

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 it actually encodes an arbitrary INTEGER.

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

> +/**
> + * 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().



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