* [PATCH 0/2] net: sock_diag fixes
@ 2013-02-23 11:13 Mathias Krause
2013-02-23 11:13 ` [PATCH 1/2] sock_diag: Fix out-of-bounds access to sock_diag_handlers[] Mathias Krause
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Mathias Krause @ 2013-02-23 11:13 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, 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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] sock_diag: Fix out-of-bounds access to sock_diag_handlers[]
2013-02-23 11:13 [PATCH 0/2] net: sock_diag fixes Mathias Krause
@ 2013-02-23 11:13 ` Mathias Krause
2013-02-23 17:35 ` Eric Dumazet
2013-02-23 11:13 ` [PATCH 2/2] sock_diag: Simplify sock_diag_handlers[] handling in __sock_diag_rcv_msg Mathias Krause
2013-02-23 18:54 ` [PATCH 0/2] net: sock_diag fixes David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Mathias Krause @ 2013-02-23 11:13 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] sock_diag: Fix out-of-bounds access to sock_diag_handlers[]
2013-02-23 11:13 ` [PATCH 1/2] sock_diag: Fix out-of-bounds access to sock_diag_handlers[] Mathias Krause
@ 2013-02-23 17:35 ` Eric Dumazet
2013-02-23 19:10 ` Mathias Krause
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2013-02-23 17:35 UTC (permalink / raw)
To: Mathias Krause; +Cc: David S. Miller, netdev, Dave Jones
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] sock_diag: Fix out-of-bounds access to sock_diag_handlers[]
2013-02-23 17:35 ` Eric Dumazet
@ 2013-02-23 19:10 ` Mathias Krause
0 siblings, 0 replies; 6+ messages in thread
From: Mathias Krause @ 2013-02-23 19:10 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, netdev, Dave Jones
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] sock_diag: Simplify sock_diag_handlers[] handling in __sock_diag_rcv_msg
2013-02-23 11:13 [PATCH 0/2] net: sock_diag fixes Mathias Krause
2013-02-23 11:13 ` [PATCH 1/2] sock_diag: Fix out-of-bounds access to sock_diag_handlers[] Mathias Krause
@ 2013-02-23 11:13 ` Mathias Krause
2013-02-23 18:54 ` [PATCH 0/2] net: sock_diag fixes David Miller
2 siblings, 0 replies; 6+ messages in thread
From: Mathias Krause @ 2013-02-23 11:13 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] net: sock_diag fixes
2013-02-23 11:13 [PATCH 0/2] net: sock_diag fixes Mathias Krause
2013-02-23 11:13 ` [PATCH 1/2] sock_diag: Fix out-of-bounds access to sock_diag_handlers[] Mathias Krause
2013-02-23 11:13 ` [PATCH 2/2] sock_diag: Simplify sock_diag_handlers[] handling in __sock_diag_rcv_msg Mathias Krause
@ 2013-02-23 18:54 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2013-02-23 18:54 UTC (permalink / raw)
To: minipli; +Cc: netdev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-23 19:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-23 11:13 [PATCH 0/2] net: sock_diag fixes Mathias Krause
2013-02-23 11:13 ` [PATCH 1/2] sock_diag: Fix out-of-bounds access to sock_diag_handlers[] Mathias Krause
2013-02-23 17:35 ` Eric Dumazet
2013-02-23 19:10 ` Mathias Krause
2013-02-23 11:13 ` [PATCH 2/2] sock_diag: Simplify sock_diag_handlers[] handling in __sock_diag_rcv_msg Mathias Krause
2013-02-23 18:54 ` [PATCH 0/2] net: sock_diag fixes David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).