* Re: Pull "Load keys from signed PE binaries" branch into linux-next
2013-01-03 13:05 Pull "Load keys from signed PE binaries" branch into linux-next David Howells
@ 2013-01-03 23:24 ` Stephen Rothwell
2013-01-09 21:09 ` Kees Cook
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Stephen Rothwell @ 2013-01-03 23:24 UTC (permalink / raw)
To: David Howells
Cc: pjones, jwboyer, mjg59, keescook, dmitry.kasatkin, zohar,
keyrings, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2248 bytes --]
Hi David,
On Thu, 03 Jan 2013 13:05:19 +0000 David Howells <dhowells@redhat.com> wrote:
>
> 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
Added from today. Though I note that it was all last posted at the end
of October and has been rebased onto v3.8-rc2 ... I assume some of the
cc'd people will scream is there are problems.
Thanks for adding your subsystem tree as a participant of linux-next. As
you may know, this is not a judgment of your code. The purpose of
linux-next is for integration testing and to lower the impact of
conflicts between subsystems in the next merge window.
You will need to ensure that the patches/commits in your tree/series have
been:
* submitted under GPL v2 (or later) and include the Contributor's
Signed-off-by,
* posted to the relevant mailing list,
* reviewed by you (or another maintainer of your subsystem tree),
* successfully unit tested, and
* destined for the current or next Linux merge window.
Basically, this should be just what you would send to Linus (or ask him
to fetch). It is allowed to be rebased if you deem it necessary.
--
Cheers,
Stephen Rothwell
sfr@canb.auug.org.au
Legal Stuff:
By participating in linux-next, your subsystem tree contributions are
public and will be included in the linux-next trees. You may be sent
e-mail messages indicating errors or other issues when the
patches/commits from your subsystem tree are merged and tested in
linux-next. These messages may also be cross-posted to the linux-next
mailing list, the linux-kernel mailing list, etc. The linux-next tree
project and IBM (my employer) make no warranties regarding the linux-next
project, the testing procedures, the results, the e-mails, etc. If you
don't agree to these ground rules, let me know and I'll remove your tree
from participation in linux-next.
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Pull "Load keys from signed PE binaries" branch into linux-next
2013-01-03 13:05 Pull "Load keys from signed PE binaries" branch into linux-next David Howells
2013-01-03 23:24 ` Stephen Rothwell
@ 2013-01-09 21:09 ` Kees Cook
2013-01-10 16:12 ` David Howells
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2013-01-09 21:09 UTC (permalink / raw)
To: David Howells; +Cc: LKML
Hi,
This is a quick review of the devel-pekeys tree...
On Thu, Jan 3, 2013 at 5:05 AM, David Howells <dhowells@redhat.com> 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@chromium.org>
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Pull "Load keys from signed PE binaries" branch into linux-next
2013-01-03 13:05 Pull "Load keys from signed PE binaries" branch into linux-next David Howells
2013-01-03 23:24 ` Stephen Rothwell
2013-01-09 21:09 ` Kees Cook
@ 2013-01-10 16:12 ` David Howells
2013-01-10 18:16 ` David Howells
2013-01-10 20:05 ` Josh Boyer
4 siblings, 0 replies; 6+ messages in thread
From: David Howells @ 2013-01-10 16:12 UTC (permalink / raw)
To: Kees Cook; +Cc: dhowells, LKML
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Pull "Load keys from signed PE binaries" branch into linux-next
2013-01-03 13:05 Pull "Load keys from signed PE binaries" branch into linux-next David Howells
` (2 preceding siblings ...)
2013-01-10 16:12 ` David Howells
@ 2013-01-10 18:16 ` David Howells
2013-01-10 20:05 ` Josh Boyer
4 siblings, 0 replies; 6+ messages in thread
From: David Howells @ 2013-01-10 18:16 UTC (permalink / raw)
To: Kees Cook; +Cc: dhowells, LKML
> > 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])
There should be a '!' here.
> return -ENOPKG;
David
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Pull "Load keys from signed PE binaries" branch into linux-next
2013-01-03 13:05 Pull "Load keys from signed PE binaries" branch into linux-next David Howells
` (3 preceding siblings ...)
2013-01-10 18:16 ` David Howells
@ 2013-01-10 20:05 ` Josh Boyer
4 siblings, 0 replies; 6+ messages in thread
From: Josh Boyer @ 2013-01-10 20:05 UTC (permalink / raw)
To: David Howells
Cc: Stephen Rothwell, pjones, mjg59, keescook, dmitry.kasatkin,
zohar, keyrings, linux-kernel
On Thu, Jan 03, 2013 at 01:05:19PM +0000, David Howells wrote:
> 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
> 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
> X.509: Handle certificates that lack an authorityKeyIdentifier field
> X.509: Export certificate parse and free functions
The above patches are mostly cleanups and I can follow them fairly well.
The only real comment I had was the same one Kees already made about the
public_key_verify_signature_2 function.
Feel free to add a:
Reviewed-by: Josh Boyer <jwboyer@redhat.com>
josh
^ permalink raw reply [flat|nested] 6+ messages in thread