| Message ID | 20210114151909.2344974-7-mic@digikod.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable root to update the blacklist keyring | expand |
On Thu, Jan 14, 2021 at 04:19:05PM +0100, Mickaël Salaün wrote: > From: Mickaël Salaün <mic@linux.microsoft.com> > > Before exposing this new key type to user space, make sure that only > meaningful blacklisted hashes are accepted. This is also checked for > builtin blacklisted hashes, but a following commit make sure that the > user will notice (at built time) and will fix the configuration if it > already included errors. > > Check that a blacklist key description starts with a valid prefix and > then a valid hexadecimal string. > > Cc: David Howells <dhowells@redhat.com> > Cc: David Woodhouse <dwmw2@infradead.org> > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> > Acked-by: Jarkko Sakkinen <jarkko@kernel.org> In this I'm not as worried about ABI, i.e. you don't have any reason supply any other data, which doesn't follow these ruels, whereas there could very well be a script that does format hex "incorrectly". /Jarkko
On 20/01/2021 05:16, Jarkko Sakkinen wrote: > On Thu, Jan 14, 2021 at 04:19:05PM +0100, Mickaël Salaün wrote: >> From: Mickaël Salaün <mic@linux.microsoft.com> >> >> Before exposing this new key type to user space, make sure that only >> meaningful blacklisted hashes are accepted. This is also checked for >> builtin blacklisted hashes, but a following commit make sure that the >> user will notice (at built time) and will fix the configuration if it >> already included errors. >> >> Check that a blacklist key description starts with a valid prefix and >> then a valid hexadecimal string. >> >> Cc: David Howells <dhowells@redhat.com> >> Cc: David Woodhouse <dwmw2@infradead.org> >> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> >> Acked-by: Jarkko Sakkinen <jarkko@kernel.org> > > In this I'm not as worried about ABI, i.e. you don't have any reason > supply any other data, which doesn't follow these ruels, whereas there > could very well be a script that does format hex "incorrectly". I think I answered this comment in patch 2/10: there is no ABI breakage, it only prepares for safe dynamic key addition. Patch 10/10 enables to avoid using incorrect/useless/mis-leading hashes and force users to fix these hashes (that were not taken into account) > > /Jarkko >
diff --git a/certs/blacklist.c b/certs/blacklist.c index bffe4c6f4a9e..334ab7b964bc 100644 --- a/certs/blacklist.c +++ b/certs/blacklist.c @@ -18,6 +18,16 @@ #include <keys/system_keyring.h> #include "blacklist.h" +/* + * According to crypto/asymmetric_keys/x509_cert_parser.c:x509_note_pkey_algo(), + * the size of the currently longest supported hash algorithm is 512 bits, + * which translates into 128 hex characters. + */ +#define MAX_HASH_LEN 128 + +static const char tbs_prefix[] = "tbs"; +static const char bin_prefix[] = "bin"; + static struct key *blacklist_keyring; /* @@ -26,24 +36,40 @@ static struct key *blacklist_keyring; */ static int blacklist_vet_description(const char *desc) { - int n = 0; - - if (*desc == ':') - return -EINVAL; - for (; *desc; desc++) - if (*desc == ':') - goto found_colon; + int i, prefix_len, tbs_step = 0, bin_step = 0; + + /* The following algorithm only works if prefix lengths match. */ + BUILD_BUG_ON(sizeof(tbs_prefix) != sizeof(bin_prefix)); + prefix_len = sizeof(tbs_prefix) - 1; + for (i = 0; *desc; desc++, i++) { + if (*desc == ':') { + if (tbs_step == prefix_len) + goto found_colon; + if (bin_step == prefix_len) + goto found_colon; + return -EINVAL; + } + if (i >= prefix_len) + return -EINVAL; + if (*desc == tbs_prefix[i]) + tbs_step++; + if (*desc == bin_prefix[i]) + bin_step++; + } return -EINVAL; found_colon: desc++; - for (; *desc; desc++) { + for (i = 0; *desc && i < MAX_HASH_LEN; desc++, i++) { if (!isxdigit(*desc) || isupper(*desc)) return -EINVAL; - n++; } + if (*desc) + /* The hash is greater than MAX_HASH_LEN. */ + return -ENOPKG; - if (n == 0 || n & 1) + /* Checks for an even number of hexadecimal characters. */ + if (i == 0 || i & 1) return -EINVAL; return 0; }