[PATCH 0/2] net: sock_diag fixes

[PATCH 0/2] net: sock_diag fixes

[Mathias Krause]

Hi Dave,

this small series fixes an exploitable bug in sock_diag. An unprivileged
user can send us a netlink message resulting in an out-of-bounds access
that allows userland to take over control while in kernel mode.

The first patch fixes the bug and should be pushed to stable. The second
one is an attempt to cleanup the sock_diag_handlers[] access mess in
__sock_diag_rcv_msg.

Please apply!


Mathias Krause (2):
  sock_diag: Fix out-of-bounds access to sock_diag_handlers[]
  sock_diag: Simplify sock_diag_handlers[] handling in
    __sock_diag_rcv_msg

 net/core/sock_diag.c |   27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] sock_diag: Fix out-of-bounds access to sock_diag_handlers[]

[Mathias Krause]

Userland can send a netlink message requesting SOCK_DIAG_BY_FAMILY
with a family greater or equal then AF_MAX -- the array size of
sock_diag_handlers[]. The current code does not test for this
condition therefore is vulnerable to an out-of-bound access opening
doors for a privilege escalation.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 net/core/sock_diag.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
index 602cd63..750f44f 100644
--- a/net/core/sock_diag.c
+++ b/net/core/sock_diag.c
@@ -121,6 +121,9 @@ static int __sock_diag_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	if (nlmsg_len(nlh) < sizeof(*req))
 		return -EINVAL;
 
+	if (req->sdiag_family >= AF_MAX)
+		return -EINVAL;
+
 	hndl = sock_diag_lock_handler(req->sdiag_family);
 	if (hndl == NULL)
 		err = -ENOENT;
-- 
1.7.10.4

[PATCH 2/2] sock_diag: Simplify sock_diag_handlers[] handling in __sock_diag_rcv_msg

[Mathias Krause]

The sock_diag_lock_handler() and sock_diag_unlock_handler() actually
make the code less readable. Get rid of them and make the lock usage
and access to sock_diag_handlers[] clear on the first sight.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 net/core/sock_diag.c |   24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
index 750f44f..a29e90c 100644
--- a/net/core/sock_diag.c
+++ b/net/core/sock_diag.c
@@ -97,21 +97,6 @@ void sock_diag_unregister(const struct sock_diag_handler *hnld)
 }
 EXPORT_SYMBOL_GPL(sock_diag_unregister);
 
-static const inline struct sock_diag_handler *sock_diag_lock_handler(int family)
-{
-	if (sock_diag_handlers[family] == NULL)
-		request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
-				NETLINK_SOCK_DIAG, family);
-
-	mutex_lock(&sock_diag_table_mutex);
-	return sock_diag_handlers[family];
-}
-
-static inline void sock_diag_unlock_handler(const struct sock_diag_handler *h)
-{
-	mutex_unlock(&sock_diag_table_mutex);
-}
-
 static int __sock_diag_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 {
 	int err;
@@ -124,12 +109,17 @@ static int __sock_diag_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	if (req->sdiag_family >= AF_MAX)
 		return -EINVAL;
 
-	hndl = sock_diag_lock_handler(req->sdiag_family);
+	if (sock_diag_handlers[req->sdiag_family] == NULL)
+		request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
+				NETLINK_SOCK_DIAG, req->sdiag_family);
+
+	mutex_lock(&sock_diag_table_mutex);
+	hndl = sock_diag_handlers[req->sdiag_family];
 	if (hndl == NULL)
 		err = -ENOENT;
 	else
 		err = hndl->dump(skb, nlh);
-	sock_diag_unlock_handler(hndl);
+	mutex_unlock(&sock_diag_table_mutex);
 
 	return err;
 }
-- 
1.7.10.4

Re: [PATCH 1/2] sock_diag: Fix out-of-bounds access to sock_diag_handlers[]

[Eric Dumazet]

On Sat, 2013-02-23 at 12:13 +0100, Mathias Krause wrote:
> Userland can send a netlink message requesting SOCK_DIAG_BY_FAMILY
> with a family greater or equal then AF_MAX -- the array size of
> sock_diag_handlers[]. The current code does not test for this
> condition therefore is vulnerable to an out-of-bound access opening
> doors for a privilege escalation.
> 
> Signed-off-by: Mathias Krause <minipli@googlemail.com>
> ---
>  net/core/sock_diag.c |    3 +++
>  1 file changed, 3 insertions(+)

Thanks for fixing this.

It seems trinity didnt catch it !

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH 0/2] net: sock_diag fixes

[David Miller]

From: Mathias Krause <minipli@googlemail.com>
Date: Sat, 23 Feb 2013 12:13:46 +0100

> Hi Dave,
> 
> this small series fixes an exploitable bug in sock_diag. An unprivileged
> user can send us a netlink message resulting in an out-of-bounds access
> that allows userland to take over control while in kernel mode.
> 
> The first patch fixes the bug and should be pushed to stable. The second
> one is an attempt to cleanup the sock_diag_handlers[] access mess in
> __sock_diag_rcv_msg.
> 
> Please apply!

Series applied, thanks.

Re: [PATCH 1/2] sock_diag: Fix out-of-bounds access to sock_diag_handlers[]

[Mathias Krause]

On Sat, Feb 23, 2013 at 6:35 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sat, 2013-02-23 at 12:13 +0100, Mathias Krause wrote:
>> Userland can send a netlink message requesting SOCK_DIAG_BY_FAMILY
>> with a family greater or equal then AF_MAX -- the array size of
>> sock_diag_handlers[]. The current code does not test for this
>> condition therefore is vulnerable to an out-of-bound access opening
>> doors for a privilege escalation.
>>
>> Signed-off-by: Mathias Krause <minipli@googlemail.com>
>> ---
>>  net/core/sock_diag.c |    3 +++
>>  1 file changed, 3 insertions(+)
>
> Thanks for fixing this.
>
> It seems trinity didnt catch it !

For trinity to catch that one, trinity needs to generate a valid
netlink request on a PF_NETLINK socket with protocol
NETLINK_SOCK_DIAG. Very unlikely. Especially the sanity checks in
netlink_rcv_skb() will probably filter invalid messages before they
reach any interesting code. But if trinity would have support for
generating netlink messages, then, yes, it should probably have found
that bug easily. It's in there for ages, now ;)

>
> Acked-by: Eric Dumazet <edumazet@google.com>
>
>

Thanks,
Mathias