diff mbox series

[v7,3/5] certs: Make blacklist_vet_description() more strict

Message ID 20210312171232.2681989-4-mic@digikod.net (mailing list archive)
State New, archived
Headers show
Series Enable root to update the blacklist keyring | expand

Commit Message

Mickaël Salaün March 12, 2021, 5:12 p.m. UTC 
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>
Cc: Eric Snowberg <eric.snowberg@oracle.com>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Link: https://lore.kernel.org/r/20210312171232.2681989-4-mic@digikod.net
---

Changes since v5:
* Add Reviewed-by Jarkko.

Changes since v2:
* Fix typo in blacklist_vet_description() comment, spotted by Tyler
  Hicks.
* Add Jarkko's Acked-by.

Changes since v1:
* Return ENOPKG (instead of EINVAL) when a hash is greater than the
  maximum currently known hash (suggested by David Howells).
---
 certs/blacklist.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

Comments

David Howells April 20, 2022, 10:29 a.m. UTC | #1 
Mickaël Salaün <mic@digikod.net> wrote:

> +	/* 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++;
> +	}

I wonder if:

	static const char tbs_prefix[] = "tbs:";
	static const char bin_prefix[] = "bin:";

	if (strncmp(desc, tbs_prefix, sizeof(tbs_prefix) - 1) == 0 ||
	    strncmp(desc, bin_prefix, sizeof(bin_prefix) - 1) == 0)
		goto found_colon;

might be better.

David
Jarkko Sakkinen April 21, 2022, 3:12 p.m. UTC | #2 
On Wed, Apr 20, 2022 at 11:29:08AM +0100, David Howells wrote:
> Mickaël Salaün <mic@digikod.net> wrote:
> 
> > +	/* 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++;
> > +	}
> 
> I wonder if:
> 
> 	static const char tbs_prefix[] = "tbs:";
> 	static const char bin_prefix[] = "bin:";
> 
> 	if (strncmp(desc, tbs_prefix, sizeof(tbs_prefix) - 1) == 0 ||
> 	    strncmp(desc, bin_prefix, sizeof(bin_prefix) - 1) == 0)
> 		goto found_colon;
> 
> might be better.
> 
> David

I think it'd be. 

BR, Jarkko
Mickaël Salaün April 21, 2022, 3:27 p.m. UTC | #3 
On 21/04/2022 17:12, Jarkko Sakkinen wrote:
> On Wed, Apr 20, 2022 at 11:29:08AM +0100, David Howells wrote:
>> Mickaël Salaün <mic@digikod.net> wrote:
>>
>>> +	/* 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++;
>>> +	}
>>
>> I wonder if:
>>
>> 	static const char tbs_prefix[] = "tbs:";
>> 	static const char bin_prefix[] = "bin:";
>>
>> 	if (strncmp(desc, tbs_prefix, sizeof(tbs_prefix) - 1) == 0 ||
>> 	    strncmp(desc, bin_prefix, sizeof(bin_prefix) - 1) == 0)
>> 		goto found_colon;
>>
>> might be better.
>>
>> David
> 
> I think it'd be.
> 
> BR, Jarkko

I'm confused. Didn't you plan to send this patch series before 
v5.18-rc2? It's been a while since I started working on this.
Jarkko Sakkinen April 21, 2022, 3:57 p.m. UTC | #4 
On Thu, Apr 21, 2022 at 05:27:42PM +0200, Mickaël Salaün wrote:
> 
> On 21/04/2022 17:12, Jarkko Sakkinen wrote:
> > On Wed, Apr 20, 2022 at 11:29:08AM +0100, David Howells wrote:
> > > Mickaël Salaün <mic@digikod.net> wrote:
> > > 
> > > > +	/* 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++;
> > > > +	}
> > > 
> > > I wonder if:
> > > 
> > > 	static const char tbs_prefix[] = "tbs:";
> > > 	static const char bin_prefix[] = "bin:";
> > > 
> > > 	if (strncmp(desc, tbs_prefix, sizeof(tbs_prefix) - 1) == 0 ||
> > > 	    strncmp(desc, bin_prefix, sizeof(bin_prefix) - 1) == 0)
> > > 		goto found_colon;
> > > 
> > > might be better.
> > > 
> > > David
> > 
> > I think it'd be.
> > 
> > BR, Jarkko
> 
> I'm confused. Didn't you plan to send this patch series before v5.18-rc2?
> It's been a while since I started working on this.

That was my original plan but due to some other things, I've sent
a PR for rc4. I CC'd you to the PR.

BR, Jarkko
Mickaël Salaün April 21, 2022, 5:29 p.m. UTC | #5 
On 21/04/2022 17:57, Jarkko Sakkinen wrote:
> On Thu, Apr 21, 2022 at 05:27:42PM +0200, Mickaël Salaün wrote:
>>
>> On 21/04/2022 17:12, Jarkko Sakkinen wrote:
>>> On Wed, Apr 20, 2022 at 11:29:08AM +0100, David Howells wrote:
>>>> Mickaël Salaün <mic@digikod.net> wrote:
>>>>
>>>>> +	/* 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++;
>>>>> +	}
>>>>
>>>> I wonder if:
>>>>
>>>> 	static const char tbs_prefix[] = "tbs:";
>>>> 	static const char bin_prefix[] = "bin:";
>>>>
>>>> 	if (strncmp(desc, tbs_prefix, sizeof(tbs_prefix) - 1) == 0 ||
>>>> 	    strncmp(desc, bin_prefix, sizeof(bin_prefix) - 1) == 0)
>>>> 		goto found_colon;
>>>>
>>>> might be better.
>>>>
>>>> David
>>>
>>> I think it'd be.
>>>
>>> BR, Jarkko
>>
>> I'm confused. Didn't you plan to send this patch series before v5.18-rc2?
>> It's been a while since I started working on this.
> 
> That was my original plan but due to some other things, I've sent
> a PR for rc4. I CC'd you to the PR.

OK, I missed it. My micro-optimization isn't worth it, strncmp is much 
simple indeed.
Jarkko Sakkinen April 22, 2022, 8:54 a.m. UTC | #6 
On Thu, Apr 21, 2022 at 07:29:10PM +0200, Mickaël Salaün wrote:
> 
> On 21/04/2022 17:57, Jarkko Sakkinen wrote:
> > On Thu, Apr 21, 2022 at 05:27:42PM +0200, Mickaël Salaün wrote:
> > > 
> > > On 21/04/2022 17:12, Jarkko Sakkinen wrote:
> > > > On Wed, Apr 20, 2022 at 11:29:08AM +0100, David Howells wrote:
> > > > > Mickaël Salaün <mic@digikod.net> wrote:
> > > > > 
> > > > > > +	/* 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++;
> > > > > > +	}
> > > > > 
> > > > > I wonder if:
> > > > > 
> > > > > 	static const char tbs_prefix[] = "tbs:";
> > > > > 	static const char bin_prefix[] = "bin:";
> > > > > 
> > > > > 	if (strncmp(desc, tbs_prefix, sizeof(tbs_prefix) - 1) == 0 ||
> > > > > 	    strncmp(desc, bin_prefix, sizeof(bin_prefix) - 1) == 0)
> > > > > 		goto found_colon;
> > > > > 
> > > > > might be better.
> > > > > 
> > > > > David
> > > > 
> > > > I think it'd be.
> > > > 
> > > > BR, Jarkko
> > > 
> > > I'm confused. Didn't you plan to send this patch series before v5.18-rc2?
> > > It's been a while since I started working on this.
> > 
> > That was my original plan but due to some other things, I've sent
> > a PR for rc4. I CC'd you to the PR.
> 
> OK, I missed it. My micro-optimization isn't worth it, strncmp is much
> simple indeed.

Yeah, anyway, it's fine to submit this as a separate cleanup.

It's anyway working and tested code.

BR, Jarkko
diff mbox series

Patch

diff --git a/certs/blacklist.c b/certs/blacklist.c
index c9a435b15af4..97a35cf9a62c 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -19,6 +19,16 @@ 
 #include "blacklist.h"
 #include "common.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;
 
 #ifdef CONFIG_SYSTEM_REVOCATION_LIST
@@ -32,24 +42,40 @@  extern __initconst const unsigned long revocation_certificate_list_size;
  */
 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;
 }