Skip to contentSkip to conversation

af_key: Fix for sadb_key memcpy read overrun

PATCH Applied Kevin Easton 2 participants 7 messages Apr 7, 2018 – Apr 10, 2018

Subsystems (3)
Networking Network drivers Networking IPsec
lore
Share & tools
lore

Kevin Easton

author
#0 lore raw

v2 cover af_key: Fix for sadb_key memcpy read overrun

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(-)
Signature · 1 line
2.8.1
author
#1 lore raw

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

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 <[email protected]>
Reported-by syzbot+5022a34ca5a3d49b84223653fab632dfb7b4cf37@syzkaller.appspotmail.com
---
 net/key/af_key.c | 45 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)
net/key/af_key.c +35 -10
index 7e2e718..e62e52e 100644
@@ -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);
Signature · 1 line
2.8.1
maintainer Applied
#2 lore raw

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

Quoted 9 lines · 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.

Quoted 6 lines

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 <[email protected]>
Reported-by syzbot+5022a34ca5a3d49b84223653fab632dfb7b4cf37@syzkaller.appspotmail.com

Applied to the ipsec tree, thanks Kevin!

author
#3 lore raw

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

In reply to 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 <[email protected]>
---
 net/key/af_key.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
net/key/af_key.c +7 -6
index e62e52e..f3ebb84 100644
@@ -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;
Signature · 1 line
2.8.1
maintainer
#4 lore raw

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

Quoted 5 lines · 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 <[email protected]>

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

author
#5 lore raw

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

Quoted 9 lines · Steffen Klassert
Quoted 5 lines · 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 <[email protected]>

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

Will do!

- Kevin

maintainer
#6 lore raw
In reply to Kevin Easton
Quoted 9 lines · 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.

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.