Re: Pull "Load keys from signed PE binaries" branch into linux-next

[David Howells <dhowells@redhat.com>]

Kees Cook <keescook@chromium.org> wrote:

> This is a quick review of the devel-pekeys tree...

Thanks!

> +static int public_key_verify_signature_2(const struct key *key,
> 
> Maybe name this "key_verify_signature" instead of using the trailing _2?

I would prefer that it begin with "public_key_" as that reflects the what it
deals with and makes it easier for me to find.

> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -24,72 +24,83 @@
> [...]
> -       tfm = crypto_alloc_shash(pkey_hash_algo_name[cert->sig_hash_algo],
> 0, 0);
> +       tfm = crypto_alloc_shash(pkey_hash_algo_name[cert->sig.pkey_hash_algo],
> 0, 0);
> 
> I think, even if it wasn't done before, it's worth bounds-checking the
> array access here too.

Probably not necessary, but I should check that we have the algorithms if the
number is in range.  How about:

	--- a/crypto/asymmetric_keys/x509_public_key.c
	+++ b/crypto/asymmetric_keys/x509_public_key.c
	@@ -176,6 +176,16 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
			goto error_free_cert;
		}

	+	if (cert->pub->pkey_algo > PKEY_ALGO__LAST ||
	+	    cert->sig.pkey_algo > PKEY_ALGO__LAST ||
	+	    cert->sig.pkey_hash_algo > PKEY_HASH__LAST ||
	+	    !pkey_algo[cert->pub->pkey_algo] ||
	+	    !pkey_algo[cert->sig.pkey_algo] ||
	+	    !pkey_hash_algo_name[cert->sig.pkey_hash_algo]) {
	+		ret = -ENOPKG;
	+		goto error_free_cert;
	+	}
	+
		cert->pub->algo = pkey_algo[cert->pub->pkey_algo];
		cert->pub->id_type = PKEY_ID_X509;
	 

> +       tfm = crypto_alloc_shash(pkey_hash_algo_name[pkcs7->sig.pkey_hash_algo],
> +                                0, 0);
> 
> More of my paranoia for array access here. :)

I've added this at the top of pkc7_digest():

	if (pkcs7->sig.pkey_hash_algo > PKEY_HASH__LAST ||
	    pkey_hash_algo_name[pkcs7->sig.pkey_hash_algo])
		return -ENOPKG;

> --- /dev/null
> +++ b/crypto/asymmetric_keys/pkcs7_trust.c
> @@ -0,0 +1,145 @@
> [...]
> +       id[signer_len + 0] = ':';
> +       id[signer_len + 1] = ' ';
> 
> the key matching routing seems to not expect this trailing space
> character? Also, is there some risk here that a requested signer
> string could include a ":" character to confuse things?

This bit of asymmetric_key_match() takes care of that:

	/* See if the full key description matches as is */
	if (key->description && strcmp(key->description, description) == 0)
		return 1;

David