[PATCH v2 0/2] af_key: Fix for sadb_key memcpy read overrun

[PATCH v2 0/2] af_key: Fix for sadb_key memcpy read overrun

[Kevin Easton]

As found by syzbot, af_key does not properly validate the key length in
sadb_key messages from userspace.  This can result in copying from beyond
the end of the sadb_key part of the message, or indeed beyond the end of
the entire packet.

Both these patches apply cleanly to ipsec-next.  Based on Steffen's
feedback I have re-ordered them so that the fix only is in patch 1, which
I would suggest is also a stable tree candidate, whereas patch 2 is a
cleanup only.

Kevin Easton (2):
  af_key: Always verify length of provided sadb_key
  af_key: Use DIV_ROUND_UP() instead of open-coded equivalent

 net/key/af_key.c | 58 ++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 16 deletions(-)

-- 
2.8.1

[PATCH v2 1/2] af_key: Always verify length of provided sadb_key

[Kevin Easton]

Key extensions (struct sadb_key) include a user-specified number of key
bits.  The kernel uses that number to determine how much key data to copy
out of the message in pfkey_msg2xfrm_state().

The length of the sadb_key message must be verified to be long enough,
even in the case of SADB_X_AALG_NULL.  Furthermore, the sadb_key_len value
must be long enough to include both the key data and the struct sadb_key
itself.

Introduce a helper function verify_key_len(), and call it from
parse_exthdrs() where other exthdr types are similarly checked for
correctness.

Signed-off-by: Kevin Easton <kevin@guarana.org>
Reported-by: syzbot+5022a34ca5a3d49b84223653fab632dfb7b4cf37@syzkaller.appspotmail.com
---
 net/key/af_key.c | 45 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 7e2e718..e62e52e 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -437,6 +437,24 @@ static int verify_address_len(const void *p)
 	return 0;
 }
 
