Check connect address in NETLINK

Check connect address in NETLINK

[Herbert Xu]

Hi:

The recent thread on NLMSG_OK has reminded me about an old problem
with NETLINK.

The problem is that any user on the system can launch a DoS attack on
any NETLINK application by flooding its NETLINK address with packets.
This will easily fill up the receive queue of the destination
application and therefore cause legitimate packets from the kernel
or elsewhere to be dropped.

The solution seems simple.  We already have a connect(2) call for
NETLINK sockets.  So why don't we check the connected address of
the destination socket against the address of the sender before
putting the packet on the queue?

Any comments before I go ahead and code it?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Check connect address in NETLINK

[David S. Miller]

On Tue, 29 Jun 2004 09:14:39 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> The solution seems simple.  We already have a connect(2) call for
> NETLINK sockets.  So why don't we check the connected address of
> the destination socket against the address of the sender before
> putting the packet on the queue?
> 
> Any comments before I go ahead and code it?

This really won't break any existing legitimate cases?
Are you sure?

Re: Check connect address in NETLINK

[Herbert Xu]

On Mon, Jun 28, 2004 at 05:30:39PM -0700, David S. Miller wrote:
> 
> This really won't break any existing legitimate cases?
> Are you sure?

I would've thought that it shouldn't break anything.  But let me
have a look around and get back to you.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Check connect address in NETLINK

[Alexey Kuznetsov]

Hello!

> The solution seems simple.  We already have a connect(2) call for
> NETLINK sockets.  So why don't we check the connected address of
> the destination socket against the address of the sender before
> putting the packet on the queue?

Do you mean the restriction sort of made in AF_UNIX SOCK_DGRAM:
a connected socket receives messages only from its destination?

I think this is safe.

It was not done because netlink sockets were expected to listen
for broadcasts, so that this kind of protection would be not useful
and even harmful. But taking into account that inter-application
communication is not used, only kernel sends broadcasts and applications
talking to kernel will receive such broadcasts, because they are connected
to kernel.

The troube is that pid of kernel socket used to be 0, so that
applications connected to kernel are not connected in technical sense. :-)
Apparently, to implement this we have to add some kind of flag
marking connected sockets.

Alexey

Re: Check connect address in NETLINK

[Herbert Xu]

On Tue, Jun 29, 2004 at 12:22:52PM +0400, Alexey Kuznetsov wrote:
> 
> Do you mean the restriction sort of made in AF_UNIX SOCK_DGRAM:
> a connected socket receives messages only from its destination?

Exactly.  Another example would be UDP over IP.

> It was not done because netlink sockets were expected to listen
> for broadcasts, so that this kind of protection would be not useful
> and even harmful. But taking into account that inter-application
> communication is not used, only kernel sends broadcasts and applications
> talking to kernel will receive such broadcasts, because they are connected
> to kernel.

I've had a look in the various NETLINK applications that I know of,
including quagga/iproute/iptables and all the stuff that I wrote, 
none of them does a connect at all.

So it should be harmless to introduce this new semantics.

> The troube is that pid of kernel socket used to be 0, so that
> applications connected to kernel are not connected in technical sense. :-)

That's kind of a good thing since it means that existing applications
are less likely to call connect(2) :)

> Apparently, to implement this we have to add some kind of flag
> marking connected sockets.

Or we can set the disconnected pid to a negative value since POSIX
requires pid_t to be signed.  I see that you've reserved everything
between -4096 and 0.  So perhaps we can pick -1?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Check connect address in NETLINK

[Alexey Kuznetsov]

Hello!

> Or we can set the disconnected pid to a negative value since POSIX
> requires pid_t to be signed.  I see that you've reserved everything
> between -4096 and 0.  So perhaps we can pick -1?

I think we can.

Alexey

Re: Check connect address in NETLINK

[Herbert Xu]

On Tue, Jun 29, 2004 at 03:14:33PM +0400, Alexey Kuznetsov wrote:
> 
> > Or we can set the disconnected pid to a negative value since POSIX
> > requires pid_t to be signed.  I see that you've reserved everything
> > between -4096 and 0.  So perhaps we can pick -1?
> 
> I think we can.

Great.  I'll code it up then.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Check connect address in NETLINK

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

On Tue, Jun 29, 2004 at 09:18:33PM +1000, herbert wrote:
> > > Or we can set the disconnected pid to a negative value since POSIX
> > > requires pid_t to be signed.  I see that you've reserved everything
> > > between -4096 and 0.  So perhaps we can pick -1?

