Messages in this thread Patch in this message | | | From | David Howells <> | Subject | Re: Pull "Load keys from signed PE binaries" branch into linux-next | Date | Thu, 10 Jan 2013 16:12:15 +0000 |
| |
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
| |