Problem with new X.509 is_hash_blacklisted() interface

Problem with new X.509 is_hash_blacklisted() interface

[James Bottomley]

Added by

commit 436529562df2748fd9918f578205b22cf8ced277
Author: David Howells <dhowells@redhat.com>
Date:   Mon Apr 3 16:07:25 2017 +0100

    X.509: Allow X.509 certs to be blacklisted
 
Ironically it duplicates a UEFI bug we've been struggling with for a
while in the pkcs11 handlers:  namely if you have a blacklist based on
certificate hashes, an interface which only takes a hash cannot
definitively tell you if the certificate is on the blacklist or not
because the hash the cert is blacklisted by may be a different
algorithm from the hash you feed in to is_hash_blacklisted().  This
means that the only safe way to use the interface is to construct every
possible hash of the cert and feed them one at a time into
is_hash_blacklisted().  This makes it an almost unusable API.

I suggest you deprecate this interface immediately and introduce an
is_cert_blacklisted() one which takes a pointer to the TBS data.  Then
the implementation can loop over the blacklists, see the hash type and
construct the hash of the TBS data for comparison (caching the hashes
for efficiency).  That way you'll be assured of a definitive answer and
an easy API.

It might be reasonable to cc linux-efi on future kernel keyring stuff,
because some of the other issues may have also come up in the UEFI
keyrings.

James

Re: Problem with new X.509 is_hash_blacklisted() interface

[Ard Biesheuvel]

On 27 May 2017 at 15:05, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> Added by
>
> commit 436529562df2748fd9918f578205b22cf8ced277
> Author: David Howells <dhowells@redhat.com>
> Date:   Mon Apr 3 16:07:25 2017 +0100
>
>     X.509: Allow X.509 certs to be blacklisted
>
> Ironically it duplicates a UEFI bug we've been struggling with for a
> while in the pkcs11 handlers:  namely if you have a blacklist based on
> certificate hashes, an interface which only takes a hash cannot
> definitively tell you if the certificate is on the blacklist or not
> because the hash the cert is blacklisted by may be a different
> algorithm from the hash you feed in to is_hash_blacklisted().  This
> means that the only safe way to use the interface is to construct every
> possible hash of the cert and feed them one at a time into
> is_hash_blacklisted().  This makes it an almost unusable API.
>
> I suggest you deprecate this interface immediately and introduce an
> is_cert_blacklisted() one which takes a pointer to the TBS data.  Then
> the implementation can loop over the blacklists, see the hash type and
> construct the hash of the TBS data for comparison (caching the hashes
> for efficiency).  That way you'll be assured of a definitive answer and
> an easy API.
>
> It might be reasonable to cc linux-efi on future kernel keyring stuff,
> because some of the other issues may have also come up in the UEFI
> keyrings.
>

Hi James,

Thanks for highlighting this. I agree that this should be addressed
asap, given that this code has not appeared in a release yet (it was
added this cycle)

Perhaps redundantly, I'd like to emphasize that this is really not a
UEFI specific issue, it applies to any application of X.509 that does
not restrict the set of permitted hash algorithms to a single one.

Regards,
Ard.

Re: Problem with new X.509 is_hash_blacklisted() interface

[David Howells]

James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> Added by
> 
> commit 436529562df2748fd9918f578205b22cf8ced277
> Author: David Howells <dhowells@redhat.com>
> Date:   Mon Apr 3 16:07:25 2017 +0100
> 
>     X.509: Allow X.509 certs to be blacklisted
>  
> Ironically it duplicates a UEFI bug we've been struggling with for a
> while in the pkcs11 handlers:  namely if you have a blacklist based on
> certificate hashes, an interface which only takes a hash cannot
> definitively tell you if the certificate is on the blacklist or not
> because the hash the cert is blacklisted by may be a different
> algorithm from the hash you feed in to is_hash_blacklisted().  This
> means that the only safe way to use the interface is to construct every
> possible hash of the cert and feed them one at a time into
> is_hash_blacklisted().  This makes it an almost unusable API.
> 
> I suggest you deprecate this interface immediately and introduce an
> is_cert_blacklisted() one which takes a pointer to the TBS data.  Then
> the implementation can loop over the blacklists, see the hash type and
> construct the hash of the TBS data for comparison (caching the hashes
> for efficiency).  That way you'll be assured of a definitive answer and
> an easy API.
> 
> It might be reasonable to cc linux-efi on future kernel keyring stuff,
> because some of the other issues may have also come up in the UEFI
> keyrings.

You should also cc keyrings and possibly l-s-m.

How about the attached patch?

David
---
KEYS: Make the blacklisting check all possible types of hash

The blacklisting facility is not limited to hashes of a specific type, so
certificate and message blocks that need to be checked may have to be
checked against hashes of more than one hash type.

Make the blacklisting facility check all the types of hash it has blacklist
entries for.