Actually that doesn't quite work.  Users are allowed to bind to any
non-zero address including -1.  Besides, we already have sock->sk_state
and socket->state which are perfect for this.

So here is a patch to disallow sending unicast messages to connected
sockets from addresses other than the one that it is connected to.

I've tested it with a locally patched Openswan and it works as
intended by stopping me from sending bogus messages to it and
still allowing kernel messages to go through.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 930 bytes --]

===== net/netlink/af_netlink.c 1.48 vs edited =====
--- 1.48/net/netlink/af_netlink.c	2004-06-19 04:43:31 +10:00
+++ edited/net/netlink/af_netlink.c	2004-06-30 20:53:00 +10:00
@@ -365,6 +365,7 @@
 	struct sockaddr_nl *nladdr=(struct sockaddr_nl*)addr;
 
 	if (addr->sa_family == AF_UNSPEC) {
+		sock->state	= SS_UNCONNECTED;
 		nlk->dst_pid	= 0;
 		nlk->dst_groups = 0;
 		return 0;
@@ -380,6 +381,7 @@
 		err = netlink_autobind(sock);
 
 	if (err == 0) {
+		sock->state	= SS_CONNECTED;
 		nlk->dst_pid 	= nladdr->nl_pid;
 		nlk->dst_groups = nladdr->nl_groups;
 	}
@@ -428,6 +430,12 @@
 	/* Don't bother queuing skb if kernel socket has no input function */
 	nlk = nlk_sk(sock);
 	if (nlk->pid == 0 && !nlk->data_ready) {
+		sock_put(sock);
+		return ERR_PTR(-ECONNREFUSED);
+	}
+
+	if (sock->sk_socket->state == SS_CONNECTED &&
+	    nlk->dst_pid != nlk_sk(ssk)->pid) {
 		sock_put(sock);
 		return ERR_PTR(-ECONNREFUSED);
 	}

Re: Check connect address in NETLINK

[Alexey Kuznetsov]

Hello!

> +	if (sock->sk_socket->state == SS_CONNECTED &&
> +	    nlk->dst_pid != nlk_sk(ssk)->pid) {

No-no-no! sock->sk_socket can be NULL at this point.

You can use sock->sk_state =  TCP_ESTABLISHED, forxample.

Alexey

Re: Check connect address in NETLINK

[Herbert Xu]

On Wed, Jun 30, 2004 at 04:00:45PM +0400, Alexey Kuznetsov wrote:
> 
> > +	if (sock->sk_socket->state == SS_CONNECTED &&
> > +	    nlk->dst_pid != nlk_sk(ssk)->pid) {
> 
> No-no-no! sock->sk_socket can be NULL at this point.
> 
> You can use sock->sk_state =  TCP_ESTABLISHED, forxample.

OK.  Can you give me a code path that allows sk_socket to be NULL
at this point?

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Check connect address in NETLINK

[Alexey Kuznetsov]

Hello!

> OK.  Can you give me a code path that allows sk_socket to be NULL
> at this point?

cpu 0:				cpu1 (or just preempted cpu)

sk = netlink_lookup(...);
				... closing sk
				netlink_release() clears sk_socket

use sk->sk_socket. Oops.

Alexey

Re: Check connect address in NETLINK

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

On Wed, Jun 30, 2004 at 04:14:20PM +0400, Alexey Kuznetsov wrote:
> 
> cpu 0:				cpu1 (or just preempted cpu)
> 
> sk = netlink_lookup(...);
> 				... closing sk
> 				netlink_release() clears sk_socket
> 
> use sk->sk_socket. Oops.

Thanks for the example.

Here is a version that uses sk_state instead.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 1320 bytes --]

===== include/linux/netlink.h 1.15 vs edited =====
--- 1.15/include/linux/netlink.h	2004-04-17 02:24:35 +10:00
+++ edited/include/linux/netlink.h	2004-06-30 22:34:18 +10:00
@@ -90,6 +90,11 @@
 
 #define NET_MAJOR 36		/* Major 36 is reserved for networking 						*/
 
+enum {
+	NETLINK_UNCONNECTED = 0,
+	NETLINK_CONNECTED,
+};
+
 #ifdef __KERNEL__
 
 #include <linux/capability.h>
===== net/netlink/af_netlink.c 1.48 vs edited =====
--- 1.48/net/netlink/af_netlink.c	2004-06-19 04:43:31 +10:00
+++ edited/net/netlink/af_netlink.c	2004-06-30 22:34:24 +10:00
@@ -365,6 +365,7 @@
 	struct sockaddr_nl *nladdr=(struct sockaddr_nl*)addr;
 
 	if (addr->sa_family == AF_UNSPEC) {
+		sk->sk_state	= NETLINK_UNCONNECTED;
 		nlk->dst_pid	= 0;
 		nlk->dst_groups = 0;
 		return 0;
@@ -380,6 +381,7 @@
 		err = netlink_autobind(sock);
 
 	if (err == 0) {
+		sk->sk_state	= NETLINK_CONNECTED;
 		nlk->dst_pid 	= nladdr->nl_pid;
 		nlk->dst_groups = nladdr->nl_groups;
 	}
@@ -428,6 +430,12 @@
 	/* Don't bother queuing skb if kernel socket has no input function */
 	nlk = nlk_sk(sock);
 	if (nlk->pid == 0 && !nlk->data_ready) {
+		sock_put(sock);
+		return ERR_PTR(-ECONNREFUSED);
+	}
+
+	if (sock->sk_state == NETLINK_CONNECTED &&
+	    nlk->dst_pid != nlk_sk(ssk)->pid) {
 		sock_put(sock);
 		return ERR_PTR(-ECONNREFUSED);
 	}

Re: Check connect address in NETLINK

[David S. Miller]

Why don't you combine the two "ERR_PTR(-ECONNREFUSED)" tests
into one test like:

	if ((nlk->pid == 0 && !nlk->data_ready) ||
	    (sock->sk_state == NELTINK_CONNECTED &&
	     nlk->dst_pid != nlk_sk(ssk)->pid)) {
		sock_put(sock);
		return ERR_PTR(-ECONNREFUSED);
	}

so we don't have two copies of the "sock_put(); return ERR_PTR()"
thing emitted by the compiler?

Re: Check connect address in NETLINK

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 832 bytes --]

On Wed, Jun 30, 2004 at 03:36:06PM -0700, David S. Miller wrote:
> 
> Why don't you combine the two "ERR_PTR(-ECONNREFUSED)" tests
> into one test like:
> 
> 	if ((nlk->pid == 0 && !nlk->data_ready) ||
> 	    (sock->sk_state == NELTINK_CONNECTED &&
> 	     nlk->dst_pid != nlk_sk(ssk)->pid)) {
> 		sock_put(sock);
> 		return ERR_PTR(-ECONNREFUSED);
> 	}
> 
> so we don't have two copies of the "sock_put(); return ERR_PTR()"
> thing emitted by the compiler?

Well at least under i386, gcc (3.3.4) is smart enough to merge these
common exit paths.

But yes we could merge them.  What about the following incremental
patch?
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 883 bytes --]

--- work-2.6/net/netlink/af_netlink.c.orig	2004-07-01 08:56:55.000000000 +1000
+++ work-2.6/net/netlink/af_netlink.c	2004-07-01 08:59:14.000000000 +1000
@@ -425,21 +425,23 @@
 
 	sock = netlink_lookup(protocol, pid);
 	if (!sock)
-		return ERR_PTR(-ECONNREFUSED);
+		goto err;
 
 	/* Don't bother queuing skb if kernel socket has no input function */
 	nlk = nlk_sk(sock);
-	if (nlk->pid == 0 && !nlk->data_ready) {
-		sock_put(sock);
-		return ERR_PTR(-ECONNREFUSED);
-	}
+	if (nlk->pid == 0 && !nlk->data_ready)
+		goto err_sock;
 
 	if (sock->sk_state == NETLINK_CONNECTED &&
-	    nlk->dst_pid != nlk_sk(ssk)->pid) {
-		sock_put(sock);
-		return ERR_PTR(-ECONNREFUSED);
-	}
+	    nlk->dst_pid != nlk_sk(ssk)->pid)
+		goto err_sock;
+
 	return sock;
+
+err_sock:
+	sock_put(sock);
+err:
+	return ERR_PTR(-ECONNREFUSED);
 }
 
 struct sock *netlink_getsockbyfilp(struct file *filp)

Re: Check connect address in NETLINK

[David S. Miller]

On Wed, 30 Jun 2004 22:40:50 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Wed, Jun 30, 2004 at 04:14:20PM +0400, Alexey Kuznetsov wrote:
> > 
> > cpu 0:				cpu1 (or just preempted cpu)
> > 
> > sk = netlink_lookup(...);
> > 				... closing sk
> > 				netlink_release() clears sk_socket
> > 
> > use sk->sk_socket. Oops.
> 
> Thanks for the example.
> 
> Here is a version that uses sk_state instead.

Applied, thanks Herbert.