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

[Kees Cook]

Hi,

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

On Thu, Jan 3, 2013 at 5:05 AM, David Howells <dhowells@xxxxxxxxxx> wrote:
>
> Hi Stephen,
>
> Could you pull my branch to load module signing keys from signed PE binaries
> into linux-next please?
>
> Thanks,
> David
> ---
>
> The following changes since commit d1c3ed669a2d452cacfb48c2d171a1f364dae2ed:
>
>   Linux 3.8-rc2 (2013-01-02 18:13:21 -0800)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-modsign.git devel-pekey
>
> for you to fetch changes up to cb37a0303559a414aa74f43ae3c8c60f01555b7a:
>
>   KEYS: Add a 'trusted' flag and a 'trusted only' flag (2013-01-03 12:06:48 +0000)
>
> ----------------------------------------------------------------
> (from the branch description for devel-pekey local branch)
>
> clone of "master"
> ----------------------------------------------------------------
> David Howells (23):
>       KEYS: Rename public key parameter name arrays
>       KEYS: Move the algorithm pointer array from x509 to public_key.c
>       KEYS: Store public key algo ID in public_key struct
>       KEYS: Split public_key_verify_signature() and make available

--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -86,21 +86,43 @@ EXPORT_SYMBOL_GPL(public_key_destroy);
[...]
+       if (!algo) {
+               algo = pkey_algo[pk->pkey_algo];

pkey_algo should be bounds-checked against pkey_algo size.

+static int public_key_verify_signature_2(const struct key *key,

Maybe name this "key_verify_signature" instead of using the trailing _2?

>       KEYS: Store public key algo ID in public_key_signature struct
>       X.509: struct x509_certificate needs struct tm declaring
>       X.509: Add bits needed for PKCS#7
>       X.509: Embed public_key_signature struct and create filler function

--- 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.

>       X.509: Handle certificates that lack an authorityKeyIdentifier field
>       X.509: Export certificate parse and free functions
>       PKCS#7: Implement a parser [RFC 2315]

--- /dev/null
+++ b/crypto/asymmetric_keys/pkcs7_parser.c
@@ -0,0 +1,326 @@
[...]
+               while (pkcs7->crl) {
+                       cert = pkcs7->certs;
+                       pkcs7->certs = cert->next;
+                       x509_free_certificate(cert);
+               }

cut/paste-o? Shouldn't this while operate on pkcs7->crl instead of
pkcs7->certs? Looks like a deadlock if pkcs7->crl is !NULL.

>       PKCS#7: Digest the data in a signed-data message

--- /dev/null
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -0,0 +1,130 @@
[...]
+       tfm = crypto_alloc_shash(pkey_hash_algo_name[pkcs7->sig.pkey_hash_algo],
+                                0, 0);

More of my paranoia for array access here. :)

>       PKCS#7: Find the right key in the PKCS#7 key list and verify the signature
>       PKCS#7: Verify internal certificate chain
>       Provide PE binary definitions
>       pefile: Parse a PE binary to find a key and a signature contained therein
>       pefile: Strip the wrapper off of the cert data block
>       pefile: Parse the presumed PKCS#7 content of the certificate blob
>       pefile: Parse the "Microsoft individual code signing" data blob
>       pefile: Digest the PE binary and compare to the PKCS#7 data
>       PKCS#7: Find intersection between PKCS#7 message and known, trusted keys

--- /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?

>       PEFILE: Load the contained key if we consider the container to be validly signed
>       KEYS: Add a 'trusted' flag and a 'trusted only' flag

Otherwise, looks good. Thanks for cleaning up the pefile parser stuff
I pointed out in the earlier review! :)

Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>

-Kees

--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/