+static inline int sadb_key_len(const struct sadb_key *key)
+{
+	int key_bytes = DIV_ROUND_UP(key->sadb_key_bits, 8);
+
+	return DIV_ROUND_UP(sizeof(struct sadb_key) + key_bytes,
+			    sizeof(uint64_t));
+}
+
+static int verify_key_len(const void *p)
+{
+	const struct sadb_key *key = p;
+
+	if (sadb_key_len(key) > key->sadb_key_len)
+		return -EINVAL;
+
+	return 0;
+}
+
 static inline int pfkey_sec_ctx_len(const struct sadb_x_sec_ctx *sec_ctx)
 {
 	return DIV_ROUND_UP(sizeof(struct sadb_x_sec_ctx) +
@@ -533,16 +551,25 @@ static int parse_exthdrs(struct sk_buff *skb, const struct sadb_msg *hdr, void *
 				return -EINVAL;
 			if (ext_hdrs[ext_type-1] != NULL)
 				return -EINVAL;
-			if (ext_type == SADB_EXT_ADDRESS_SRC ||
-			    ext_type == SADB_EXT_ADDRESS_DST ||
-			    ext_type == SADB_EXT_ADDRESS_PROXY ||
-			    ext_type == SADB_X_EXT_NAT_T_OA) {
+			switch (ext_type) {
+			case SADB_EXT_ADDRESS_SRC:
+			case SADB_EXT_ADDRESS_DST:
+			case SADB_EXT_ADDRESS_PROXY:
+			case SADB_X_EXT_NAT_T_OA:
 				if (verify_address_len(p))
 					return -EINVAL;
-			}
-			if (ext_type == SADB_X_EXT_SEC_CTX) {
+				break;
+			case SADB_X_EXT_SEC_CTX:
 				if (verify_sec_ctx_len(p))
 					return -EINVAL;
+				break;
+			case SADB_EXT_KEY_AUTH:
+			case SADB_EXT_KEY_ENCRYPT:
+				if (verify_key_len(p))
+					return -EINVAL;
+				break;
+			default:
+				break;
 			}
 			ext_hdrs[ext_type-1] = (void *) p;
 		}
@@ -1104,14 +1131,12 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 	key = ext_hdrs[SADB_EXT_KEY_AUTH - 1];
 	if (key != NULL &&
 	    sa->sadb_sa_auth != SADB_X_AALG_NULL &&
-	    ((key->sadb_key_bits+7) / 8 == 0 ||
-	     (key->sadb_key_bits+7) / 8 > key->sadb_key_len * sizeof(uint64_t)))
+	    key->sadb_key_bits == 0)
 		return ERR_PTR(-EINVAL);
 	key = ext_hdrs[SADB_EXT_KEY_ENCRYPT-1];
 	if (key != NULL &&
 	    sa->sadb_sa_encrypt != SADB_EALG_NULL &&
-	    ((key->sadb_key_bits+7) / 8 == 0 ||
-	     (key->sadb_key_bits+7) / 8 > key->sadb_key_len * sizeof(uint64_t)))
+	    key->sadb_key_bits == 0)
 		return ERR_PTR(-EINVAL);
 
 	x = xfrm_state_alloc(net);
-- 
2.8.1

Re: [PATCH v2 1/2] af_key: Always verify length of provided sadb_key

[Steffen Klassert]

On Sat, Apr 07, 2018 at 11:40:33AM -0400, Kevin Easton wrote:
> Key extensions (struct sadb_key) include a user-specified number of key
> bits.  The kernel uses that number to determine how much key data to copy
> out of the message in pfkey_msg2xfrm_state().
> 
> The length of the sadb_key message must be verified to be long enough,
> even in the case of SADB_X_AALG_NULL.  Furthermore, the sadb_key_len value
> must be long enough to include both the key data and the struct sadb_key
> itself.
> 
> Introduce a helper function verify_key_len(), and call it from
> parse_exthdrs() where other exthdr types are similarly checked for
> correctness.
> 
> Signed-off-by: Kevin Easton <kevin@guarana.org>
> Reported-by: syzbot+5022a34ca5a3d49b84223653fab632dfb7b4cf37@syzkaller.appspotmail.com

Applied to the ipsec tree, thanks Kevin!

[PATCH v2 2/2] af_key: Use DIV_ROUND_UP() instead of open-coded equivalent

[Kevin Easton]

Several places use (x + 7) / 8 to convert from a number of bits to a number
of bytes.  Replace those with DIV_ROUND_UP(x, 8) instead, for consistency
with other parts of the same file.

Signed-off-by: Kevin Easton <kevin@guarana.org>
---
 net/key/af_key.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index e62e52e..f3ebb84 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -822,12 +822,12 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const struct xfrm_state *x,
 	if (add_keys) {
 		if (x->aalg && x->aalg->alg_key_len) {
 			auth_key_size =
-				PFKEY_ALIGN8((x->aalg->alg_key_len + 7) / 8);
+				PFKEY_ALIGN8(DIV_ROUND_UP(x->aalg->alg_key_len, 8));
 			size += sizeof(struct sadb_key) + auth_key_size;
 		}
 		if (x->ealg && x->ealg->alg_key_len) {
 			encrypt_key_size =
-				PFKEY_ALIGN8((x->ealg->alg_key_len+7) / 8);
+				PFKEY_ALIGN8(DIV_ROUND_UP(x->ealg->alg_key_len, 8));
 			size += sizeof(struct sadb_key) + encrypt_key_size;
 		}
 	}
@@ -987,7 +987,8 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const struct xfrm_state *x,
 		key->sadb_key_exttype = SADB_EXT_KEY_AUTH;
 		key->sadb_key_bits = x->aalg->alg_key_len;
 		key->sadb_key_reserved = 0;
-		memcpy(key + 1, x->aalg->alg_key, (x->aalg->alg_key_len+7)/8);
+		memcpy(key + 1, x->aalg->alg_key,
+			DIV_ROUND_UP(x->aalg->alg_key_len, 8));
 	}
 	/* encrypt key */
 	if (add_keys && encrypt_key_size) {
@@ -998,7 +999,7 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const struct xfrm_state *x,
 		key->sadb_key_bits = x->ealg->alg_key_len;
 		key->sadb_key_reserved = 0;
 		memcpy(key + 1, x->ealg->alg_key,
-		       (x->ealg->alg_key_len+7)/8);
+			DIV_ROUND_UP(x->ealg->alg_key_len, 8));
 	}
 
 	/* sa */
