Message ID | 20200310051607.30334-2-James.Bottomley@HansenPartnership.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | TPM 2.0 trusted keys with attached policy | expand |
James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > + * Copyright (C) 2019 James.Bottomley@HansenPartnership.com 2020? > +unsigned char * > +asn1_encode_integer(unsigned char *data, const unsigned char *end_data, > + s64 integer); I wonder if we should actually use u8 rather than unsigned char for data pointers like this. That applies to asn1_ber_decoder() also. You should be able to precalculate the length from fls64() or ilog2(), e.g.: static size_t asn1_uint_len(unsigned long long integer) { size_t l = integer ? fls64(integer) : 1; return l / 8 + 1; } See attached toy program. > +/** > + * asn1_encode_tag() - add a tag for optional or explicit value > + * @data: pointer to place tag at > + * @end_data: end of data pointer, points one beyond last usable byte in @data > + * @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. To encode > + * in place pass a NULL @string and -1 for @len; all this will do is > + * add an indefinite length tag and update the data pointer to the > + * place where the tag contents should be placed. After the data is > + * placed, repeat the prior statement but now with the known length. > + * In order to avoid having to keep both before and after pointers, > + * the repeat expects to be called with @data pointing to where the > + * first encode placed it. > + */ I wonder if it's worth appending a note to the comment that if indefinite length encoding is selected, then the result is not DER-compliant and may not be CER-compliant since you're advertising BER/DER/CER. > + if (*data_len < 1) > + return -EINVAL; ENOBUFS? I guess it doesn't really matter. David --- #include <stdio.h> static inline int fls64(unsigned long long x) { int bitpos = -1; /* * AMD64 says BSRQ won't clobber the dest reg if x==0; Intel64 says the * dest reg is undefined if x==0, but their CPU architect says its * value is written to set it to the same as before. */ asm("bsrq %1,%q0" : "+r" (bitpos) : "rm" (x)); return bitpos + 1; } static const unsigned long long vals[] = { 0x1000000, 0xffffff, 0x800000, 0x7fffff, 0x100000, 0xfffff, 0x80000, 0x7ffff, 0x10000, 0xffff, 0x8000, 0x7fff, 0x1000, 0xfff, 0x800, 0x7ff, 0x100, 0xff, 0x80, 0x7f, 3, 2, 1, 0 }; static size_t asn1_uint_len(unsigned long long integer) { size_t l = integer ? fls64(integer) : 1; return l / 8 + 1; } int main() { const unsigned long long *p = vals; unsigned long long integer; do { integer = *p++; printf("len: %16llx -> %zu\n", integer, asn1_uint_len(integer)); } while (integer); }
On Thu, 2020-03-19 at 16:47 +0000, David Howells wrote: > James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > > > + * Copyright (C) 2019 James.Bottomley@HansenPartnership.com > > 2020? Actually, no, under the Berne convention it should be the date the work was committed to a fixed medium. In theory, that's the first git commit I did in my internal repository. There's a lot of wiggle room in this: authors tend to use the date the manuscript was completed, not when it was started, for instance, but 2019 would seem to be the more accurate year even so. > > +unsigned char * > > +asn1_encode_integer(unsigned char *data, const unsigned char > > *end_data, > > + s64 integer); > > I wonder if we should actually use u8 rather than unsigned char for > data pointers like this. That applies to asn1_ber_decoder() also. I followed exactly what you did in asn1_decoder.c ... I think there's value in having a completely signature consistent interface. Of course, if you want to alter the encoder and decoder to u8 that can be done as a follow on patch. > You should be able to precalculate the length from fls64() or > ilog2(), e.g.: > > static size_t asn1_uint_len(unsigned long long integer) > { > size_t l = integer ? fls64(integer) : 1; > return l / 8 + 1; > } > > See attached toy program. We can, but it adds a lot of complexity for pretty much no gain: it's no real hassle to begin the encoding and then find the buffer is too short, and the code is definitely much easier to follow. > > +/** > > + * asn1_encode_tag() - add a tag for optional or explicit value > > + * @data: pointer to place tag at > > + * @end_data: end of data pointer, points one beyond last > > usable byte in @data > > + * @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. To > > encode > > + * in place pass a NULL @string and -1 for @len; all this will do > > is > > + * add an indefinite length tag and update the data pointer to the > > + * place where the tag contents should be placed. After the data > > is > > + * placed, repeat the prior statement but now with the known > > length. > > + * In order to avoid having to keep both before and after > > pointers, > > + * the repeat expects to be called with @data pointing to where > > the > > + * first encode placed it. > > + */ > > I wonder if it's worth appending a note to the comment that if > indefinite length encoding is selected, then the result is not DER- > compliant and may not be CER-compliant since you're advertising > BER/DER/CER. We only encode definite length currently, so the comment is superfluous (and probably confusing if you don't know the difference between DER/BER and CER). Let's add something like this iff we ever start to use indefinite lengths in the encoder. > > + if (*data_len < 1) > > + return -EINVAL; > > ENOBUFS? I guess it doesn't really matter. This error gets sent to the user who's not doing to know why because it's a kernel internal length we got wrong ... let's just keep EINVAL which is our default "something went wrong" error. > David > --- > #include <stdio.h> > > static inline int fls64(unsigned long long x) > { > int bitpos = -1; > /* > * AMD64 says BSRQ won't clobber the dest reg if x==0; Intel64 > says the > * dest reg is undefined if x==0, but their CPU architect says > its > * value is written to set it to the same as before. > */ > asm("bsrq %1,%q0" > : "+r" (bitpos) > : "rm" (x)); > return bitpos + 1; > } > > static const unsigned long long vals[] = { > 0x1000000, 0xffffff, 0x800000, 0x7fffff, > 0x100000, 0xfffff, 0x80000, 0x7ffff, > 0x10000, 0xffff, 0x8000, 0x7fff, > 0x1000, 0xfff, 0x800, 0x7ff, > 0x100, 0xff, 0x80, 0x7f, > 3, 2, 1, 0 > }; > > static size_t asn1_uint_len(unsigned long long integer) > { > size_t l = integer ? fls64(integer) : 1; > return l / 8 + 1; > } > > int main() > { > const unsigned long long *p = vals; > unsigned long long integer; > > do { > integer = *p++; > printf("len: %16llx -> %zu\n", integer, > asn1_uint_len(integer)); > } while (integer); > } >
James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > > I wonder if it's worth appending a note to the comment that if > > indefinite length encoding is selected, then the result is not DER- > > compliant and may not be CER-compliant since you're advertising > > BER/DER/CER. > > We only encode definite length currently, so the comment is superfluous > (and probably confusing if you don't know the difference between > DER/BER and CER). Let's add something like this iff we ever start to > use indefinite lengths in the encoder. Your code appears to actually do indefinite length encoding if -1 is passed as len to asn1_encode_tag(). The kdoc says: To encode in place pass a NULL @string and -1 for @len; all this will do is add an indefinite length tag and update the data pointer to the place where the tag contents should be placed. Granted, your patches might not use it, but you're making a generic ASN.1 encoder library. David
On Mon, Mar 09, 2020 at 10:16:00PM -0700, James Bottomley wrote: > We have a need in the TPM2 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. The specific tools are the openssl_tpm2_engine, openconnect > and the Intel tpm2-tss-engine. To do that, we have to be able to read > and write the same binary key format the tools use. The current ASN.1 > decoder does fine for reading, but we need pieces of an ASN.1 encoder > to write the key blob in binary compatible form. > > For backwards compatibility, the trusted key reader code will still > accept the two TPM2B quantities that it uses today, but the writer > will only output the ASN.1 form. > > The current implementation only encodes the ASN.1 bits we actually need. [...] > diff --git a/lib/Makefile b/lib/Makefile > index 611872c06926..1a9169ef2bed 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -237,7 +237,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/ > Shouldn't there be separate kconfig options CONFIG_ASN1_DECODER and CONFIG_ASN1_ENCODER so that the kernel doesn't get bloated for most users, who will only need the decoder? - Eric
On Thu, 2020-03-19 at 19:12 +0000, David Howells wrote: > James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > > > > I wonder if it's worth appending a note to the comment that if > > > indefinite length encoding is selected, then the result is not > > > DER-compliant and may not be CER-compliant since you're > > > advertising BER/DER/CER. > > > > We only encode definite length currently, so the comment is > > superfluous (and probably confusing if you don't know the > > difference between DER/BER and CER). Let's add something like this > > iff we ever start to use indefinite lengths in the encoder. > > Your code appears to actually do indefinite length encoding if -1 is > passed as len to asn1_encode_tag(). The kdoc says: > > To encode in place pass a NULL @string and -1 for @len; all > this will do is add an indefinite length tag and update the data > pointer to the place where the tag contents should be placed. > > Granted, your patches might not use it, but you're making a generic > ASN.1 encoder library. That was a thing the other David asked for. But actually, I think the comment is a lie: the first time around we encode a definite length for the max buffer size and on the recode we do the length for the actual buffer size, so we never actually place an indefinite length tag there ... I think David wanted us to, to keep the ASN.1 always legal, but the max len thing does that too so I must have changed it without updating the comment, I'll fix that. James
diff --git a/include/linux/asn1_encoder.h b/include/linux/asn1_encoder.h new file mode 100644 index 000000000000..08cd0c2ad34f --- /dev/null +++ b/include/linux/asn1_encoder.h @@ -0,0 +1,32 @@ +/* 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> +#include <linux/bug.h> + +#define asn1_oid_len(oid) (sizeof(oid)/sizeof(u32)) +unsigned char * +asn1_encode_integer(unsigned char *data, const unsigned char *end_data, + s64 integer); +unsigned char * +asn1_encode_oid(unsigned char *data, const unsigned char *end_data, + u32 oid[], int oid_len); +unsigned char * +asn1_encode_tag(unsigned char *data, const unsigned char *end_data, + u32 tag, const unsigned char *string, int len); +unsigned char * +asn1_encode_octet_string(unsigned char *data, + const unsigned char *end_data, + const unsigned char *string, u32 len); +unsigned char * +asn1_encode_sequence(unsigned char *data, const unsigned char *end_data, + const unsigned char *seq, int len); +unsigned char * +asn1_encode_boolean(unsigned char *data, const unsigned char *end_data, + bool val); + +#endif diff --git a/lib/Makefile b/lib/Makefile index 611872c06926..1a9169ef2bed 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -237,7 +237,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..24d45a838887 --- /dev/null +++ b/lib/asn1_encoder.c @@ -0,0 +1,431 @@ +// 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 + * @end_data: end of data pointer, points one beyond last usable byte in @data + * @integer: integer to be encoded + * + * This is a simplified encoder: it only currently does + * positive integers, but it should be simple enough to add the + * negative case if a use comes along. + */ +unsigned char * +asn1_encode_integer(unsigned char *data, const unsigned char *end_data, + s64 integer) +{ + int data_len = end_data - data; + unsigned char *d = &data[2]; + bool found = false; + int i; + + if (WARN(integer < 0, + "BUG: integer encode only supports positive integers")) + return ERR_PTR(-EINVAL); + + if (IS_ERR(data)) + return data; + + /* need at least 3 bytes for tag, length and integer encoding */ + if (data_len < 3) + return ERR_PTR(-EINVAL); + + /* remaining length where at d (the start of the integer encoding) */ + data_len -= 2; + + 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; + + /* + * for a positive number the first byte must have bit + * 7 clear in two's complement (otherwise it's a + * negative number) so prepend a leading zero if + * that's not the case + */ + if (!found && (byte & 0x80)) { + /* + * no check needed here, we already know we + * have len >= 1 + */ + *d++ = 0; + data_len--; + } + + found = true; + if (data_len == 0) + return ERR_PTR(-EINVAL); + + *d++ = byte; + data_len--; + } + + out: + data[1] = d - data - 2; + + return d; +} +EXPORT_SYMBOL_GPL(asn1_encode_integer); + +/* calculate the base 128 digit values setting the top bit of the first octet */ +static int asn1_encode_oid_digit(unsigned char **_data, int *data_len, u32 oid) +{ + unsigned char *data = *_data; + int start = 7 + 7 + 7 + 7; + int ret = 0; + + if (*data_len < 1) + return -EINVAL; + + /* quick case */ + if (oid == 0) { + *data++ = 0x80; + (*data_len)--; + goto out; + } + + while (oid >> start == 0) + start -= 7; + + while (start > 0 && *data_len > 0) { + u8 byte; + + byte = oid >> start; + oid = oid - (byte << start); + start -= 7; + byte |= 0x80; + *data++ = byte; + (*data_len)--; + } + + if (*data_len > 0) { + *data++ = oid; + (*data_len)--; + } else { + ret = -EINVAL; + } + + out: + *_data = data; + return ret; +} + +/** + * asn1_encode_oid() - encode an oid to ASN.1 + * @data: position to begin encoding at + * @end_data: end of data pointer, points one beyond last usable byte in @data + * @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 + */ +unsigned char * +asn1_encode_oid(unsigned char *data, const unsigned char *end_data, + u32 oid[], int oid_len) +{ + int data_len = end_data - data; + unsigned char *d = data + 2; + int i, ret; + + if (WARN(oid_len < 2, "OID must have at least two elements")) + return ERR_PTR(-EINVAL); + + if (WARN(oid_len > 32, "OID is too large")) + return ERR_PTR(-EINVAL); + + if (IS_ERR(data)) + return data; + + + /* need at least 3 bytes for tag, length and OID encoding */ + if (data_len < 3) + return ERR_PTR(-EINVAL); + + data[0] = _tag(UNIV, PRIM, OID); + *d++ = oid[0] * 40 + oid[1]; + + data_len -= 3; + + ret = 0; + + for (i = 2; i < oid_len; i++) { + ret = asn1_encode_oid_digit(&d, &data_len, oid[i]); + if (ret < 0) + return ERR_PTR(ret); + } + + data[1] = d - data - 2; + + return d; +} +EXPORT_SYMBOL_GPL(asn1_encode_oid); + +static int asn1_encode_length(unsigned char **data, int *data_len, int len) +{ + if (*data_len < 1) + return -EINVAL; + + if (len < 0) { + *((*data)++) = ASN1_INDEFINITE_LENGTH; + (*data_len)--; + return 0; + } + + if (len <= 0x7f) { + *((*data)++) = len; + (*data_len)--; + return 0; + } + + if (*data_len < 2) + return -EINVAL; + + if (len <= 0xff) { + *((*data)++) = 0x81; + *((*data)++) = len & 0xff; + *data_len -= 2; + return 0; + } + + if (*data_len < 3) + return -EINVAL; + + if (len <= 0xffff) { + *((*data)++) = 0x82; + *((*data)++) = (len >> 8) & 0xff; + *((*data)++) = len & 0xff; + *data_len -= 3; + return 0; + } + + if (WARN(len > 0xffffff, "ASN.1 length can't be > 0xffffff")) + return -EINVAL; + + if (*data_len < 4) + return -EINVAL; + *((*data)++) = 0x83; + *((*data)++) = (len >> 16) & 0xff; + *((*data)++) = (len >> 8) & 0xff; + *((*data)++) = len & 0xff; + *data_len -= 4; + + return 0; +} + +/** + * asn1_encode_tag() - add a tag for optional or explicit value + * @data: pointer to place tag at + * @end_data: end of data pointer, points one beyond last usable byte in @data + * @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. To encode + * in place pass a NULL @string and -1 for @len; all this will do is + * add an indefinite length tag and update the data pointer to the + * place where the tag contents should be placed. After the data is + * placed, repeat the prior statement but now with the known length. + * In order to avoid having to keep both before and after pointers, + * the repeat expects to be called with @data pointing to where the + * first encode placed it. + */ +unsigned char * +asn1_encode_tag(unsigned char *data, const unsigned char *end_data, + u32 tag, const unsigned char *string, int len) +{ + int data_len = end_data - data; + int ret; + + if (WARN(tag > 30, "ASN.1 tag can't be > 30")) + return ERR_PTR(-EINVAL); + + if (!string && WARN(len > 127, + "BUG: recode tag is too big (>127)")) + return ERR_PTR(-EINVAL); + + if (IS_ERR(data)) + return data; + + if (!string && len > 0) { + /* + * we're recoding, so move back to the start of the + * tag and install a dummy length because the real + * data_len should be NULL + */ + data -= 2; + data_len = 2; + } + + if (data_len < 2) + return ERR_PTR(-EINVAL); + + *(data++) = _tagn(CONT, CONS, tag); + data_len--; + ret = asn1_encode_length(&data, &data_len, len); + if (ret < 0) + return ERR_PTR(ret); + + if (!string) + return data; + + if (data_len < len) + return ERR_PTR(-EINVAL); + + memcpy(data, string, len); + data += len; + + return data; +} +EXPORT_SYMBOL_GPL(asn1_encode_tag); + +/** + * asn1_encode_octet_string() - encode an ASN.1 OCTET STRING + * @data: pointer to encode at + * @end_data: end of data pointer, points one beyond last usable byte in @data + * @string: string to be encoded + * @len: length of string + * + * Note ASN.1 octet strings may contain zeros, so the length is obligatory. + */ +unsigned char * +asn1_encode_octet_string(unsigned char *data, + const unsigned char *end_data, + const unsigned char *string, u32 len) +{ + int data_len = end_data - data; + int ret; + + if (IS_ERR(data)) + return data; + + /* need minimum of 2 bytes for tag and length of zero length string */ + if (data_len < 2) + return ERR_PTR(-EINVAL); + + *(data++) = _tag(UNIV, PRIM, OTS); + data_len--; + + ret = asn1_encode_length(&data, &data_len, len); + if (ret) + return ERR_PTR(ret); + + if (data_len < len) + return ERR_PTR(-EINVAL); + + memcpy(data, string, len); + data += len; + + return data; +} +EXPORT_SYMBOL_GPL(asn1_encode_octet_string); + +/** + * asn1_encode_sequence() - wrap a byte stream in an ASN.1 SEQUENCE + * @data: pointer to encode at + * @end_data: end of data pointer, points one beyond last usable byte in @data + * @seq: data to be encoded as a sequence + * @len: length of the data to be encoded as a sequence + * + * Fill in a sequence. To encode in place, pass NULL for @seq and -1 + * for @len; then call again once the length is known (still with NULL + * for @seq). In order to avoid having to keep both before and after + * pointers, the repeat expects to be called with @data pointing to + * where the first encode placed it. + */ +unsigned char * +asn1_encode_sequence(unsigned char *data, const unsigned char *end_data, + const unsigned char *seq, int len) +{ + int data_len = end_data - data; + int ret; + + if (!seq && WARN(len > 127, + "BUG: recode sequence is too big (>127)")) + return ERR_PTR(-EINVAL); + + if (IS_ERR(data)) + return data; + + if (!seq && len >= 0) { + /* + * we're recoding, so move back to the start of the + * sequence and install a dummy length because the + * real length should be NULL + */ + data -= 2; + data_len = 2; + } + + if (data_len < 2) + return ERR_PTR(-EINVAL); + + *(data++) = _tag(UNIV, CONS, SEQ); + data_len--; + + ret = asn1_encode_length(&data, &data_len, len); + if (ret) + return ERR_PTR(ret); + + if (!seq) + return data; + + if (data_len < len) + return ERR_PTR(-EINVAL); + + memcpy(data, seq, len); + data += len; + + return data; +} +EXPORT_SYMBOL_GPL(asn1_encode_sequence); + +/** + * asn1_encode_boolean() - encode a boolean value to ASN.1 + * @data: pointer to encode at + * @end_data: end of data pointer, points one beyond last usable byte in @data + * @val: the boolean true/false value + */ +unsigned char * +asn1_encode_boolean(unsigned char *data, const unsigned char *end_data, + bool val) +{ + int data_len = end_data - data; + + if (IS_ERR(data)) + return data; + + /* booleans are 3 bytes: tag, length == 1 and value == 0 or 1 */ + if (data_len < 3) + return ERR_PTR(-EINVAL); + + *(data++) = _tag(UNIV, PRIM, BOOL); + data_len--; + + asn1_encode_length(&data, &data_len, 1); + + if (val) + *(data++) = 1; + else + *(data++) = 0; + + return data; +} +EXPORT_SYMBOL_GPL(asn1_encode_boolean);