| Message ID | 1575781706.14069.10.camel@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix TPM 2.0 trusted keys | expand |
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().
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(). > >
On Sat, Dec 7, 2019 at 9:08 PM James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > We have a need in the TPM trusted keys to return the ASN.1 form of the > TPM key blob so it can be operated on by tools outside of the kernel. > To do that, we have to be able to read and write the key format. The > current ASN.1 decoder does fine for reading, but we need pieces of an > ASN.1 encoder to return the key blob. Is there a reason the kernel needs to do this encoding, rather than having something in userland do the translation?
On Mon, 2019-12-09 at 14:05 -0800, Matthew Garrett wrote: > On Sat, Dec 7, 2019 at 9:08 PM James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > > > We have a need in the TPM trusted keys to return the ASN.1 form of > > the TPM key blob so it can be operated on by tools outside of the > > kernel. To do that, we have to be able to read and write the key > > format. The current ASN.1 decoder does fine for reading, but we > > need pieces of an ASN.1 encoder to return the key blob. > > Is there a reason the kernel needs to do this encoding, rather than > having something in userland do the translation? Well, yes, we'd have to define a format to pass up first and then you'd always need an encoder programme to do it. Given it's fairly simple to encode the key format, doing it directly in ASN.1 ... especially as we already read ASN.1 keys, seems to be the best for the user. James
diff --git a/include/linux/asn1_encoder.h b/include/linux/asn1_encoder.h new file mode 100644 index 000000000000..0e908d748c8e --- /dev/null +++ b/include/linux/asn1_encoder.h @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef _LINUX_ASN1_ENCODER_H +#define _LINUX_ASN1_ENCODER_H + +#include <linux/types.h> +#include <linux/asn1.h> +#include <linux/asn1_ber_bytecode.h> + +#define asn1_oid_len(oid) (sizeof(oid)/sizeof(u32)) +void asn1_encode_integer(unsigned char **_data, u64 integer); +void asn1_encode_oid(unsigned char **_data, u32 oid[], int oid_len); +void asn1_encode_tag(unsigned char **data, u32 tag, + const unsigned char *string, u32 len); +void asn1_encode_octet_string(unsigned char **data, const unsigned char *string, + u32 len); +void asn1_encode_sequence(unsigned char **data, const unsigned char *seq, + u32 len); +void asn1_encode_boolean(unsigned char **data, bool val); + +#endif diff --git a/lib/Makefile b/lib/Makefile index c2f0e2a4e4e8..515b35f92c3c 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -233,7 +233,7 @@ obj-$(CONFIG_INTERVAL_TREE_TEST) += interval_tree_test.o obj-$(CONFIG_PERCPU_TEST) += percpu_test.o -obj-$(CONFIG_ASN1) += asn1_decoder.o +obj-$(CONFIG_ASN1) += asn1_decoder.o asn1_encoder.o obj-$(CONFIG_FONT_SUPPORT) += fonts/ diff --git a/lib/asn1_encoder.c b/lib/asn1_encoder.c new file mode 100644 index 000000000000..2a3b96761344 --- /dev/null +++ b/lib/asn1_encoder.c @@ -0,0 +1,201 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Simple encoder primitives for ASN.1 BER/DER/CER + * + * Copyright (C) 2019 James.Bottomley@HansenPartnership.com + */ + +#include <linux/asn1_encoder.h> +#include <linux/bug.h> +#include <linux/string.h> +#include <linux/module.h> + +/** + * 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; +} +EXPORT_SYMBOL(asn1_encode_integer); + +/* calculate the base 128 digit values setting the top bit of the first octet */ +static void asn1_encode_oid_digit(unsigned char **_data, u32 oid) +{ + int start = 7 + 7 + 7 + 7; + unsigned char *data = *_data; + + /* quick case */ + if (oid == 0) { + *data++ = 0x80; + goto out; + } + + while (oid >> start == 0) + start -= 7; + + while (start > 0) { + u8 byte; + + byte = oid >> start; + oid = oid - (byte << start); + start -= 7; + byte |= 0x80; + *data++ = byte; + } + *data++ = oid; + + out: + *_data = data; +} + +/** + * asn1_encode_oid - encode an oid to ASN.1 + * @_data: position to begin encoding at + * @oid: array of oids + * @oid_len: length of oid array + * + * this encodes an OID up to ASN.1 when presented as an array of OID values + */ +void asn1_encode_oid(unsigned char **_data, u32 oid[], int oid_len) +{ + unsigned char *data = *_data; + unsigned char *d = data + 2; + int i; + + if (WARN(oid_len < 2, "OID must have at least two elements")) + return; + if (WARN(oid_len > 32, "OID is too large")) + return; + + data[0] = _tag(UNIV, PRIM, OID); + *d++ = oid[0] * 40 + oid[1]; + for (i = 2; i < oid_len; i++) + asn1_encode_oid_digit(&d, oid[i]); + data[1] = d - data - 2; + *_data = d; +} +EXPORT_SYMBOL(asn1_encode_oid); + +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; +} + +/** + * 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); + +/** + * asn1_encode_octet_string - encode an ASN.1 OCTET STRING + * @data: pointer to encode at + * @string: string to be encoded + * @len: length of string + * + * Note ASN.1 octet strings may contain zeros, so the length is obligatory. + */ +void asn1_encode_octet_string(unsigned char **data, const unsigned char *string, + u32 len) +{ + *((*data)++) = _tag(UNIV, PRIM, OTS); + asn1_encode_definite_length(data, len); + memcpy(*data, string, len); + *data += len; +} +EXPORT_SYMBOL(asn1_encode_octet_string); + +/** + * asn1_encode_sequence - wrap a byte stream in an ASN.1 SEQUENCE + * @data: pointer to encode at + * @seq: data to be encoded as a sequence + * @len: length of the data to be encoded as a sequence + */ +void asn1_encode_sequence(unsigned char **data, const unsigned char *seq, + u32 len) +{ + *((*data)++) = _tag(UNIV, CONS, SEQ); + asn1_encode_definite_length(data, len); + memcpy(*data, seq, len); + *data += len; +} +EXPORT_SYMBOL(asn1_encode_sequence); + +/** + * asn1_encode_boolean - encode a boolean value to ASN.1 + * @data: pointer to encode at + * @val: the boolean true/false value + */ +void asn1_encode_boolean(unsigned char **data, bool val) +{ + *((*data)++) = _tag(UNIV, PRIM, BOOL); + asn1_encode_definite_length(data, 1); + *((*data)++) = val ? 1 : 0; +} +EXPORT_SYMBOL(asn1_encode_boolean);
We have a need in the TPM trusted keys to return the ASN.1 form of the TPM key blob so it can be operated on by tools outside of the kernel. To do that, we have to be able to read and write the key format. The current ASN.1 decoder does fine for reading, but we need pieces of an ASN.1 encoder to return the key blob. The current implementation only encodes the ASN.1 bits we actually need. Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> --- include/linux/asn1_encoder.h | 21 +++++ lib/Makefile | 2 +- lib/asn1_encoder.c | 201 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 223 insertions(+), 1 deletion(-) create mode 100644 include/linux/asn1_encoder.h create mode 100644 lib/asn1_encoder.c