[PATCH] [RFC] xt_owner: enable xt_owner on INPUT chain

[PATCH] [RFC] xt_owner: enable xt_owner on INPUT chain

[valentina.giusti]

From: Valentina Giusti <valentina.giusti@bmw-carit.de>

I'm working on getting the owner extension also on the INPUT chain.

While it works already for TCP with the following patch (and thanks to commit
41063e9 ipv4: Early TCP socket demux), I'll also need to enable the match for
other kinds of sockets (e.g. UDP) on INPUT, which IMO should be achieved by
implementing the early demultiplexing for other kinds of connections.

In the meanwhile, could anybody please give feedback and tell me if this is the
right direction?

[PATCH] xt_owner: enable xt_owner on INPUT chain

[valentina.giusti]

From: Valentina Giusti <valentina.giusti@bmw-carit.de>

Since (41063e9 ipv4: Early TCP socket demux), we can apply the owner
extension on the INPUT chain and match established TCP sockets.
However, because of the same commit, we can have skb->sk pointing to a
timewait socket, in which case accessing skb->sk->sk_socket is invalid.

Signed-off-by: Valentina Giusti <valentina.giusti@bmw-carit.de>

Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Cc: "David S. Miller" <davem@davemloft.net>
---
 net/netfilter/xt_owner.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
index ca2e577..df03cac 100644
--- a/net/netfilter/xt_owner.c
+++ b/net/netfilter/xt_owner.c
@@ -16,6 +16,7 @@
 #include <net/sock.h>
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_owner.h>
+#include <net/tcp_states.h>
 
 static int owner_check(const struct xt_mtchk_param *par)
 {
@@ -34,7 +35,8 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	const struct xt_owner_match_info *info = par->matchinfo;
 	const struct file *filp;
 
-	if (skb->sk == NULL || skb->sk->sk_socket == NULL)
+	if (skb->sk == NULL || skb->sk->sk_state == TCP_TIME_WAIT ||
+	    skb->sk->sk_socket == NULL)
 		return (info->match ^ info->invert) == 0;
 	else if (info->match & info->invert & XT_OWNER_SOCKET)
 		/*
@@ -76,7 +78,8 @@ static struct xt_match owner_mt_reg __read_mostly = {
 	.checkentry = owner_check,
 	.match      = owner_mt,
 	.matchsize  = sizeof(struct xt_owner_match_info),
-	.hooks      = (1 << NF_INET_LOCAL_OUT) |
+	.hooks      = (1 << NF_INET_LOCAL_IN) |
+		      (1 << NF_INET_LOCAL_OUT) |
 	              (1 << NF_INET_POST_ROUTING),
 	.me         = THIS_MODULE,
 };
-- 
1.7.10.4

Re: [PATCH] [RFC] xt_owner: enable xt_owner on INPUT chain

[Phil Oester]

On Fri, Aug 30, 2013 at 02:43:42PM +0200, valentina.giusti@bmw-carit.de wrote:
> I'm working on getting the owner extension also on the INPUT chain.
> 
> In the meanwhile, could anybody please give feedback and tell me if this is the
> right direction?

What about the (common) case of no local socket?  I think that's why the owner
match was restricted to output|postrouting in the first place, no?

Phil

Re: [PATCH] [RFC] xt_owner: enable xt_owner on INPUT chain

[Florian Westphal]

Phil Oester <kernel@linuxace.com> wrote:
> On Fri, Aug 30, 2013 at 02:43:42PM +0200, valentina.giusti@bmw-carit.de wrote:
> > I'm working on getting the owner extension also on the INPUT chain.
> > 
> > In the meanwhile, could anybody please give feedback and tell me if this is the
> > right direction?
> 
> What about the (common) case of no local socket?  I think that's why the owner
> match was restricted to output|postrouting in the first place, no?

No, it was restricted because skb->sk is only set for locally generated
outgoing packets.  As Valentina explained, with tcp early demux skb->sk
will already be set for incoming tcp packets when the packet traverses
the INPUT chain.

Re: [PATCH] xt_owner: enable xt_owner on INPUT chain

[Pablo Neira Ayuso]

On Fri, Aug 30, 2013 at 02:43:43PM +0200, valentina.giusti@bmw-carit.de wrote:
> From: Valentina Giusti <valentina.giusti@bmw-carit.de>
> 
> Since (41063e9 ipv4: Early TCP socket demux), we can apply the owner
> extension on the INPUT chain and match established TCP sockets.
> However, because of the same commit, we can have skb->sk pointing to a
> timewait socket, in which case accessing skb->sk->sk_socket is invalid.

This only works for established TCP sockets. Thus, this rule:

-A INPUT -m owner --socket-exists -j ACCEPT
-A OUTPUT -m owner --socket-exists -j ACCEPT

are semantically different depending on the path.

Re: [PATCH] xt_owner: enable xt_owner on INPUT chain

[Valentina Giusti]

Hi Pablo,

thanks for having a look at this patch.

On 09/04/2013 02:34 PM, Pablo Neira Ayuso wrote:
> On Fri, Aug 30, 2013 at 02:43:43PM +0200, valentina.giusti@bmw-carit.de wrote:
>> From: Valentina Giusti <valentina.giusti@bmw-carit.de>
>>
>> Since (41063e9 ipv4: Early TCP socket demux), we can apply the owner
>> extension on the INPUT chain and match established TCP sockets.
>> However, because of the same commit, we can have skb->sk pointing to a
>> timewait socket, in which case accessing skb->sk->sk_socket is invalid.
> This only works for established TCP sockets. Thus, this rule:
>
> -A INPUT -m owner --socket-exists -j ACCEPT
> -A OUTPUT -m owner --socket-exists -j ACCEPT
>
> are semantically different depending on the path.

True, in fact my idea is to enable early demultiplexing also for other 
kinds of sockets - as mentioned in the cover letter to this patch: 
http://marc.info/?l=netfilter-devel&m=137786715327396&w=2.

Sorry, I should probably have made it clear that also this patch was 
part of the [RFC], since of course I didn't mean to have it applied now.

> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html