[PATCH 0/5] KEYS: Miscellaneous fixes

[PATCH 0/5] KEYS: Miscellaneous fixes

[David Howells]

Hi James,

Here are some fixes to go upstream:

 (1) Increase root's key quotas so that the DNS resolver doesn't run out of
     capacity.  This is a bit of a stop-gap whilst I try to improve quota
     recycling, but that's proving to be a bit tricky.

 (2) Fix the definition of the public_key subtype to give the name length.

 (3) Set a prefix for printing from signature handling.

 (4) Fix a use after free in assoc_array_gc().

 (5) Relax a check in the PKCS#7 parser to allow for padding inserted after a
     PKCS#7 message.

They can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-fixes

Tagged with:

	keys-fixes-20140902

David
---
David Howells (4):
      KEYS: Fix public_key asymmetric key subtype name
      KEYS: Set pr_fmt() in asymmetric key signature handling
      KEYS: Fix use-after-free in assoc_array_gc()
      PEFILE: Relax the check on the length of the PKCS#7 cert

Steve Dickson (1):
      KEYS: Increase root_maxkeys and root_maxbytes sizes


 crypto/asymmetric_keys/public_key.c    |    1 +
 crypto/asymmetric_keys/signature.c     |    1 +
 crypto/asymmetric_keys/verify_pefile.c |   49 ++++++++++++++++++++++----------
 lib/assoc_array.c                      |    2 +
 security/keys/key.c                    |    4 +--
 5 files changed, 38 insertions(+), 19 deletions(-)

[PATCH 1/5] KEYS: Increase root_maxkeys and root_maxbytes sizes

[David Howells]

From: Steve Dickson <stevedredhatcom>

Now that NFS client uses the kernel key ring facility to store the NFSv4
id/gid mappings, the defaults for root_maxkeys and root_maxbytes need to be
substantially increased.

These values have been soak tested:

	https://bugzilla.redhat.com/show_bug.cgi?id=1033708#c73

Signed-off-by: Steve Dickson <steved@redhat.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/key.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/keys/key.c b/security/keys/key.c
index b90a68c4e2c4..6d0cad16f002 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -27,8 +27,8 @@ DEFINE_SPINLOCK(key_serial_lock);
 struct rb_root	key_user_tree; /* tree of quota records indexed by UID */
 DEFINE_SPINLOCK(key_user_lock);
 
-unsigned int key_quota_root_maxkeys = 200;	/* root's key count quota */
-unsigned int key_quota_root_maxbytes = 20000;	/* root's key space quota */
+unsigned int key_quota_root_maxkeys = 1000000;	/* root's key count quota */
+unsigned int key_quota_root_maxbytes = 25000000; /* root's key space quota */
 unsigned int key_quota_maxkeys = 200;		/* general key count quota */
 unsigned int key_quota_maxbytes = 20000;	/* general key space quota */
 

[PATCH 2/5] KEYS: Fix public_key asymmetric key subtype name

[David Howells]

