| Message ID | 1575936367.31378.52.camel@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix TPM 2.0 trusted keys | expand |
On Mon, 2019-12-09 at 16:06 -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 > + * @len: length of buffer > + * > + * 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. > + */ > +void asn1_encode_integer(unsigned char **_data, s64 integer, int len) > +{ > + unsigned char *data = *_data, *d = &data[2]; > + int i; > + bool found = false; > + > + if (WARN(integer < 0, > + "BUG: asn1_encode_integer only supports positive integers")) > + return; > + > + if (WARN(len < 3, > + "BUG: buffer for integers must have at least 3 bytes")) > + return; > + > + 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; > + found = true; > + if (byte & 0x80) { > + /* > + * no check needed here, we already know we > + * have len >= 1 > + */ > + *d++ = 0; > + len--; > + } > + if (WARN(len == 0, > + "BUG buffer too short in asn1_encode_integer")) > + return; > + *d++ = byte; > + len--; > + } > + out: > + data[1] = d - data - 2; > + *_data = d; > +} > +EXPORT_SYMBOL_GPL(asn1_encode_integer); Didn't you say you were going to make it return an error when it ran out of space or was asked to encode a negative number? There are other encoding functions which you haven't yet added the buffer length field to, and they'll want to be able to return -ENOSPC too.
On Tue, 2019-12-10 at 08:18 +0000, David Woodhouse wrote: > On Mon, 2019-12-09 at 16:06 -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 > > + * @len: length of buffer > > + * > > + * 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. > > + */ > > +void asn1_encode_integer(unsigned char **_data, s64 integer, int > > len) > > +{ > > + unsigned char *data = *_data, *d = &data[2]; > > + int i; > > + bool found = false; > > + > > + if (WARN(integer < 0, > > + "BUG: asn1_encode_integer only supports positive > > integers")) > > + return; > > + > > + if (WARN(len < 3, > > + "BUG: buffer for integers must have at least 3 > > bytes")) > > + return; > > + > > + 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; > > + found = true; > > + if (byte & 0x80) { > > + /* > > + * no check needed here, we already know we > > + * have len >= 1 > > + */ > > + *d++ = 0; > > + len--; > > + } > > + if (WARN(len == 0, > > + "BUG buffer too short in > > asn1_encode_integer")) > > + return; > > + *d++ = byte; > > + len--; > > + } > > + out: > > + data[1] = d - data - 2; > > + *_data = d; > > +} > > +EXPORT_SYMBOL_GPL(asn1_encode_integer); > > > Didn't you say you were going to make it return an error when it ran > out of space or was asked to encode a negative number? it follows the pattern of all the other functions in that it dumps a kernel warning on problems and bails. I don't really want to add error handling for my use case, since it's not expected to have any problems. My main problem case is a malicious user tricking the kernel into trying to overflow the output buffer and in that case I don't really care that the ASN.1 output will be malformed as long as the buffer doesn't overflow. James > There are other encoding functions which you haven't yet added the > buffer length field to, and they'll want to be able to return -ENOSPC > too.
James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > > Didn't you say you were going to make it return an error when it ran > > out of space or was asked to encode a negative number? > > it follows the pattern of all the other functions in that it dumps a > kernel warning on problems and bails. I don't see any bounds checking there, let alone anything that dumps a kernel warning and bails - except if the length is so large that the ASN.1 cannot be constructed. That said, there is a check in patch 4. > ... as long as the buffer doesn't overflow. Yes, but that's Dave's point. [from patch 4] > + * Assume both ovtet strings will encode to a 2 byte definite length octet, btw. At least I've found a preliminary bounds check there > + */ > + if (WARN(work - scratch + pub_len + priv_len + 8 > SCRATCH_SIZE, > + "BUG: scratch buffer is too small")) > + return -EINVAL; which I presume makes the correct calculation. I thought TPM comms were slow - slow enough that putting bounds checking in your asn1_encode_* routines will be insignificant. Further, you're promoting this ASN.1 encoder as a general library within the kernel, presumably so that other people can make use of it. Please therefore put bounds checking and error handling in it. And please *don't* just produce broken ASN.1 when something goes wrong. David
On Tue, 2019-12-10 at 14:08 +0000, David Howells wrote: > James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > > > > Didn't you say you were going to make it return an error when it > > > ran out of space or was asked to encode a negative number? > > > > it follows the pattern of all the other functions in that it dumps > > a kernel warning on problems and bails. > > I don't see any bounds checking there, let alone anything that dumps > a kernel warning and bails It's in the if (WARN part of asn1_encode_integer. > - except if the length is so large that the ASN.1 cannot be > constructed. That said, there is a check in patch 4. However, I think you'd like both a length and a buffer length to each function to cope with definite length encoding overflows? I can do that. > > ... as long as the buffer doesn't overflow. > > Yes, but that's Dave's point. > > [from patch 4] > > > + * Assume both ovtet strings will encode to a 2 byte > > definite length > > octet, btw. Heh, yes, noticed that mere seconds after I pressed send ... > At least I've found a preliminary bounds check there > > > + */ > > + if (WARN(work - scratch + pub_len + priv_len + 8 > > > SCRATCH_SIZE, > > + "BUG: scratch buffer is too small")) > > + return -EINVAL; > > which I presume makes the correct calculation. > > I thought TPM comms were slow - slow enough that putting bounds > checking in your asn1_encode_* routines will be insignificant. Yes, I'm not bothered about timings. I can add bounds checking on the buffer length like the integer case. For the other routines, I'll make it decrement the data length in place as it increments the data pointer > Further, you're promoting this ASN.1 encoder as a general library > within the kernel, presumably so that other people can make use of > it. Well, I did notice the TPM 1.2 asymmetric key code rolled its own ASN.1 encoding, yes. > Please therefore put bounds checking and error handling in it. And > please *don't* just produce broken ASN.1 when something goes wrong. OK, I'll make it return an error and add a wrapper for my use case that warns on error and causes the function to bail. James
On 10 December 2019 18:53:40 GMT, James Bottomley <James.Bottomley@HansenPartnership.com> wrote: >On Tue, 2019-12-10 at 14:08 +0000, David Howells wrote: >> Please therefore put bounds checking and error handling in it. And >> please *don't* just produce broken ASN.1 when something goes wrong. > >OK, I'll make it return an error and add a wrapper for my use case that >warns on error and causes the function to bail. Traditionally we call that "error handling" :p
On Tue, 2019-12-10 at 22:37 +0000, David Woodhouse wrote: > > On 10 December 2019 18:53:40 GMT, James Bottomley <James.Bottomley@Ha > nsenPartnership.com> wrote: > > On Tue, 2019-12-10 at 14:08 +0000, David Howells wrote: > > > Please therefore put bounds checking and error handling in > > > it. And > > > please *don't* just produce broken ASN.1 when something goes > > > wrong. > > > > OK, I'll make it return an error and add a wrapper for my use case > > that > > warns on error and causes the function to bail. > > Traditionally we call that "error handling" :p This is what I'm thinking (still reworking the rest of the series ... I found out that AA doesn't habitually have power sockets and my laptop dying in the middle of a rebase turned out not to be such a good thing). James --- From 7b0c52cf07ca2b8b8ddbe0442a6d3f9de30f7b1b Mon Sep 17 00:00:00 2001 From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: Wed, 4 Dec 2019 15:19:01 -0800 Subject: [PATCH] lib: add asn.1 encoder 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> --- v2: updated API to use indefinite length, and made symbol exports gpl diff --git a/include/linux/asn1_encoder.h b/include/linux/asn1_encoder.h new file mode 100644 index 000000000000..f4afe5ad79a8 --- /dev/null +++ b/include/linux/asn1_encoder.h @@ -0,0 +1,24 @@ +/* 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)) +int asn1_encode_integer(unsigned char **_data, int *data_len, + s64 integer); +int asn1_encode_oid(unsigned char **_data, int *data_len, + u32 oid[], int oid_len); +int asn1_encode_tag(unsigned char **data, int *data_len, u32 tag, + const unsigned char *string, int len); +int asn1_encode_octet_string(unsigned char **data, int *data_len, + const unsigned char *string, u32 len); +int asn1_encode_sequence(unsigned char **data, int *data_len, + const unsigned char *seq, int len); +int asn1_encode_boolean(unsigned char **data, int *data_len, 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..c3c03bad7e6a --- /dev/null +++ b/lib/asn1_encoder.c @@ -0,0 +1,362 @@ +// 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 + * @data_len: length of buffer remaining + * @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. + */ +int asn1_encode_integer(unsigned char **_data, int *data_len, s64 integer) +{ + unsigned char *data = *_data, *d = &data[2]; + int i; + bool found = false; + + if (WARN(integer < 0, + "BUG: integer encode only supports positive integers")) + return -EINVAL; + + if (*data_len < 3) + return -EINVAL; + + *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; + found = true; + if (byte & 0x80) { + /* + * no check needed here, we already know we + * have len >= 1 + */ + *d++ = 0; + (*data_len)--; + } + if (*data_len == 0) + return -EINVAL; + *d++ = byte; + (*data_len)--; + } + out: + data[1] = d - data - 2; + *_data = d; + return 0; +} +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) +{ + int start = 7 + 7 + 7 + 7; + unsigned char *data = *_data; + 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 + * @data_len: remaining bytes 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 + */ +int asn1_encode_oid(unsigned char **_data, int *data_len, + u32 oid[], int oid_len) +{ + unsigned char *data = *_data; + unsigned char *d = data + 2; + int i, ret; + + if (WARN(oid_len < 2, "OID must have at least two elements")) + return -EINVAL; + if (WARN(oid_len > 32, "OID is too large")) + return -EINVAL; + if (*data_len < 2) + return -EINVAL; + + data[0] = _tag(UNIV, PRIM, OID); + *d++ = oid[0] * 40 + oid[1]; + *data_len -= 2; + ret = 0; + for (i = 2; i < oid_len; i++) { + ret = asn1_encode_oid_digit(&d, data_len, oid[i]); + if (ret < 0) + return ret; + } + data[1] = d - data - 2; + *_data = d; + return ret; +} +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; + (*data_len)--; + if (len < 0) { + *((*data)++) = ASN1_INDEFINITE_LENGTH; + return 0; + } + if (*data_len < 1) + return -EINVAL; + 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 + * @data_len: remaining size of @data buffer + * @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. For the recode case, set @data_len to NULL + */ +int asn1_encode_tag(unsigned char **data, int *data_len, u32 tag, + const unsigned char *string, int len) +{ + int ret, dummy_len = 2; + + if (WARN(tag > 30, "ASN.1 tag can't be > 30")) + return -EINVAL; + + if (!string && WARN(len > 127, + "BUG: recode tag is too big (>127)")) + return -EINVAL; + + 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 = &dummy_len; + } + + if (*data_len < 2) + return -EINVAL; + + *((*data)++) = _tagn(CONT, CONS, tag); + (*data_len)--; + ret = asn1_encode_length(data, data_len, len); + if (ret < 0) + return ret; + if (!string) + return 0; + if (*data_len < len) + return -EINVAL; + memcpy(*data, string, len); + *data += len; + *data_len -= len; + + return 0; +} +EXPORT_SYMBOL_GPL(asn1_encode_tag); + +/** + * asn1_encode_octet_string - encode an ASN.1 OCTET STRING + * @data: pointer to encode at + * @data_len: bytes remaining in @data buffer + * @string: string to be encoded + * @len: length of string + * + * Note ASN.1 octet strings may contain zeros, so the length is obligatory. + */ +int asn1_encode_octet_string(unsigned char **data, int *data_len, + const unsigned char *string, u32 len) +{ + int ret; + + if (*data_len < 2) + return -EINVAL; + + *((*data)++) = _tag(UNIV, PRIM, OTS); + (*data_len)--; + ret = asn1_encode_length(data, data_len, len); + if (ret) + return ret; + + if (*data_len < len) + return -EINVAL; + + memcpy(*data, string, len); + *data += len; + *data_len -= len; + + return 0; +} +EXPORT_SYMBOL_GPL(asn1_encode_octet_string); + +/** + * asn1_encode_sequence - wrap a byte stream in an ASN.1 SEQUENCE + * @data: pointer to encode at + * @data_len: remaining size of @data pointer + * @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. The recode case should pass NULL + * to @data_size + */ +int asn1_encode_sequence(unsigned char **data, int *data_len, + const unsigned char *seq, int len) +{ + int ret, dummy_len = 2; + + if (!seq && WARN(len > 127, + "BUG: recode sequence is too big (>127)")) + return -EINVAL; + 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 = &dummy_len; + } + + if (*data_len < 2) + return -EINVAL; + + *((*data)++) = _tag(UNIV, CONS, SEQ); + (*data_len)--; + ret = asn1_encode_length(data, data_len, len); + if (ret) + return ret; + if (!seq) + return 0; + + if (*data_len < len) + return -EINVAL; + + memcpy(*data, seq, len); + *data += len; + *data_len -= len; + + return 0; +} +EXPORT_SYMBOL_GPL(asn1_encode_sequence); + +/** + * asn1_encode_boolean - encode a boolean value to ASN.1 + * @data: pointer to encode at + * @data_len: bytes remaining in @data buffer + * @val: the boolean true/false value + */ +int asn1_encode_boolean(unsigned char **data, int *data_len, bool val) +{ + if (*data_len < 2) + return -EINVAL; + *((*data)++) = _tag(UNIV, PRIM, BOOL); + asn1_encode_length(data, data_len, 1); + (*data_len)--; + *((*data)++) = val ? 1 : 0; + + return 0; +} +EXPORT_SYMBOL_GPL(asn1_encode_boolean);
James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > +/** > + * asn1_encode_octet_string - encode an ASN.1 OCTET STRING > + * @data: pointer to encode at > + * @data_len: bytes remaining in @data buffer > + * @string: string to be encoded > + * @len: length of string > + * > + * Note ASN.1 octet strings may contain zeros, so the length is obligatory. > + */ > +int asn1_encode_octet_string(unsigned char **data, int *data_len, > + const unsigned char *string, u32 len) I wonder if it makes more sense to pass in an end pointer and return the new data pointer (or an error), ie.: unsigned char *asn1_encode_octet_string(unsigned char *data, unsigned char *data_end, const unsigned char *string, u32 len) Further, I wonder - does it actually make more sense to encode backwards, ie. start at the end of the buffer and do the last element first, working towards the front. The disadvantage being that the data start would likely not be coincident with the buffer start. David
On Wed, 2019-12-18 at 10:50 +0000, David Howells wrote: > James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > > > +/** > > + * asn1_encode_octet_string - encode an ASN.1 OCTET STRING > > + * @data: pointer to encode at > > + * @data_len: bytes remaining in @data buffer > > + * @string: string to be encoded > > + * @len: length of string > > + * > > + * Note ASN.1 octet strings may contain zeros, so the length is > > obligatory. > > + */ > > +int asn1_encode_octet_string(unsigned char **data, int *data_len, > > + const unsigned char *string, u32 len) > > I wonder if it makes more sense to pass in an end pointer and return > the new data pointer (or an error), ie.: > > unsigned char *asn1_encode_octet_string(unsigned char *data, > unsigned char *data_end, > const unsigned char *string, > u32 len) On the first point: people are prone to get off by one confusion on data_end pointers (should they point to the last byte in the buffer or one beyond). If I look at how I use the API, I've no real use for either length remaining or the end pointer, so I think it makes no difference to the consumer, it's just stuff you have to do for the API. If I look at the internal API use, we definitely need the length remaining, so I've a marginal preference for that format, but since it's easy to work out it is very marginal. > Further, I wonder - does it actually make more sense to encode > backwards, ie. start at the end of the buffer and do the last element > first, working towards the front. Heh, let me ask you this: do you use a reverse polish notation calculator ... The problem is that it makes the ASN.1 hard to construct for the API user and hard to read for the reviewer because of the order reversal. Debugging is going to be a pain because you're going to get the output of asn1parse and have to read it backwards to see where the problems are. > The disadvantage being that the data start would likely not be > coincident with the buffer start. This would be a big issue: in several routines I allocate a buffer, fill it with ASN.1 and pass it back and the receiving routine has to free it. Now the buffer won't be freeable by the pointer I pass back because that may not be where the allocation was done. For these two reasons, I'd like to keep the work forwards behaviour. I'm reasonably ambivalent on the end pointer with a marginal preference for passing in the length remaining instead. James
On Thu, 2019-12-19 at 08:10 +0900, James Bottomley wrote: > On Wed, 2019-12-18 at 10:50 +0000, David Howells wrote: > > James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > > > > > +/** > > > + * asn1_encode_octet_string - encode an ASN.1 OCTET STRING > > > + * @data: pointer to encode at > > > + * @data_len: bytes remaining in @data buffer > > > + * @string: string to be encoded > > > + * @len: length of string > > > + * > > > + * Note ASN.1 octet strings may contain zeros, so the length is > > > obligatory. > > > + */ > > > +int asn1_encode_octet_string(unsigned char **data, int > > > *data_len, > > > + const unsigned char *string, u32 > > > len) > > > > I wonder if it makes more sense to pass in an end pointer and > > return > > the new data pointer (or an error), ie.: > > > > unsigned char *asn1_encode_octet_string(unsigned char *data, > > unsigned char *data_end, > > const unsigned char *string, > > u32 len) > > On the first point: people are prone to get off by one confusion on > data_end pointers (should they point to the last byte in the buffer > or > one beyond). If I look at how I use the API, I've no real use for > either length remaining or the end pointer, so I think it makes no > difference to the consumer, it's just stuff you have to do for the > API. > If I look at the internal API use, we definitely need the length > remaining, so I've a marginal preference for that format, but since > it's easy to work out it is very marginal. > > > Further, I wonder - does it actually make more sense to encode > > backwards, ie. start at the end of the buffer and do the last > > element > > first, working towards the front. > > Heh, let me ask you this: do you use a reverse polish notation > calculator ... The problem is that it makes the ASN.1 hard to > construct for the API user and hard to read for the reviewer because > of the order reversal. Debugging is going to be a pain because > you're going to get the output of asn1parse and have to read it > backwards to see where the problems are. I coded this up to see what it would look like, and I think it can all be made to work with error pass through. The latter is because you want to build up sequences of data = asn1_encode...(data, ...); data = asn1_encode...(data, ...); data = asn1_encode...(data, ...); And only check for errors when you're finished. I think the interface looks nicer than a modifying pointer, so if you wait for the v4 patches they'll show this new interface with the consumers. James
diff --git a/include/linux/asn1_encoder.h b/include/linux/asn1_encoder.h new file mode 100644 index 000000000000..9cfb8035dc46 --- /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, s64 integer, int len); +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, int 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, + int 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..768cabf8bf76 --- /dev/null +++ b/lib/asn1_encoder.c @@ -0,0 +1,258 @@ +// 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 + * @len: length of buffer + * + * 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. + */ +void asn1_encode_integer(unsigned char **_data, s64 integer, int len) +{ + unsigned char *data = *_data, *d = &data[2]; + int i; + bool found = false; + + if (WARN(integer < 0, + "BUG: asn1_encode_integer only supports positive integers")) + return; + + if (WARN(len < 3, + "BUG: buffer for integers must have at least 3 bytes")) + return; + + 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; + found = true; + if (byte & 0x80) { + /* + * no check needed here, we already know we + * have len >= 1 + */ + *d++ = 0; + len--; + } + if (WARN(len == 0, + "BUG buffer too short in asn1_encode_integer")) + return; + *d++ = byte; + len--; + } + out: + data[1] = d - data - 2; + *_data = d; +} +EXPORT_SYMBOL_GPL(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_GPL(asn1_encode_oid); + +static void asn1_encode_length(unsigned char **data, int len) +{ + if (len < 0) { + *((*data)++) = ASN1_INDEFINITE_LENGTH; + return; + } + 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. 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. + */ +void asn1_encode_tag(unsigned char **data, u32 tag, + const unsigned char *string, int len) +{ + if (WARN(tag > 30, "ASN.1 tag can't be > 30")) + return; + + if (!string && WARN(len > 127, + "BUG: recode tag is too big (>127)")) + return; + + if (!string && len > 0) + /* we're recoding, so move back to the start of the tag */ + *data -= 2; + + *((*data)++) = _tagn(CONT, CONS, tag); + asn1_encode_length(data, len); + if (!string) + return; + memcpy(*data, string, len); + *data += len; +} +EXPORT_SYMBOL_GPL(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_length(data, len); + memcpy(*data, string, len); + *data += len; +} +EXPORT_SYMBOL_GPL(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 + * + * 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. + */ +void asn1_encode_sequence(unsigned char **data, const unsigned char *seq, + int len) +{ + if (!seq && WARN(len > 127, + "BUG: recode sequence is too big (>127)")) + return; + if (!seq && len > 0) + /* we're recoding, so move back to the start of the sequence */ + *data -= 2; + + *((*data)++) = _tag(UNIV, CONS, SEQ); + asn1_encode_length(data, len); + if (!seq) + return; + memcpy(*data, seq, len); + *data += len; +} +EXPORT_SYMBOL_GPL(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_length(data, 1); + *((*data)++) = val ? 1 : 0; +} +EXPORT_SYMBOL_GPL(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> --- v2: updated API to use indefinite length, and made symbol exports gpl --- include/linux/asn1_encoder.h | 21 ++++ lib/Makefile | 2 +- lib/asn1_encoder.c | 258 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 280 insertions(+), 1 deletion(-) create mode 100644 include/linux/asn1_encoder.h create mode 100644 lib/asn1_encoder.c