@@ -1193,7 +1194,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 			goto out;
 		}
 		if (key)
-			keysize = (key->sadb_key_bits + 7) / 8;
+			keysize = DIV_ROUND_UP(key->sadb_key_bits, 8);
 		x->aalg = kmalloc(sizeof(*x->aalg) + keysize, GFP_KERNEL);
 		if (!x->aalg) {
 			err = -ENOMEM;
@@ -1232,7 +1233,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 			}
 			key = (struct sadb_key*) ext_hdrs[SADB_EXT_KEY_ENCRYPT-1];
 			if (key)
-				keysize = (key->sadb_key_bits + 7) / 8;
+				keysize = DIV_ROUND_UP(key->sadb_key_bits, 8);
 			x->ealg = kmalloc(sizeof(*x->ealg) + keysize, GFP_KERNEL);
 			if (!x->ealg) {
 				err = -ENOMEM;
-- 
2.8.1

Re: [PATCH v2 2/2] af_key: Use DIV_ROUND_UP() instead of open-coded equivalent

[Steffen Klassert]

On Sat, Apr 07, 2018 at 11:40:47AM -0400, Kevin Easton wrote:
> Several places use (x + 7) / 8 to convert from a number of bits to a number
> of bytes.  Replace those with DIV_ROUND_UP(x, 8) instead, for consistency
> with other parts of the same file.
> 
> Signed-off-by: Kevin Easton <kevin@guarana.org>

Please resubmit this one to ipsec-next after the
merge window. Thanks!

Re: [PATCH v2 2/2] af_key: Use DIV_ROUND_UP() instead of open-coded equivalent

[Kevin Easton]

On Mon, Apr 09, 2018 at 12:34:42PM +0200, Steffen Klassert wrote:
> On Sat, Apr 07, 2018 at 11:40:47AM -0400, Kevin Easton wrote:
> > Several places use (x + 7) / 8 to convert from a number of bits to a number
> > of bytes.  Replace those with DIV_ROUND_UP(x, 8) instead, for consistency
> > with other parts of the same file.
> > 
> > Signed-off-by: Kevin Easton <kevin@guarana.org>
> 
> Please resubmit this one to ipsec-next after the
> merge window. Thanks!

Will do!

    - Kevin

Re: [PATCH v2 0/2] af_key: Fix for sadb_key memcpy read overrun

[Steffen Klassert]

On Sat, Apr 07, 2018 at 11:40:18AM -0400, Kevin Easton wrote:
> As found by syzbot, af_key does not properly validate the key length in
> sadb_key messages from userspace.  This can result in copying from beyond
> the end of the sadb_key part of the message, or indeed beyond the end of
> the entire packet.
> 
> Both these patches apply cleanly to ipsec-next.  Based on Steffen's
> feedback I have re-ordered them so that the fix only is in patch 1, which
> I would suggest is also a stable tree candidate, whereas patch 2 is a
> cleanup only.

I think here is some explanation needed. Usually bugfixes and cleanups
don't go to the same tree. On IPsec bugfixes go to the'ipsec' tree
while cleanups and new features go to the 'ipsec-next' tree. So
you need to split up your patchsets into patches that are targeted
to 'ipsec' and 'ipsec-next'. Aside from that, we are in 'merge window'
currently. This means that most maintainers don't accept patches to
their -next trees. If you have patches for a -next tree, wait until
the merge window is over (when v4.17-rc1 is released) and send them
then.