| Message ID | 163673838611.45802.5085223391786276660.stgit@morisot.1015granger.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [Strawman] crypto: Handle PEM-encoded x.509 certificates | expand |
On Fri, Nov 12, 2021 at 12:39:52PM -0500, Chuck Lever wrote: > This enables "# cat cert.pem | keyctl padd asymmetric <keyring>" > > Since prep->data is a "const void *" I didn't feel comfortable with > pem_decode() simply overwriting either the pointer or the contents > of the provided buffer. A secondary buffer is therefore allocated, > and then later freed by .free_preparse. > > This compiles, but is otherwise untested. I'm interested in opinions > about this approach. > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> Why? You can easily convert PEM to DER in userspace, for example with a command like 'openssl x509 -in cert.pem -out cert.der -outform der'. There's no need for the kernel to do it. - Eric
Hi Eric- > On Nov 12, 2021, at 1:49 PM, Eric Biggers <ebiggers@kernel.org> wrote: > > On Fri, Nov 12, 2021 at 12:39:52PM -0500, Chuck Lever wrote: >> This enables "# cat cert.pem | keyctl padd asymmetric <keyring>" >> >> Since prep->data is a "const void *" I didn't feel comfortable with >> pem_decode() simply overwriting either the pointer or the contents >> of the provided buffer. A secondary buffer is therefore allocated, >> and then later freed by .free_preparse. >> >> This compiles, but is otherwise untested. I'm interested in opinions >> about this approach. >> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > > Why? You can easily convert PEM to DER in userspace, for example with a command > like 'openssl x509 -in cert.pem -out cert.der -outform der'. There's no need > for the kernel to do it. Correct, it's not a hard requirement. However, this change does broaden the add_key(2) API to enable it to accept a PEM-encoded certificate directly instead of needing to run the blob through an intermediate step, simplifying applications that might use it for adding X.509 keys to kernel keyrings. Certainly, the kernel could include a single set of base64 encoders and decoders that can be used by all in-kernel consumers. See for example net/ceph/armor.c and fs/crypto/fname.c . Because PEM decoding does not require any policy decisions, and because the kernel already has at least two existing partial base64 implementations, I'm not aware of a technical reason a system call like add_key(2) should not to accept PEM-encoded asymmetric key material. -- Chuck Lever
On Sat, Nov 13, 2021 at 07:12:44PM +0000, Chuck Lever III wrote: > > Certainly, the kernel could include a single set of base64 encoders > and decoders that can be used by all in-kernel consumers. See for > example net/ceph/armor.c and fs/crypto/fname.c . Not really, there are many variants of Base64 and different policy decisions that can be made: the chosen character set, whether to pad or not pad, whether to allow whitespace, how to handle invalid characters, how to handle invalid padding, whether to nul-terminate, and so on. There's lots of room for bugs and incompatibilities. > > Because PEM decoding does not require any policy decisions, and > because the kernel already has at least two existing partial > base64 implementations, I'm not aware of a technical reason a > system call like add_key(2) should not to accept PEM-encoded > asymmetric key material. Adding kernel UAPIs expands the kernel's attack surface, causing security vulnerabilities. It also increases the number of UAPIs that need to be permanently supported. It makes no sense to add kernel UAPIs for things that can be easily done in userspace. They work well as April Fools' jokes, though: https://lore.kernel.org/r/1459463613-32473-1-git-send-email-richard@nod.at Perhaps you meant to save your patch for April 1? - Eric
> On Nov 13, 2021, at 6:02 PM, Eric Biggers <ebiggers@kernel.org> wrote: > > On Sat, Nov 13, 2021 at 07:12:44PM +0000, Chuck Lever III wrote: >> >> Certainly, the kernel could include a single set of base64 encoders >> and decoders that can be used by all in-kernel consumers. See for >> example net/ceph/armor.c and fs/crypto/fname.c . > > Not really, there are many variants of Base64 and different policy decisions > that can be made: the chosen character set, whether to pad or not pad, whether > to allow whitespace, how to handle invalid characters, how to handle invalid > padding, whether to nul-terminate, and so on. There's lots of room for bugs and > incompatibilities. > >> >> Because PEM decoding does not require any policy decisions, and >> because the kernel already has at least two existing partial >> base64 implementations, I'm not aware of a technical reason a >> system call like add_key(2) should not to accept PEM-encoded >> asymmetric key material. > > Adding kernel UAPIs expands the kernel's attack surface, causing security > vulnerabilities. It also increases the number of UAPIs that need to be > permanently supported. It makes no sense to add kernel UAPIs for things that > can be easily done in userspace. > > They work well as April Fools' jokes, though: > https://lore.kernel.org/r/1459463613-32473-1-git-send-email-richard@nod.at > Perhaps you meant to save your patch for April 1? That remark is uncalled for and out of line. Perhaps you just don't know what "strawman" means or why someone would post unfinished code to ask for direction. I'll mark that down to your inexperience. Interestingly, I don't see you listed as a maintainer in this area: $ scripts/get_maintainer.pl crypto/asymmetric_keys/ David Howells <dhowells@redhat.com> (maintainer:ASYMMETRIC KEYS) Herbert Xu <herbert@gondor.apana.org.au> (maintainer:CRYPTO API) "David S. Miller" <davem@davemloft.net> (maintainer:CRYPTO API) keyrings@vger.kernel.org (open list:ASYMMETRIC KEYS) linux-crypto@vger.kernel.org (open list:CRYPTO API) linux-kernel@vger.kernel.org (open list) $ I actually /have/ talked with one of these maintainers, and he suggested PEM decoding under add_key(2) would be appropriate and valuable. It actually wasn't my idea. I shall credit his idea in the next version of this patch so there won't be any further confusion. -- Chuck Lever
On Sun, Nov 14, 2021 at 02:34:07AM +0000, Chuck Lever III wrote: > > Adding kernel UAPIs expands the kernel's attack surface, causing security > > vulnerabilities. It also increases the number of UAPIs that need to be > > permanently supported. It makes no sense to add kernel UAPIs for things that > > can be easily done in userspace. > > > > They work well as April Fools' jokes, though: > > https://lore.kernel.org/r/1459463613-32473-1-git-send-email-richard@nod.at > > Perhaps you meant to save your patch for April 1? > > That remark is uncalled for and out of line. Perhaps you just > don't know what "strawman" means or why someone would post > unfinished code to ask for direction. I'll mark that down to > your inexperience. > > Interestingly, I don't see you listed as a maintainer in this > area: > > $ scripts/get_maintainer.pl crypto/asymmetric_keys/ > David Howells <dhowells@redhat.com> (maintainer:ASYMMETRIC KEYS) > Herbert Xu <herbert@gondor.apana.org.au> (maintainer:CRYPTO API) > "David S. Miller" <davem@davemloft.net> (maintainer:CRYPTO API) > keyrings@vger.kernel.org (open list:ASYMMETRIC KEYS) > linux-crypto@vger.kernel.org (open list:CRYPTO API) > linux-kernel@vger.kernel.org (open list) > $ > > I actually /have/ talked with one of these maintainers, and he > suggested PEM decoding under add_key(2) would be appropriate and > valuable. It actually wasn't my idea. I shall credit his idea in > the next version of this patch so there won't be any further > confusion. It's not appropriate to add UAPIs with no regards for increasing the kernel's attack surface, especially for things that can easily be done in userspace. The kernel community is already struggling with thousands of syzbot reports and constant security vulnerabilites. I understand that your patch is not yet finished, but it doesn't really matter; this is no need for this patch at all as you can just convert PEM => DER in userspace. PEM decoding is just some data processing which can be implemented in userspace in any programming language, so it's not fundamentally different from sys_leftpad(). So in my opinion the comparison is relevant. - Eric
diff --git a/crypto/asymmetric_keys/Makefile b/crypto/asymmetric_keys/Makefile index 28b91adba2ae..1df8c976dd2f 100644 --- a/crypto/asymmetric_keys/Makefile +++ b/crypto/asymmetric_keys/Makefile @@ -8,7 +8,8 @@ obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += asymmetric_keys.o asymmetric_keys-y := \ asymmetric_type.o \ restrict.o \ - signature.o + signature.o \ + pem.o obj-$(CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE) += public_key.o obj-$(CONFIG_ASYMMETRIC_TPM_KEY_SUBTYPE) += asym_tpm.o diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c index ad8af3d70ac0..f5d810c079b6 100644 --- a/crypto/asymmetric_keys/asymmetric_type.c +++ b/crypto/asymmetric_keys/asymmetric_type.c @@ -12,10 +12,12 @@ #include <linux/seq_file.h> #include <linux/module.h> #include <linux/slab.h> +#include <linux/vmalloc.h> #include <linux/ctype.h> #include <keys/system_keyring.h> #include <keys/user-type.h> #include "asymmetric_keys.h" +#include "pem.h" MODULE_LICENSE("GPL"); @@ -378,6 +380,10 @@ static int asymmetric_key_preparse(struct key_preparsed_payload *prep) if (prep->datalen == 0) return -EINVAL; + ret = pem_decode(prep); + if (ret < 0) + return ret; + down_read(&asymmetric_key_parsers_sem); ret = -EBADMSG; @@ -428,6 +434,7 @@ static void asymmetric_key_free_preparse(struct key_preparsed_payload *prep) } asymmetric_key_free_kids(kids); kfree(prep->description); + vfree(prep->decoded); } /* diff --git a/crypto/asymmetric_keys/pem.c b/crypto/asymmetric_keys/pem.c new file mode 100644 index 000000000000..b989fe7c1049 --- /dev/null +++ b/crypto/asymmetric_keys/pem.c @@ -0,0 +1,185 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Unwrap a PEM-encoded asymmetric key. This implementation unwraps + * the interoperable text encoding format specified in RFC 7468. + * + * Author: Chuck Lever <chuck.lever@oracle.com> + * + * Copyright (c) 2021, Oracle and/or its affiliates. + */ + +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/vmalloc.h> +#include <linux/key-type.h> + +#include "pem.h" + +/* Encapsulation boundaries */ +#define PEM_EB_MARKER "-----" +#define PEM_BEGIN_MARKER PEM_EB_MARKER "BEGIN" +#define PEM_END_MARKER PEM_EB_MARKER "END" + +/* + * Unremarkable table-driven base64 decoder based on the public domain + * implementation provided at: + * https://en.wikibooks.org/wiki/Algorithm_Implementation/Miscellaneous/Base64 + * + * XXX: Replace this implementation with one that handles EBCDIC input properly. + */ + +#define WHITESPACE 253 +#define EQUALS 254 +#define INVALID 255 + +static const u8 alphabet[] = { + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, WHITESPACE, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, 62, INVALID, INVALID, INVALID, 63, + 52, 53, 54, 55, 56, 57, 58, 59, + 60, 61, INVALID, INVALID, INVALID, EQUALS, INVALID, INVALID, + INVALID, 0, 1, 2, 3, 4, 5, 6, + 7, 8, 9, 10, 11, 12, 13, 14, + 15, 16, 17, 18, 19, 20, 21, 22, + 23, 24, 25, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, 26, 27, 28, 29, 30, 31, 32, + 33, 34, 35, 36, 37, 38, 39, 40, + 41, 42, 43, 44, 45, 46, 47, 48, + 49, 50, 51, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, + INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, INVALID, +}; + +static bool base64decode(unsigned char *in, size_t inLen, + unsigned char *out, size_t *outLen) +{ + unsigned char *end = in + inLen; + char iter = 0; + uint32_t buf = 0; + size_t len = 0; + + while (in < end) { + u8 c = alphabet[*in++]; + + switch (c) { + case WHITESPACE: + continue; + case INVALID: + return false; + case EQUALS: + in = end; + continue; + default: + buf = buf << 6 | c; + iter++; + + if (iter == 4) { + if ((len += 3) > *outLen) + return false; + *(out++) = (buf >> 16) & 255; + *(out++) = (buf >> 8) & 255; + *(out++) = buf & 255; + buf = 0; + iter = 0; + } + } + } + + if (iter == 3) { + if ((len += 2) > *outLen) + return false; + *(out++) = (buf >> 10) & 255; + *(out++) = (buf >> 2) & 255; + } else if (iter == 2) { + if (++len > *outLen) + return false; + *(out++) = (buf >> 4) & 255; + } + + *outLen = len; + return true; +} + +/** + * pem_decode - Attempt to decode a PEM-encoded data blob. + * @prep: Data content to examine + * + * Assumptions: + * - The input data buffer is not more than a few pages in size. + * - The input data buffer has already been vetted for proper + * kernel read access. + * - The input data buffer might not be NUL-terminated. + * + * PEM type labels are ignored. Subsequent parsing of the + * decoded message adequately identifies its content. + * + * On success, a pointer to a dynamically-allocated buffer + * containing the decoded content is returned. This buffer is + * vfree'd in the .free_preparse method. + * + * Return values: + * %1: @prep.decoded points to the decoded message + * %0: @prep did not contain a PEM-encoded message + * + * A negative errno is returned if an unexpected error has + * occurred (eg, memory exhaustion). + */ +int pem_decode(struct key_preparsed_payload *prep) +{ + const unsigned char *in = prep->data; + unsigned char *begin, *end, *out; + size_t outlen; + + prep->decoded = NULL; + prep->decoded_len = 0; + + /* Find the beginning encapsulation boundary */ + begin = strnstr(in, PEM_BEGIN_MARKER, prep->datalen); + if (!begin) + goto out_not_pem; + begin = strnstr(begin, PEM_EB_MARKER, begin - in); + if (!begin) + goto out_not_pem; + begin += sizeof(PEM_EB_MARKER); + + /* Find the ending encapsulation boundary */ + end = strnstr(begin, PEM_END_MARKER, begin - in); + if (!end) + goto out_not_pem; + if (!strnstr(end, PEM_EB_MARKER, end - in)) + goto out_not_pem; + end--; + + /* Attempt to decode */ + out = vmalloc(end - begin); + if (!out) + return -ENOMEM; + if (!base64decode(begin, end - begin, out, &outlen)) { + vfree(out); + goto out_not_pem; + } + + prep->decoded = out; + prep->decoded_len = outlen; + return 1; + +out_not_pem: + return 0; +} diff --git a/crypto/asymmetric_keys/pem.h b/crypto/asymmetric_keys/pem.h new file mode 100644 index 000000000000..51b9db517f94 --- /dev/null +++ b/crypto/asymmetric_keys/pem.h @@ -0,0 +1,8 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __ASYMMETRIC_PEM_H +#define __ASYMMETRIC_PEM_H + +int pem_decode(struct key_preparsed_payload *prep); + +#endif /* __ASYMMETRIC_PEM_H */ diff --git a/crypto/asymmetric_keys/pkcs8_parser.c b/crypto/asymmetric_keys/pkcs8_parser.c index 105dcce27f71..b7ce76d3a767 100644 --- a/crypto/asymmetric_keys/pkcs8_parser.c +++ b/crypto/asymmetric_keys/pkcs8_parser.c @@ -137,7 +137,10 @@ static int pkcs8_key_preparse(struct key_preparsed_payload *prep) { struct public_key *pub; - pub = pkcs8_parse(prep->data, prep->datalen); + if (prep->decoded) + pub = pkcs8_parse(prep->decoded, prep->decoded_len); + else + pub = pkcs8_parse(prep->data, prep->datalen); if (IS_ERR(pub)) return PTR_ERR(pub); diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c index 3d45161b271a..d8b559ac6d7c 100644 --- a/crypto/asymmetric_keys/x509_public_key.c +++ b/crypto/asymmetric_keys/x509_public_key.c @@ -167,7 +167,10 @@ static int x509_key_preparse(struct key_preparsed_payload *prep) char *desc = NULL, *p; int ret; - cert = x509_cert_parse(prep->data, prep->datalen); + if (prep->decoded) + cert = x509_cert_parse(prep->decoded, prep->decoded_len); + else + cert = x509_cert_parse(prep->data, prep->datalen); if (IS_ERR(cert)) return PTR_ERR(cert); diff --git a/include/linux/key-type.h b/include/linux/key-type.h index 7d985a1dfe4a..1672f9599afa 100644 --- a/include/linux/key-type.h +++ b/include/linux/key-type.h @@ -34,6 +34,8 @@ struct key_preparsed_payload { union key_payload payload; /* Proposed payload */ const void *data; /* Raw data */ size_t datalen; /* Raw datalen */ + const void *decoded; /* PEM-decoded data */ + size_t decoded_len; /* Length of PEM-decoded data */ size_t quotalen; /* Quota length for proposed payload */ time64_t expiry; /* Expiry time of key */ } __randomize_layout;
This enables "# cat cert.pem | keyctl padd asymmetric <keyring>" Since prep->data is a "const void *" I didn't feel comfortable with pem_decode() simply overwriting either the pointer or the contents of the provided buffer. A secondary buffer is therefore allocated, and then later freed by .free_preparse. This compiles, but is otherwise untested. I'm interested in opinions about this approach. Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- crypto/asymmetric_keys/Makefile | 3 crypto/asymmetric_keys/asymmetric_type.c | 7 + crypto/asymmetric_keys/pem.c | 185 ++++++++++++++++++++++++++++++ crypto/asymmetric_keys/pem.h | 8 + crypto/asymmetric_keys/pkcs8_parser.c | 5 + crypto/asymmetric_keys/x509_public_key.c | 5 + include/linux/key-type.h | 2 7 files changed, 212 insertions(+), 3 deletions(-) create mode 100644 crypto/asymmetric_keys/pem.c create mode 100644 crypto/asymmetric_keys/pem.h