Kees Cook <[email protected]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