To this end:

 (1) Blacklist type key names now can take an optional type name at the
     end:

        "<type>:<hash-as-hex>[:<algo>]"

     If the algorithm is omitted, it's assumed to refer to a shaNNN
     algorithm, where NNN is defined by the number of bits in the hex hash.

 (2) mark_hash_blacklisted() maintains a list of types we've marked.  Only
     this function can modify the blacklist keyring, so this is an good
     place to do it.

     Adding to the list in the blacklist key type ops would allow userspace
     to add an unlimited number of type names to the list.

 (3) is_hash_blacklisted() is given an extra parameter for the hash
     algorithm name.  It will now check for a matching description with the
     algorithm name attached and then, if the algorithm name begins "sha",
     it will chop off the algorithm name and try that too.

 (4) is_data_blacklisted() is added to iterate through all the known
     blacklist hashes, for which it will hash the data it is given and
     invoke is_hash_blacklisted() on the digest it obtained.

     This can be told to skip a particular algorithm for when the caller
     has one precalculated.  The precalculated hash can be passed to
     is_hash_blacklisted().  This would typically be the case for a signed
     X.509 message.

Fixes: 734114f8782f ("KEYS: Add a system blacklist keyring")
Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: stable@vger.kernel.org
---
 certs/blacklist.c                        |  263 +++++++++++++++++++++++++++++--
 crypto/asymmetric_keys/x509_public_key.c |   24 ++
 include/keys/system_keyring.h            |   11 +
 3 files changed, 279 insertions(+), 19 deletions(-)

diff --git a/certs/blacklist.c b/certs/blacklist.c
index 3a507b9e2568..3be5ae5e5606 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -18,14 +18,42 @@
 #include <linux/ctype.h>
 #include <linux/err.h>
 #include <linux/seq_file.h>
+#include <crypto/hash.h>
 #include <keys/system_keyring.h>
 #include "blacklist.h"
 
+struct blacklist_hash {
+	struct blacklist_hash *next;
+	u8 name_len;
+	char name[];
+};
+
 static struct key *blacklist_keyring;
