diff mbox series

[v3,06/10] certs: Make blacklist_vet_description() more strict

Message ID 20210114151909.2344974-7-mic@digikod.net
State Superseded
Headers show
Series Enable root to update the blacklist keyring | expand

Commit Message

Mickaël Salaün Jan. 14, 2021, 3:19 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>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
---

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

Jarkko Sakkinen Jan. 20, 2021, 4:16 a.m. UTC | #1 
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
Mickaël Salaün Jan. 20, 2021, 11:23 a.m. UTC | #2 
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 mbox series

Patch

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;
 }