The length of the name of an asymmetric key subtype must be stored in struct
asymmetric_key_subtype::name_len so that it can be matched by a search for
"<subkey_name>:<partial_fingerprint>".  Fix the public_key subtype to have
name_len set.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 crypto/asymmetric_keys/public_key.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index 97eb001960b9..2f6e4fb1a1ea 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -121,6 +121,7 @@ static int public_key_verify_signature_2(const struct key *key,
 struct asymmetric_key_subtype public_key_subtype = {
 	.owner			= THIS_MODULE,
 	.name			= "public_key",
+	.name_len		= sizeof("public_key") - 1,
 	.describe		= public_key_describe,
 	.destroy		= public_key_destroy,
 	.verify_signature	= public_key_verify_signature_2,

[PATCH 3/5] KEYS: Set pr_fmt() in asymmetric key signature handling

[David Howells]

Printing in base signature handling should have a prefix, so set pr_fmt().

Signed-off-by: David Howells <dhowells@redhat.com>
---

 crypto/asymmetric_keys/signature.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/crypto/asymmetric_keys/signature.c b/crypto/asymmetric_keys/signature.c
index 50b3f880b4ff..7525fd183574 100644
--- a/crypto/asymmetric_keys/signature.c
+++ b/crypto/asymmetric_keys/signature.c
@@ -11,6 +11,7 @@
  * 2 of the Licence, or (at your option) any later version.
  */
 
+#define pr_fmt(fmt) "SIG: "fmt
 #include <keys/asymmetric-subtype.h>
 #include <linux/module.h>
 #include <linux/err.h>

[PATCH 4/5] KEYS: Fix use-after-free in assoc_array_gc()

[David Howells]

An edit script should be considered inaccessible by a function once it has
called assoc_array_apply_edit() or assoc_array_cancel_edit().

However, assoc_array_gc() is accessing the edit script just after the
gc_complete: label.

Reported-by: Andreea-Cristina Bernat <bernat.ada@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Andreea-Cristina Bernat <bernat.ada@gmail.com>
cc: shemming@brocade.com
cc: paulmck@linux.vnet.ibm.com
---

 lib/assoc_array.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/assoc_array.c b/lib/assoc_array.c
index c0b1007011e1..ae146f0734eb 100644
--- a/lib/assoc_array.c
+++ b/lib/assoc_array.c
@@ -1735,7 +1735,7 @@ ascend_old_tree:
 gc_complete:
 	edit->set[0].to = new_root;
 	assoc_array_apply_edit(edit);
-	edit->array->nr_leaves_on_tree = nr_leaves_on_tree;
+	array->nr_leaves_on_tree = nr_leaves_on_tree;
 	return 0;
 
 enomem:

[PATCH 5/5] PEFILE: Relax the check on the length of the PKCS#7 cert

[David Howells]

Relax the check on the length of the PKCS#7 cert as it appears that the PE
file wrapper size gets rounded up to the nearest 8.

The debugging output looks like this:

	PEFILE: ==> verify_pefile_signature()
	PEFILE: ==> pefile_parse_binary()
	PEFILE: checksum @ 110
	PEFILE: header size = 200
	PEFILE: cert = 968 @547be0 [68 09 00 00 00 02 02 00 30 82 09 56 ]
	PEFILE: sig wrapper = { 968, 200, 2 }
	PEFILE: Signature data not PKCS#7

The wrapper is the first 8 bytes of the hex dump inside [].  This indicates a
length of 0x968 bytes, including the wrapper header - so 0x960 bytes of
payload.

The ASN.1 wrapper begins [ ... 30 82 09 56 ].  That indicates an object of size
0x956 - a four byte discrepency, presumably just padding for alignment
purposes.

So we just check that the ASN.1 container is no bigger than the payload and
reduce the recorded size appropriately.

Whilst we're at it, allow shorter PKCS#7 objects that manage to squeeze within
127 or 255 bytes.  It's just about conceivable if no X.509 certs are included
in the PKCS#7 message.

Reported-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Acked-by: Peter Jones <pjones@redhat.com>
---

 crypto/asymmetric_keys/verify_pefile.c |   49 ++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/crypto/asymmetric_keys/verify_pefile.c b/crypto/asymmetric_keys/verify_pefile.c
index 79175e6ea0b2..90122831b818 100644
--- a/crypto/asymmetric_keys/verify_pefile.c
+++ b/crypto/asymmetric_keys/verify_pefile.c
@@ -128,6 +128,7 @@ static int pefile_strip_sig_wrapper(const void *pebuf,
 {
 	struct win_certificate wrapper;
 	const u8 *pkcs7;
+	unsigned len;
 
 	if (ctx->sig_len < sizeof(wrapper)) {
 		pr_debug("Signature wrapper too short\n");
@@ -154,33 +155,49 @@ static int pefile_strip_sig_wrapper(const void *pebuf,
 		return -ENOTSUPP;
 	}
 
-	/* Looks like actual pkcs signature length is in wrapper->length.
-	 * size obtained from data dir entries lists the total size of
-	 * certificate table which is also aligned to octawrod boundary.
-	 *
-	 * So set signature length field appropriately.
+	/* It looks like the pkcs signature length in wrapper->length and the
+	 * size obtained from the data dir entries, which lists the total size
+	 * of certificate table, are both aligned to an octaword boundary, so
+	 * we may have to deal with some padding.
 	 */
 	ctx->sig_len = wrapper.length;
 	ctx->sig_offset += sizeof(wrapper);
 	ctx->sig_len -= sizeof(wrapper);
-	if (ctx->sig_len == 0) {
+	if (ctx->sig_len < 4) {
 		pr_debug("Signature data missing\n");
 		return -EKEYREJECTED;
 	}
 
-	/* What's left should a PKCS#7 cert */
+	/* What's left should be a PKCS#7 cert */
 	pkcs7 = pebuf + ctx->sig_offset;
-	if (pkcs7[0] == (ASN1_CONS_BIT | ASN1_SEQ)) {
-		if (pkcs7[1] == 0x82 &&
-		    pkcs7[2] == (((ctx->sig_len - 4) >> 8) & 0xff) &&
-		    pkcs7[3] ==  ((ctx->sig_len - 4)       & 0xff))
-			return 0;
-		if (pkcs7[1] == 0x80)
-			return 0;
-		if (pkcs7[1] > 0x82)
-			return -EMSGSIZE;
+	if (pkcs7[0] != (ASN1_CONS_BIT | ASN1_SEQ))
+		goto not_pkcs7;
+	
+	switch (pkcs7[1]) {
+	case 0 ... 0x7f:
+		len = pkcs7[1] + 2;
+		goto check_len;
+	case ASN1_INDEFINITE_LENGTH:
+		return 0;
+	case 0x81:
+		len = pkcs7[2] + 3;
+		goto check_len;
+	case 0x82:
+		len = ((pkcs7[2] << 8) | pkcs7[3]) + 4;
+		goto check_len;
+	case 0x83 ... 0xff:
+		return -EMSGSIZE;
+	default:
+		goto not_pkcs7;
 	}
 
+check_len:
+	if (len <= ctx->sig_len) {
+		/* There may be padding */
+		ctx->sig_len = len;
+		return 0;
+	}
+not_pkcs7:
 	pr_debug("Signature data not PKCS#7\n");
 	return -ELIBBAD;
 }

Re: [PATCH 0/5] KEYS: Miscellaneous fixes

[James Morris]

On Tue, 2 Sep 2014, David Howells wrote:

> 
> Hi James,
> 
> Here are some fixes to go upstream:


Which of these need to go into current Linus?

#4 looks like it should.

> 
>  (1) Increase root's key quotas so that the DNS resolver doesn't run out of
>      capacity.  This is a bit of a stop-gap whilst I try to improve quota
>      recycling, but that's proving to be a bit tricky.
> 
>  (2) Fix the definition of the public_key subtype to give the name length.
> 
>  (3) Set a prefix for printing from signature handling.
> 
>  (4) Fix a use after free in assoc_array_gc().
> 
>  (5) Relax a check in the PKCS#7 parser to allow for padding inserted after a
>      PKCS#7 message.
> 
> They can be found here also:
> 
> 	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-fixes
> 
> Tagged with:
> 
> 	keys-fixes-20140902
> 
> David
> ---
> David Howells (4):
>       KEYS: Fix public_key asymmetric key subtype name
>       KEYS: Set pr_fmt() in asymmetric key signature handling
>       KEYS: Fix use-after-free in assoc_array_gc()
>       PEFILE: Relax the check on the length of the PKCS#7 cert
> 
> Steve Dickson (1):
>       KEYS: Increase root_maxkeys and root_maxbytes sizes
> 
> 
>  crypto/asymmetric_keys/public_key.c    |    1 +
>  crypto/asymmetric_keys/signature.c     |    1 +
>  crypto/asymmetric_keys/verify_pefile.c |   49 ++++++++++++++++++++++----------
>  lib/assoc_array.c                      |    2 +
>  security/keys/key.c                    |    4 +--
>  5 files changed, 38 insertions(+), 19 deletions(-)
> 

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 0/5] KEYS: Miscellaneous fixes

[David Howells]

James Morris <jmorris@namei.org> wrote:

> Which of these need to go into current Linus?
> 
> #4 looks like it should.

All of them, preferably, though I'm okay with #3 being deferred.

David

Re: [PATCH 0/5] KEYS: Miscellaneous fixes

[James Morris]

On Tue, 2 Sep 2014, David Howells wrote:

> James Morris <jmorris@namei.org> wrote:
> 
> > Which of these need to go into current Linus?
> > 
> > #4 looks like it should.
> 
> All of them, preferably, though I'm okay with #3 being deferred.

Ok, I applied #3 to next.


-- 
James Morris
<jmorris@namei.org>