+static struct blacklist_hash blacklist_sha256 = { NULL, 6, "sha256" };
+static struct blacklist_hash *blacklist_hash_types = &blacklist_sha256;
+static DEFINE_SPINLOCK(blacklist_hash_types_lock);
+
+static const struct blacklist_hash *blacklist_hash_type(const char *hash_algo,
+							size_t name_len)
+{
+	const struct blacklist_hash *bhash;
+
+	bhash = blacklist_hash_types;
+	smp_rmb(); /* Content after pointer.  List tail is immutable */
+	for (; bhash; bhash = bhash->next)
+		if (name_len == bhash->name_len &&
+		    memcmp(hash_algo, bhash->name, name_len) == 0)
+			return bhash;
+	return NULL;
+}
 
 /*
  * The description must be a type prefix, a colon and then an even number of
- * hex digits.  The hash is kept in the description.
+ * hex digits then optionally another colon and the hash type.  If the hash
+ * type isn't specified, it's assumed to be SHAnnn where nnn is the number of
+ * bits in the hash.
+ *
+ * The hash data is kept in the description.
  */
 static int blacklist_vet_description(const char *desc)
 {
@@ -42,13 +70,18 @@ static int blacklist_vet_description(const char *desc)
 	desc++;
 	for (; *desc; desc++) {
 		if (!isxdigit(*desc))
-			return -EINVAL;
+			goto found_hash_algo;
 		n++;
 	}
 
 	if (n == 0 || n & 1)
 		return -EINVAL;
 	return 0;
+
+found_hash_algo:
+	if (*desc != ':')
+		return -EINVAL;
+	return 0;
 }
 
 /*
@@ -80,13 +113,115 @@ static struct key_type key_type_blacklist = {
 	.describe		= blacklist_describe,
 };
 
+/*
+ * Extract the type.
+ */
+static const char *blacklist_extract_type(const char *desc, size_t *_len)
+{
+	const char *h, *algo;
+	size_t len;
+
+	/* Prepare a hash record if this is a new hash.  It may be discarded
+	 * during instantiation if we find we raced with someone else.
+	 */
+	h = strchr(desc, ':');
+	if (!h)
+		return NULL;
+	h++;
+	algo = strchr(h, ':');
+	if (algo) {
+		algo++;
+		len = strlen(algo);
+		if (len <= 0 || len > 255)
+			return NULL;
+	} else {
+		/* The hash wasn't specified - assume it to be the SHA with the
+		 * same number of bits as the hash data.
+		 */
+		len = strlen(h) * 4;
+		switch (len) {
+		case 160:
+			algo = "sha1";
+			break;
+		case 224:
+			algo = "sha224";
+			break;
+		case 256:
+			algo = "sha256";
+			break;
+		case 384:
+			algo = "sha384";
+			break;
+		case 512:
+			algo = "sha512";
+			break;
+		default:
+			return NULL;
+		}
+	}
+
+	*_len = strlen(algo);
+	return algo;
+}
+
+/*
+ * Make sure the type is listed.
+ */
+static int blacklist_add_type(const char *desc)
+{
+	struct blacklist_hash *bhash;
+	const char *algo;
+	size_t len;
+
+	algo = blacklist_extract_type(desc, &len);
+	if (!algo)
+		return -EINVAL;
+
+	if (blacklist_hash_type(algo, len))
+		return 0;
+
+	bhash = kmalloc(sizeof(*bhash) + len + 1, GFP_KERNEL);
+	if (!bhash)
+		return -ENOMEM;
+	memcpy(bhash->name, algo, len);
+	bhash->name[len] = 0;
+
+	spin_lock(&blacklist_hash_types_lock);
+	if (!blacklist_hash_type(bhash->name, bhash->name_len)) {
+		bhash->next = blacklist_hash_types;
+		smp_wmb(); /* Content before pointer */
+		blacklist_hash_types = bhash;
+		bhash = NULL;
+	}
+	spin_unlock(&blacklist_hash_types_lock);
+
+	kfree(bhash);
+	return 0;
+}
+
 /**
  * mark_hash_blacklisted - Add a hash to the system blacklist
- * @hash - The hash as a hex string with a type prefix (eg. "tbs:23aa429783")
+ * @hash - The hash as a formatted string.
+ *
+ * The hash string is formatted as:
+ *
+ *	"<type>:<hash-as-hex>[:<algo>]"
+ *
+ * Where <algo> is optional and defaults to shaNNN where NNN is the number of
+ * bits in hash-as-hex, eg.:
+ *
+ *	"tbs:23aa429783:foohash"
+ *
+ * The hash must have all leading zeros present.
  */
 int mark_hash_blacklisted(const char *hash)
 {
 	key_ref_t key;
+	int ret;
+
+	ret = blacklist_add_type(hash);
+	if (ret < 0)
+		return ret;
 
 	key = key_create_or_update(make_key_ref(blacklist_keyring, true),
 				   "blacklist",
@@ -109,15 +244,18 @@ int mark_hash_blacklisted(const char *hash)
  * @hash: The hash to be checked as a binary blob
  * @hash_len: The length of the binary hash
  * @type: Type of hash
+ * @hash_algo: Hash algorithm used
  */
-int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type)
+int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type,
+			const char *hash_algo)
 {
 	key_ref_t kref;
-	size_t type_len = strlen(type);
+	size_t type_len = strlen(type), hash_algo_len = strlen(hash);
 	char *buffer, *p;
 	int ret = 0;
 
-	buffer = kmalloc(type_len + 1 + hash_len * 2 + 1, GFP_KERNEL);
+	buffer = kmalloc(type_len + 1 + hash_len * 2 + 1 + hash_algo_len + 1,
+			 GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
 	p = memcpy(buffer, type, type_len);
@@ -125,21 +263,126 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type)
 	*p++ = ':';
 	bin2hex(p, hash, hash_len);
 	p += hash_len * 2;
-	*p = 0;
+	*p++ = ':';
+	p = memcpy(p, hash_algo, hash_algo_len);
+	p[hash_algo_len] = 0;
 
 	kref = keyring_search(make_key_ref(blacklist_keyring, true),
 			      &key_type_blacklist, buffer);
-	if (!IS_ERR(kref)) {
-		key_ref_put(kref);
-		ret = -EKEYREJECTED;
+	if (!IS_ERR(kref))
+		goto black;
+
+	/* For SHA hashes, the hash type is optional. */
+	if (hash_algo[0] == 's' &&
+	    hash_algo[1] == 'h' &&
+	    hash_algo[2] == 'a') {
+		p[-1] = 0;
+
+		kref = keyring_search(make_key_ref(blacklist_keyring, true),
+				      &key_type_blacklist, buffer);
+		if (!IS_ERR(kref))
+			goto black;
 	}
 
+out:
 	kfree(buffer);
 	return ret;
+
+black:
+	key_ref_put(kref);
+	ret = -EKEYREJECTED;
+	goto out;
 }
 EXPORT_SYMBOL_GPL(is_hash_blacklisted);
 
 /*
+ * Test the blacklistedness of one combination of data and hash.
+ */
+static int blacklist_test_one(const void *data, size_t data_len,
+			      const char *type, const char *hash_algo)
+{
+	struct crypto_shash *tfm;
+	struct shash_desc *desc;
+	size_t digest_size, desc_size;
+	u8 *digest;
+	int ret;
+
+	/* Allocate the hashing algorithm we're going to need and find out how
+	 * big the hash operational data will be.  We skip any hash type for
+	 * which we don't have a crypto module available.
+	 */
+	tfm = crypto_alloc_shash(hash_algo, 0, 0);
+	if (IS_ERR(tfm))
+		return (PTR_ERR(tfm) == -ENOENT) ? 0 : PTR_ERR(tfm);
+
+	desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
+	digest_size = crypto_shash_digestsize(tfm);
+
+	ret = -ENOMEM;
+	digest = kmalloc(digest_size, GFP_KERNEL);
+	if (!digest)
+		goto error_tfm;
+
+	desc = kzalloc(desc_size, GFP_KERNEL);
+	if (!desc)
+		goto error_digest;
+
+	desc->tfm   = tfm;
+	desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+
+	/* Digest the message [RFC2315 9.3] */
+	ret = crypto_shash_init(desc);
+	if (ret < 0)
+		goto error_desc;
+	ret = crypto_shash_finup(desc, data, data_len, digest);
+	if (ret < 0)
+		goto error_desc;
+
+	ret = is_hash_blacklisted(digest, digest_size, type, hash_algo);
+error_desc:
+	kfree(desc);
+error_digest:
+	kfree(digest);
+error_tfm:
+	crypto_free_shash(tfm);
+	return ret;
+}
+
+/**
+ * is_data_blacklisted - Determine if a data blob is blacklisted
+ * @data: The data to check.
+ * @data_len: The amount of data.
+ * @type: The type of object.
+ * @skip_hash: A hash type to skip
+ *
+ * Iterate through all the types of hash for which we have blacklisted hashes
+ * and generate a hash for each and check it against the blacklist.
+ *
+ * If the caller has a precomputed hash, they can call is_hash_blacklisted() on
+ * it and then call this function with @skip_hash set to the hash type to skip.
+ */
+int is_data_blacklisted(const void *data, size_t data_len,
+			const char *type, const char *skip_hash)
+{
+	const struct blacklist_hash *bhash;
+	int ret = 0;
+
+	bhash = blacklist_hash_types;
+	smp_rmb(); /* Content after pointer.  List tail is immutable */
+
+	for (; bhash; bhash = bhash->next) {
+		if (strcmp(skip_hash, bhash->name) == 0)
+			continue;
+		ret = blacklist_test_one(data, data_len, type, bhash->name);
+		if (ret < 0)
+			return ret;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(is_data_blacklisted);
+
+/*
  * Initialise the blacklist
  */
 static int __init blacklist_init(void)
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index eea71dc9686c..57be229dd7bf 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -87,20 +87,30 @@ int x509_get_sig_params(struct x509_certificate *cert)
 	if (ret < 0)
 		goto error_2;
 
-	ret = is_hash_blacklisted(sig->digest, sig->digest_size, "tbs");
-	if (ret == -EKEYREJECTED) {
-		pr_err("Cert %*phN is blacklisted\n",
-		       sig->digest_size, sig->digest);
-		cert->blacklisted = true;
-		ret = 0;
-	}
+	ret = is_hash_blacklisted(sig->digest, sig->digest_size, "tbs",
+				  sig->hash_algo);
+	if (ret == -EKEYREJECTED)
+		goto blacklisted;
+	if (ret < 0)
+		goto error_2;
 
+	ret = is_data_blacklisted(cert->tbs, cert->tbs_size, "tbs",
+				  sig->hash_algo);
+	if (ret == -EKEYREJECTED)
+		goto blacklisted;
+	
 error_2:
 	kfree(desc);
 error:
 	crypto_free_shash(tfm);
 	pr_devel("<==%s() = %d\n", __func__, ret);
 	return ret;
+
+blacklisted:
+	pr_err("Cert %*phN is blacklisted\n", sig->digest_size, sig->digest);
+	cert->blacklisted = true;
+	ret = 0;
+	goto error_2;
 }
 
 /*
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 359c2f936004..6ab1260893eb 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -38,10 +38,17 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
 #ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
 extern int mark_hash_blacklisted(const char *hash);
 extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
-			       const char *type);
+			       const char *type, const char *hash_algo);
+extern int is_data_blacklisted(const void *data, size_t data_len,
+			       const char *type, const char *skip_hash);
 #else
 static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
-				      const char *type)
+				      const char *type, const char *hash_algo)
+{
+	return 0;
+}
+static inline int is_data_blacklisted(const void *data, size_t data_len,
+				      const char *type, const char *skip_hash)
 {
 	return 0;
 }

Re: Problem with new X.509 is_hash_blacklisted() interface

[Ard Biesheuvel]

On 20 June 2017 at 18:09, David Howells <dhowells@redhat.com> wrote:
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>
>> Added by
>>
>> commit 436529562df2748fd9918f578205b22cf8ced277
>> Author: David Howells <dhowells@redhat.com>
>> Date:   Mon Apr 3 16:07:25 2017 +0100
>>
>>     X.509: Allow X.509 certs to be blacklisted
>>
>> Ironically it duplicates a UEFI bug we've been struggling with for a
>> while in the pkcs11 handlers:  namely if you have a blacklist based on
>> certificate hashes, an interface which only takes a hash cannot
>> definitively tell you if the certificate is on the blacklist or not
>> because the hash the cert is blacklisted by may be a different
>> algorithm from the hash you feed in to is_hash_blacklisted().  This
>> means that the only safe way to use the interface is to construct every
>> possible hash of the cert and feed them one at a time into
>> is_hash_blacklisted().  This makes it an almost unusable API.
>>
>> I suggest you deprecate this interface immediately and introduce an
>> is_cert_blacklisted() one which takes a pointer to the TBS data.  Then
>> the implementation can loop over the blacklists, see the hash type and
>> construct the hash of the TBS data for comparison (caching the hashes
>> for efficiency).  That way you'll be assured of a definitive answer and
>> an easy API.
>>
>> It might be reasonable to cc linux-efi on future kernel keyring stuff,
>> because some of the other issues may have also come up in the UEFI
>> keyrings.
>
> You should also cc keyrings and possibly l-s-m.
>
> How about the attached patch?
>
> David
> ---
> KEYS: Make the blacklisting check all possible types of hash
>
> The blacklisting facility is not limited to hashes of a specific type, so
> certificate and message blocks that need to be checked may have to be
> checked against hashes of more than one hash type.
>
> Make the blacklisting facility check all the types of hash it has blacklist
> entries for.
>
> To this end:
>
>  (1) Blacklist type key names now can take an optional type name at the
>      end:
>
>         "<type>:<hash-as-hex>[:<algo>]"
>
>      If the algorithm is omitted, it's assumed to refer to a shaNNN
>      algorithm, where NNN is defined by the number of bits in the hex hash.
>
>  (2) mark_hash_blacklisted() maintains a list of types we've marked.  Only
>      this function can modify the blacklist keyring, so this is an good
>      place to do it.
>
>      Adding to the list in the blacklist key type ops would allow userspace
>      to add an unlimited number of type names to the list.
>
>  (3) is_hash_blacklisted() is given an extra parameter for the hash
>      algorithm name.  It will now check for a matching description with the
>      algorithm name attached and then, if the algorithm name begins "sha",
>      it will chop off the algorithm name and try that too.
>
>  (4) is_data_blacklisted() is added to iterate through all the known
>      blacklist hashes, for which it will hash the data it is given and
>      invoke is_hash_blacklisted() on the digest it obtained.
>

You iterate over the distinct types, not the hashes themselves, right?
You may want to clarify that here.

>      This can be told to skip a particular algorithm for when the caller
>      has one precalculated.  The precalculated hash can be passed to
>      is_hash_blacklisted().  This would typically be the case for a signed
>      X.509 message.
>

This last part seems a premature optimization to me. Is there a
performance concern preventing us from using (4) only?

In any case, the approach and the code look sound to me, although I
think adding a hash of a type that we don't know how to calculate
deserves a warning at least.

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thanks,
Ard.


> Fixes: 734114f8782f ("KEYS: Add a system blacklist keyring")
> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: stable@vger.kernel.org
> ---
>  certs/blacklist.c                        |  263 +++++++++++++++++++++++++++++--
>  crypto/asymmetric_keys/x509_public_key.c |   24 ++
>  include/keys/system_keyring.h            |   11 +
>  3 files changed, 279 insertions(+), 19 deletions(-)
>
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index 3a507b9e2568..3be5ae5e5606 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -18,14 +18,42 @@
>  #include <linux/ctype.h>
>  #include <linux/err.h>
>  #include <linux/seq_file.h>
> +#include <crypto/hash.h>
>  #include <keys/system_keyring.h>
>  #include "blacklist.h"
>
> +struct blacklist_hash {
> +       struct blacklist_hash *next;
> +       u8 name_len;
> +       char name[];
> +};
> +
>  static struct key *blacklist_keyring;
> +static struct blacklist_hash blacklist_sha256 = { NULL, 6, "sha256" };
> +static struct blacklist_hash *blacklist_hash_types = &blacklist_sha256;
> +static DEFINE_SPINLOCK(blacklist_hash_types_lock);
> +
> +static const struct blacklist_hash *blacklist_hash_type(const char *hash_algo,
> +                                                       size_t name_len)
> +{
> +       const struct blacklist_hash *bhash;
> +
> +       bhash = blacklist_hash_types;
> +       smp_rmb(); /* Content after pointer.  List tail is immutable */
> +       for (; bhash; bhash = bhash->next)
> +               if (name_len == bhash->name_len &&
> +                   memcmp(hash_algo, bhash->name, name_len) == 0)
> +                       return bhash;
> +       return NULL;
> +}
>
>  /*
>   * The description must be a type prefix, a colon and then an even number of
> - * hex digits.  The hash is kept in the description.
> + * hex digits then optionally another colon and the hash type.  If the hash
> + * type isn't specified, it's assumed to be SHAnnn where nnn is the number of
> + * bits in the hash.
> + *
> + * The hash data is kept in the description.
>   */
>  static int blacklist_vet_description(const char *desc)
>  {
> @@ -42,13 +70,18 @@ static int blacklist_vet_description(const char *desc)
>         desc++;
>         for (; *desc; desc++) {
>                 if (!isxdigit(*desc))
> -                       return -EINVAL;
> +                       goto found_hash_algo;
>                 n++;
>         }
>
>         if (n == 0 || n & 1)
>                 return -EINVAL;
>         return 0;
> +
> +found_hash_algo:
> +       if (*desc != ':')
> +               return -EINVAL;
> +       return 0;
>  }
>
>  /*
> @@ -80,13 +113,115 @@ static struct key_type key_type_blacklist = {
>         .describe               = blacklist_describe,
>  };
>
> +/*
> + * Extract the type.
> + */
> +static const char *blacklist_extract_type(const char *desc, size_t *_len)
> +{
> +       const char *h, *algo;
> +       size_t len;
> +
> +       /* Prepare a hash record if this is a new hash.  It may be discarded
> +        * during instantiation if we find we raced with someone else.
> +        */
> +       h = strchr(desc, ':');
> +       if (!h)
> +               return NULL;
> +       h++;
> +       algo = strchr(h, ':');
> +       if (algo) {
> +               algo++;
> +               len = strlen(algo);
> +               if (len <= 0 || len > 255)
> +                       return NULL;
> +       } else {
> +               /* The hash wasn't specified - assume it to be the SHA with the
> +                * same number of bits as the hash data.
> +                */
> +               len = strlen(h) * 4;
> +               switch (len) {
> +               case 160:
> +                       algo = "sha1";
> +                       break;
> +               case 224:
> +                       algo = "sha224";
> +                       break;
> +               case 256:
> +                       algo = "sha256";
> +                       break;
> +               case 384:
> +                       algo = "sha384";
> +                       break;
> +               case 512:
> +                       algo = "sha512";
> +                       break;
> +               default:
> +                       return NULL;
> +               }
> +       }
> +
> +       *_len = strlen(algo);
> +       return algo;
> +}
> +
> +/*
> + * Make sure the type is listed.
> + */
> +static int blacklist_add_type(const char *desc)
> +{
> +       struct blacklist_hash *bhash;
> +       const char *algo;
> +       size_t len;
> +
> +       algo = blacklist_extract_type(desc, &len);
> +       if (!algo)
> +               return -EINVAL;
> +
> +       if (blacklist_hash_type(algo, len))
> +               return 0;
> +
> +       bhash = kmalloc(sizeof(*bhash) + len + 1, GFP_KERNEL);
> +       if (!bhash)
> +               return -ENOMEM;
> +       memcpy(bhash->name, algo, len);
> +       bhash->name[len] = 0;
> +
> +       spin_lock(&blacklist_hash_types_lock);
> +       if (!blacklist_hash_type(bhash->name, bhash->name_len)) {
> +               bhash->next = blacklist_hash_types;
> +               smp_wmb(); /* Content before pointer */
> +               blacklist_hash_types = bhash;
> +               bhash = NULL;
> +       }
> +       spin_unlock(&blacklist_hash_types_lock);
> +
> +       kfree(bhash);
> +       return 0;
> +}
> +
>  /**
>   * mark_hash_blacklisted - Add a hash to the system blacklist
> - * @hash - The hash as a hex string with a type prefix (eg. "tbs:23aa429783")
> + * @hash - The hash as a formatted string.
> + *
> + * The hash string is formatted as:
> + *
> + *     "<type>:<hash-as-hex>[:<algo>]"
> + *
> + * Where <algo> is optional and defaults to shaNNN where NNN is the number of
> + * bits in hash-as-hex, eg.:
> + *
> + *     "tbs:23aa429783:foohash"
> + *
> + * The hash must have all leading zeros present.
>   */
>  int mark_hash_blacklisted(const char *hash)
>  {
>         key_ref_t key;
> +       int ret;
> +
> +       ret = blacklist_add_type(hash);
> +       if (ret < 0)
> +               return ret;
>
>         key = key_create_or_update(make_key_ref(blacklist_keyring, true),
>                                    "blacklist",
> @@ -109,15 +244,18 @@ int mark_hash_blacklisted(const char *hash)
>   * @hash: The hash to be checked as a binary blob
>   * @hash_len: The length of the binary hash
>   * @type: Type of hash
> + * @hash_algo: Hash algorithm used
>   */
> -int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type)
> +int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type,
> +                       const char *hash_algo)
>  {
>         key_ref_t kref;
> -       size_t type_len = strlen(type);
> +       size_t type_len = strlen(type), hash_algo_len = strlen(hash);
>         char *buffer, *p;
>         int ret = 0;
>
> -       buffer = kmalloc(type_len + 1 + hash_len * 2 + 1, GFP_KERNEL);
> +       buffer = kmalloc(type_len + 1 + hash_len * 2 + 1 + hash_algo_len + 1,
> +                        GFP_KERNEL);
>         if (!buffer)
>                 return -ENOMEM;
>         p = memcpy(buffer, type, type_len);
> @@ -125,21 +263,126 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type)
>         *p++ = ':';
>         bin2hex(p, hash, hash_len);
>         p += hash_len * 2;
> -       *p = 0;
> +       *p++ = ':';
> +       p = memcpy(p, hash_algo, hash_algo_len);
> +       p[hash_algo_len] = 0;
>
>         kref = keyring_search(make_key_ref(blacklist_keyring, true),
>                               &key_type_blacklist, buffer);
> -       if (!IS_ERR(kref)) {
> -               key_ref_put(kref);
> -               ret = -EKEYREJECTED;
> +       if (!IS_ERR(kref))
> +               goto black;
> +
> +       /* For SHA hashes, the hash type is optional. */
> +       if (hash_algo[0] == 's' &&
> +           hash_algo[1] == 'h' &&
> +           hash_algo[2] == 'a') {
> +               p[-1] = 0;
> +
> +               kref = keyring_search(make_key_ref(blacklist_keyring, true),
> +                                     &key_type_blacklist, buffer);
> +               if (!IS_ERR(kref))
> +                       goto black;
>         }
>
> +out:
>         kfree(buffer);
>         return ret;
> +
> +black:
> +       key_ref_put(kref);
> +       ret = -EKEYREJECTED;
> +       goto out;
>  }
>  EXPORT_SYMBOL_GPL(is_hash_blacklisted);
>
>  /*
> + * Test the blacklistedness of one combination of data and hash.
> + */
> +static int blacklist_test_one(const void *data, size_t data_len,
> +                             const char *type, const char *hash_algo)
> +{
> +       struct crypto_shash *tfm;
> +       struct shash_desc *desc;
> +       size_t digest_size, desc_size;
> +       u8 *digest;
> +       int ret;
> +
> +       /* Allocate the hashing algorithm we're going to need and find out how
> +        * big the hash operational data will be.  We skip any hash type for
> +        * which we don't have a crypto module available.
> +        */
> +       tfm = crypto_alloc_shash(hash_algo, 0, 0);
> +       if (IS_ERR(tfm))
> +               return (PTR_ERR(tfm) == -ENOENT) ? 0 : PTR_ERR(tfm);
> +
> +       desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> +       digest_size = crypto_shash_digestsize(tfm);
> +
> +       ret = -ENOMEM;
> +       digest = kmalloc(digest_size, GFP_KERNEL);
> +       if (!digest)
> +               goto error_tfm;
> +
> +       desc = kzalloc(desc_size, GFP_KERNEL);
> +       if (!desc)
> +               goto error_digest;
> +
> +       desc->tfm   = tfm;
> +       desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> +
> +       /* Digest the message [RFC2315 9.3] */
> +       ret = crypto_shash_init(desc);
> +       if (ret < 0)
> +               goto error_desc;
> +       ret = crypto_shash_finup(desc, data, data_len, digest);
> +       if (ret < 0)
> +               goto error_desc;
> +
> +       ret = is_hash_blacklisted(digest, digest_size, type, hash_algo);
> +error_desc:
> +       kfree(desc);
> +error_digest:
> +       kfree(digest);
> +error_tfm:
> +       crypto_free_shash(tfm);
> +       return ret;
> +}
> +
> +/**
> + * is_data_blacklisted - Determine if a data blob is blacklisted
> + * @data: The data to check.
> + * @data_len: The amount of data.
> + * @type: The type of object.
> + * @skip_hash: A hash type to skip
> + *
> + * Iterate through all the types of hash for which we have blacklisted hashes
> + * and generate a hash for each and check it against the blacklist.
> + *
> + * If the caller has a precomputed hash, they can call is_hash_blacklisted() on
> + * it and then call this function with @skip_hash set to the hash type to skip.
> + */
> +int is_data_blacklisted(const void *data, size_t data_len,
> +                       const char *type, const char *skip_hash)
> +{
> +       const struct blacklist_hash *bhash;
> +       int ret = 0;
> +
> +       bhash = blacklist_hash_types;
> +       smp_rmb(); /* Content after pointer.  List tail is immutable */
> +
> +       for (; bhash; bhash = bhash->next) {
> +               if (strcmp(skip_hash, bhash->name) == 0)
> +                       continue;
> +               ret = blacklist_test_one(data, data_len, type, bhash->name);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(is_data_blacklisted);
> +
> +/*
>   * Initialise the blacklist
>   */
>  static int __init blacklist_init(void)
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index eea71dc9686c..57be229dd7bf 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -87,20 +87,30 @@ int x509_get_sig_params(struct x509_certificate *cert)
>         if (ret < 0)
>                 goto error_2;
>
> -       ret = is_hash_blacklisted(sig->digest, sig->digest_size, "tbs");
> -       if (ret == -EKEYREJECTED) {
> -               pr_err("Cert %*phN is blacklisted\n",
> -                      sig->digest_size, sig->digest);
> -               cert->blacklisted = true;
> -               ret = 0;
> -       }
> +       ret = is_hash_blacklisted(sig->digest, sig->digest_size, "tbs",
> +                                 sig->hash_algo);
> +       if (ret == -EKEYREJECTED)
> +               goto blacklisted;
> +       if (ret < 0)
> +               goto error_2;
>
> +       ret = is_data_blacklisted(cert->tbs, cert->tbs_size, "tbs",
> +                                 sig->hash_algo);
> +       if (ret == -EKEYREJECTED)
> +               goto blacklisted;
> +
>  error_2:
>         kfree(desc);
>  error:
>         crypto_free_shash(tfm);
>         pr_devel("<==%s() = %d\n", __func__, ret);
>         return ret;
> +
> +blacklisted:
> +       pr_err("Cert %*phN is blacklisted\n", sig->digest_size, sig->digest);
> +       cert->blacklisted = true;
> +       ret = 0;
> +       goto error_2;
>  }
>
>  /*
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 359c2f936004..6ab1260893eb 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -38,10 +38,17 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
>  #ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
>  extern int mark_hash_blacklisted(const char *hash);
>  extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
> -                              const char *type);
> +                              const char *type, const char *hash_algo);
> +extern int is_data_blacklisted(const void *data, size_t data_len,
> +                              const char *type, const char *skip_hash);
>  #else
>  static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
> -                                     const char *type)
> +                                     const char *type, const char *hash_algo)
> +{
> +       return 0;
> +}
> +static inline int is_data_blacklisted(const void *data, size_t data_len,
> +                                     const char *type, const char *skip_hash)
>  {
>         return 0;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with new X.509 is_hash_blacklisted() interface

[David Howells]

Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> >      This can be told to skip a particular algorithm for when the caller
> >      has one precalculated.  The precalculated hash can be passed to
> >      is_hash_blacklisted().  This would typically be the case for a signed
> >      X.509 message.
> 
> This last part seems a premature optimization to me. Is there a
> performance concern preventing us from using (4) only?

Crypto stuff is relatively slow - and in the case of X.509 and PKCS#7 the
caller will already have calculated a hash.  The most likely situation
currently, I think, is that we will only have sha256 hashes in the blacklist,
and whatever we're checking will have a sha256 hash also.

Possibly, I could just pass the precalculated hash into is_data_blacklisted()
and so avoid having to call is_hash_blacklisted() from outside.

> In any case, the approach and the code look sound to me, although I
> think adding a hash of a type that we don't know how to calculate
> deserves a warning at least.

There are two issues with that:

 (1) We don't know what hashes are available without checking to see what
     modules are available.  However, to do this would involve loading the
     hash algorithm module - but we might not be in a position to do this yet
     (the blacklist is loaded before we start userspace).

 (2) A module implementing a hash algorithm might be blacklisted by the hash
     that we've been given to add to the blacklist.  I think this is a more
     general problem - and might require us to restrict blacklisting to hash
     algorithms that are built in.

David

Re: Problem with new X.509 is_hash_blacklisted() interface

[Ard Biesheuvel]

On 21 June 2017 at 14:49, David Howells <dhowells@redhat.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> >      This can be told to skip a particular algorithm for when the caller
>> >      has one precalculated.  The precalculated hash can be passed to
>> >      is_hash_blacklisted().  This would typically be the case for a signed
>> >      X.509 message.
>>
>> This last part seems a premature optimization to me. Is there a
>> performance concern preventing us from using (4) only?
>
> Crypto stuff is relatively slow - and in the case of X.509 and PKCS#7 the
> caller will already have calculated a hash.  The most likely situation
> currently, I think, is that we will only have sha256 hashes in the blacklist,
> and whatever we're checking will have a sha256 hash also.
>
> Possibly, I could just pass the precalculated hash into is_data_blacklisted()
> and so avoid having to call is_hash_blacklisted() from outside.
>

That would be cleaner, yes. As I said, it looks correct to me, but I
would simply prefer to keep the code as simple as possible. If there
is a good reason for the additional fast path, then I have no
objections at all.

>> In any case, the approach and the code look sound to me, although I
>> think adding a hash of a type that we don't know how to calculate
>> deserves a warning at least.
>
> There are two issues with that:
>
>  (1) We don't know what hashes are available without checking to see what
>      modules are available.  However, to do this would involve loading the
>      hash algorithm module - but we might not be in a position to do this yet
>      (the blacklist is loaded before we start userspace).
>

Ah ok. That does complicate matters, indeed.

>  (2) A module implementing a hash algorithm might be blacklisted by the hash
>      that we've been given to add to the blacklist.  I think this is a more
>      general problem - and might require us to restrict blacklisting to hash
>      algorithms that are built in.
>

That makes sense, but deserves another big fat warning when it turns
out that the blacklist contains an entry we cannot verify against.