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.
Kevin Easton (2):
af_key: Use DIV_ROUND_UP() instead of open-coded equivalent
af_key: Always verify length of provided sadb_key
net/key/af_key.c | 58 ++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 42 insertions(+), 16 deletions(-)
--
2.8.1
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 | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 7e2e718..911b68d 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -795,12 +795,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;
}
}
@@ -960,7 +960,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) {
@@ -971,7 +972,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 */
@@ -1104,14 +1105,14 @@ 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)))
+ (DIV_ROUND_UP(key->sadb_key_bits, 8) == 0 ||
+ DIV_ROUND_UP(key->sadb_key_bits, 8) > key->sadb_key_len * sizeof(uint64_t)))
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)))
+ (DIV_ROUND_UP(key->sadb_key_bits, 8) == 0 ||
+ DIV_ROUND_UP(key->sadb_key_bits, 8) > key->sadb_key_len * sizeof(uint64_t)))
return ERR_PTR(-EINVAL);
x = xfrm_state_alloc(net);
@@ -1168,7 +1169,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;
@@ -1207,7 +1208,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
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(-)
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 911b68d..f3ebb84 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;
}
@@ -1105,14 +1132,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 &&
- (DIV_ROUND_UP(key->sadb_key_bits, 8) == 0 ||
- DIV_ROUND_UP(key->sadb_key_bits, 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 &&
- (DIV_ROUND_UP(key->sadb_key_bits, 8) == 0 ||
- DIV_ROUND_UP(key->sadb_key_bits, 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
On Mon, Mar 26, 2018 at 07:39:16AM -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 <[email protected]>
Is this a fix or just a cleanup?
If it is just a cleanup, please resent it based on ipsec-next.
On Wed, Mar 28, 2018 at 07:59:25AM +0200, Steffen Klassert wrote:
> On Mon, Mar 26, 2018 at 07:39:16AM -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 <[email protected]>
>
> Is this a fix or just a cleanup?
> If it is just a cleanup, please resent it based on ipsec-next.
Hi Steffen,
This patch (1/2) is just a cleanup, but it's in preparation for patch 2/2
which is a fix and modifies some of the same lines.
- Kevin
On Wed, Mar 28, 2018 at 09:35:26PM -0400, Kevin Easton wrote:
> On Wed, Mar 28, 2018 at 07:59:25AM +0200, Steffen Klassert wrote:
> > On Mon, Mar 26, 2018 at 07:39:16AM -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 <[email protected]>
> >
> > Is this a fix or just a cleanup?
> > If it is just a cleanup, please resent it based on ipsec-next.
>
> Hi Steffen,
>
> This patch (1/2) is just a cleanup, but it's in preparation for patch 2/2
> which is a fix and modifies some of the same lines.
So please resend the fix without the cleanup, otherwise we can
get conflicts when backporting the fix to the stable trees.