[PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Pablo Neira Ayuso]

Currently, we have four xtables extensions that cannot be used from the
xt over nft compat layer. The problem is that they need real access to
the full blown xt_entry to validate that the rule comes with the right
dependencies. This check was introduced to overcome the lack of
sufficient userspace dependency validation in iptables.

To resolve this problem, this patch introduces a new field to the
xt_tgchk_param structure that tell us if the target is executed from
nft_compat context.

The four affected extensions are:

1) CLUSTERIP, this target has been superseded by xt_cluster. So just
   bail out by returning -EINVAL.

2) TCPMSS. Relax the checking when used from nft_compat and make sure
   that we skip !syn packets in case userspace provides a wrong
   configuration.

3) SYMPROXY6. Don't check for e->ipv6.flags, we can instead check
   for e->ipv6.proto as other extensions do, if zero then it doesn't
   fulfill the dependency.

4) ebt_stp. Relax the check to make sure it uses the reserved
   destination MAC address for STP. The packet path seems safe.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
ebt_among also needs some glue code in nft_compat to get the hackish
matchsize = -1 case. Arturo is working to sort out that.

 include/linux/netfilter/x_tables.h |    2 ++
 net/bridge/netfilter/ebt_stp.c     |    6 ++++--
 net/ipv4/netfilter/ipt_CLUSTERIP.c |    5 +++++
 net/ipv6/netfilter/ip6t_SYNPROXY.c |    3 +--
 net/netfilter/nft_compat.c         |    2 ++
 net/netfilter/xt_TCPMSS.c          |    9 +++++++++
 6 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index a3e215b..09f3820 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -62,6 +62,7 @@ struct xt_mtchk_param {
 	void *matchinfo;
 	unsigned int hook_mask;
 	u_int8_t family;
+	bool nft_compat;
 };
 
 /**
@@ -92,6 +93,7 @@ struct xt_tgchk_param {
 	void *targinfo;
 	unsigned int hook_mask;
 	u_int8_t family;
+	bool nft_compat;
 };
 
 /* Target destructor parameters */
diff --git a/net/bridge/netfilter/ebt_stp.c b/net/bridge/netfilter/ebt_stp.c
index 071d872..0c40570 100644
--- a/net/bridge/netfilter/ebt_stp.c
+++ b/net/bridge/netfilter/ebt_stp.c
@@ -164,8 +164,10 @@ static int ebt_stp_mt_check(const struct xt_mtchk_param *par)
 	    !(info->bitmask & EBT_STP_MASK))
 		return -EINVAL;
 	/* Make sure the match only receives stp frames */
-	if (!ether_addr_equal(e->destmac, bridge_ula) ||
-	    !ether_addr_equal(e->destmsk, msk) || !(e->bitmask & EBT_DESTMAC))
+	if (!par->nft_compat &&
+	    (!ether_addr_equal(e->destmac, bridge_ula) ||
+	     !ether_addr_equal(e->destmsk, msk) ||
+	     !(e->bitmask & EBT_DESTMAC)))
 		return -EINVAL;
 
 	return 0;
diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
index e90f83a..a287e02 100644
--- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
+++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
@@ -367,6 +367,11 @@ static int clusterip_tg_check(const struct xt_tgchk_param *par)
 	struct clusterip_config *config;
 	int ret;
 
+	if (par->nft_compat) {
+		pr_err("cannot use this target from nftables compat\n");
+		return -EINVAL;
+	}
+
 	if (cipinfo->hash_mode != CLUSTERIP_HASHMODE_SIP &&
 	    cipinfo->hash_mode != CLUSTERIP_HASHMODE_SIP_SPT &&
 	    cipinfo->hash_mode != CLUSTERIP_HASHMODE_SIP_SPT_DPT) {
diff --git a/net/ipv6/netfilter/ip6t_SYNPROXY.c b/net/ipv6/netfilter/ip6t_SYNPROXY.c
index a0d1727..ec00b1b 100644
--- a/net/ipv6/netfilter/ip6t_SYNPROXY.c
+++ b/net/ipv6/netfilter/ip6t_SYNPROXY.c
@@ -430,8 +430,7 @@ static int synproxy_tg6_check(const struct xt_tgchk_param *par)
 {
 	const struct ip6t_entry *e = par->entryinfo;
 
-	if (!(e->ipv6.flags & IP6T_F_PROTO) ||
-	    e->ipv6.proto != IPPROTO_TCP ||
+	if (e->ipv6.proto != IPPROTO_TCP ||
 	    e->ipv6.invflags & XT_INV_PROTO)
 		return -EINVAL;
 
diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c
index a990df2..365c531 100644
--- a/net/netfilter/nft_compat.c
+++ b/net/netfilter/nft_compat.c
@@ -158,6 +158,7 @@ nft_target_set_tgchk_param(struct xt_tgchk_param *par,
 		par->hook_mask = 0;
 	}
 	par->family	= ctx->afi->family;
+	par->nft_compat = true;
 }
 
 static void target_compat_from_user(struct xt_target *t, void *in, void *out)
@@ -371,6 +372,7 @@ nft_match_set_mtchk_param(struct xt_mtchk_param *par, const struct nft_ctx *ctx,
 		par->hook_mask = 0;
 	}
 	par->family	= ctx->afi->family;
+	par->nft_compat = true;
 }
 
 static void match_compat_from_user(struct xt_match *m, void *in, void *out)
diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
index e762de5..3b9761f 100644
--- a/net/netfilter/xt_TCPMSS.c
+++ b/net/netfilter/xt_TCPMSS.c
@@ -97,6 +97,9 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 	if (!skb_make_writable(skb, skb->len))
 		return -1;
 
+	if (unlikely(!tcph->syn))
+		return 0;
+
 	len = skb->len - tcphoff;
 	if (len < (int)sizeof(struct tcphdr))
 		return -1;
@@ -277,6 +280,9 @@ static int tcpmss_tg4_check(const struct xt_tgchk_param *par)
 			"FORWARD, OUTPUT and POSTROUTING hooks\n");
 		return -EINVAL;
 	}
+	if (par->nft_compat)
+		return 0;
+
 	xt_ematch_foreach(ematch, e)
 		if (find_syn_match(ematch))
 			return 0;
@@ -299,6 +305,9 @@ static int tcpmss_tg6_check(const struct xt_tgchk_param *par)
 			"FORWARD, OUTPUT and POSTROUTING hooks\n");
 		return -EINVAL;
 	}
+	if (par->nft_compat)
+		return 0;
+
 	xt_ematch_foreach(ematch, e)
 		if (find_syn_match(ematch))
 			return 0;
-- 
1.7.10.4

Re: [PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Patrick McHardy]

On 02.03, Pablo Neira Ayuso wrote:
> Currently, we have four xtables extensions that cannot be used from the
> xt over nft compat layer. The problem is that they need real access to
> the full blown xt_entry to validate that the rule comes with the right
> dependencies. This check was introduced to overcome the lack of
> sufficient userspace dependency validation in iptables.
> 
> To resolve this problem, this patch introduces a new field to the
> xt_tgchk_param structure that tell us if the target is executed from
> nft_compat context.
> 
> The four affected extensions are:
> 
> 1) CLUSTERIP, this target has been superseded by xt_cluster. So just
>    bail out by returning -EINVAL.
> 
> 2) TCPMSS. Relax the checking when used from nft_compat and make sure
>    that we skip !syn packets in case userspace provides a wrong
>    configuration.
> 
> 3) SYMPROXY6. Don't check for e->ipv6.flags, we can instead check
>    for e->ipv6.proto as other extensions do, if zero then it doesn't
>    fulfill the dependency.

But we don't perform a protocol match in ip6_tables if the IP6T_F_PROTO
flag is not given. ip6_tables differs from ip_tables in this regard.

Re: [PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 2) TCPMSS. Relax the checking when used from nft_compat and make sure
>    that we skip !syn packets in case userspace provides a wrong
>    configuration.
> diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
> index e762de5..3b9761f 100644
> --- a/net/netfilter/xt_TCPMSS.c
> +++ b/net/netfilter/xt_TCPMSS.c
> @@ -97,6 +97,9 @@ tcpmss_mangle_packet(struct sk_buff *skb,
>  	if (!skb_make_writable(skb, skb->len))
>  		return -1;
>  
> +	if (unlikely(!tcph->syn))
> +		return 0;
> +
>  	len = skb->len - tcphoff;

Applying this to my copy of nf-next would insert this test before
tcph is set up.

Re: [PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Patrick McHardy]

On 02.03, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > 2) TCPMSS. Relax the checking when used from nft_compat and make sure
> >    that we skip !syn packets in case userspace provides a wrong
> >    configuration.
> > diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
> > index e762de5..3b9761f 100644
> > --- a/net/netfilter/xt_TCPMSS.c
> > +++ b/net/netfilter/xt_TCPMSS.c
> > @@ -97,6 +97,9 @@ tcpmss_mangle_packet(struct sk_buff *skb,
> >  	if (!skb_make_writable(skb, skb->len))
> >  		return -1;
> >  
> > +	if (unlikely(!tcph->syn))
> > +		return 0;
> > +
> >  	len = skb->len - tcphoff;
> 
> Applying this to my copy of nf-next would insert this test before
> tcph is set up.

I actually don't think the test is necessary at all. Since we
don't check the protocol with nft, any packet with that bit set
will pass. It will most likely fail or corrupt the packet, but
why should we care? It won't crash and with nft its the
responsibility of userspace to take care of using the extension
correctly.

Re: [PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Pablo Neira Ayuso]

On Mon, Mar 02, 2015 at 02:19:40PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > 2) TCPMSS. Relax the checking when used from nft_compat and make sure
> >    that we skip !syn packets in case userspace provides a wrong
> >    configuration.
> > diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
> > index e762de5..3b9761f 100644
> > --- a/net/netfilter/xt_TCPMSS.c
> > +++ b/net/netfilter/xt_TCPMSS.c
> > @@ -97,6 +97,9 @@ tcpmss_mangle_packet(struct sk_buff *skb,
> >  	if (!skb_make_writable(skb, skb->len))
> >  		return -1;
> >  
> > +	if (unlikely(!tcph->syn))
> > +		return 0;
> > +
> >  	len = skb->len - tcphoff;
> 
> Applying this to my copy of nf-next would insert this test before
> tcph is set up.

Thanks, will fix this up.

Re: [PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Pablo Neira Ayuso]

On Mon, Mar 02, 2015 at 01:27:05PM +0000, Patrick McHardy wrote:
> On 02.03, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > 2) TCPMSS. Relax the checking when used from nft_compat and make sure
> > >    that we skip !syn packets in case userspace provides a wrong
> > >    configuration.
> > > diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
> > > index e762de5..3b9761f 100644
> > > --- a/net/netfilter/xt_TCPMSS.c
> > > +++ b/net/netfilter/xt_TCPMSS.c
> > > @@ -97,6 +97,9 @@ tcpmss_mangle_packet(struct sk_buff *skb,
> > >  	if (!skb_make_writable(skb, skb->len))
> > >  		return -1;
> > >  
> > > +	if (unlikely(!tcph->syn))
> > > +		return 0;
> > > +
> > >  	len = skb->len - tcphoff;
> > 
> > Applying this to my copy of nf-next would insert this test before
> > tcph is set up.
> 
> I actually don't think the test is necessary at all. Since we
> don't check the protocol with nft, any packet with that bit set
> will pass. It will most likely fail or corrupt the packet, but
> why should we care? It won't crash and with nft its the
> responsibility of userspace to take care of using the extension
> correctly.

According to what I can read from the code, it will corrupt the
packet since we will get a MSS option in non-syn TCP packet.

I thought we could crash, but looking at it again you seem to be
right, I'm going to undo this change and send a v2.

Re: [PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Pablo Neira Ayuso]

On Mon, Mar 02, 2015 at 01:19:01PM +0000, Patrick McHardy wrote:
> On 02.03, Pablo Neira Ayuso wrote:
> > Currently, we have four xtables extensions that cannot be used from the
> > xt over nft compat layer. The problem is that they need real access to
> > the full blown xt_entry to validate that the rule comes with the right
> > dependencies. This check was introduced to overcome the lack of
> > sufficient userspace dependency validation in iptables.
> > 
> > To resolve this problem, this patch introduces a new field to the
> > xt_tgchk_param structure that tell us if the target is executed from
> > nft_compat context.
> > 
> > The four affected extensions are:
> > 
> > 1) CLUSTERIP, this target has been superseded by xt_cluster. So just
> >    bail out by returning -EINVAL.
> > 
> > 2) TCPMSS. Relax the checking when used from nft_compat and make sure
> >    that we skip !syn packets in case userspace provides a wrong
> >    configuration.
> > 
> > 3) SYMPROXY6. Don't check for e->ipv6.flags, we can instead check
> >    for e->ipv6.proto as other extensions do, if zero then it doesn't
> >    fulfill the dependency.
> 
> But we don't perform a protocol match in ip6_tables if the IP6T_F_PROTO
> flag is not given. ip6_tables differs from ip_tables in this regard.

This just makes sure that SYNPROXY6 is not called for non-tcp traffic
in the rule loading path, which should be OK.

Re: [PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Patrick McHardy]

On 02.03, Pablo Neira Ayuso wrote:
> On Mon, Mar 02, 2015 at 01:19:01PM +0000, Patrick McHardy wrote:
> > On 02.03, Pablo Neira Ayuso wrote:
> > > Currently, we have four xtables extensions that cannot be used from the
> > > xt over nft compat layer. The problem is that they need real access to
> > > the full blown xt_entry to validate that the rule comes with the right
> > > dependencies. This check was introduced to overcome the lack of
> > > sufficient userspace dependency validation in iptables.
> > > 
> > > To resolve this problem, this patch introduces a new field to the
> > > xt_tgchk_param structure that tell us if the target is executed from
> > > nft_compat context.
> > > 
> > > The four affected extensions are:
> > > 
> > > 1) CLUSTERIP, this target has been superseded by xt_cluster. So just
> > >    bail out by returning -EINVAL.
> > > 
> > > 2) TCPMSS. Relax the checking when used from nft_compat and make sure
> > >    that we skip !syn packets in case userspace provides a wrong
> > >    configuration.
> > > 
> > > 3) SYMPROXY6. Don't check for e->ipv6.flags, we can instead check
> > >    for e->ipv6.proto as other extensions do, if zero then it doesn't
> > >    fulfill the dependency.
> > 
> > But we don't perform a protocol match in ip6_tables if the IP6T_F_PROTO
> > flag is not given. ip6_tables differs from ip_tables in this regard.
> 
> This just makes sure that SYNPROXY6 is not called for non-tcp traffic
> in the rule loading path, which should be OK.

Yeah, but for ip6_tables we actually need the check the way it is,
without IP6T_F_PROTO we will not perform the protocol match.

Re: [PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Pablo Neira Ayuso]

On Mon, Mar 02, 2015 at 02:19:00PM +0000, Patrick McHardy wrote:
> On 02.03, Pablo Neira Ayuso wrote:
> > On Mon, Mar 02, 2015 at 01:19:01PM +0000, Patrick McHardy wrote:
> > > On 02.03, Pablo Neira Ayuso wrote:
> > > > Currently, we have four xtables extensions that cannot be used from the
> > > > xt over nft compat layer. The problem is that they need real access to
> > > > the full blown xt_entry to validate that the rule comes with the right
> > > > dependencies. This check was introduced to overcome the lack of
> > > > sufficient userspace dependency validation in iptables.
> > > > 
> > > > To resolve this problem, this patch introduces a new field to the
> > > > xt_tgchk_param structure that tell us if the target is executed from
> > > > nft_compat context.
> > > > 
> > > > The four affected extensions are:
> > > > 
> > > > 1) CLUSTERIP, this target has been superseded by xt_cluster. So just
> > > >    bail out by returning -EINVAL.
> > > > 
> > > > 2) TCPMSS. Relax the checking when used from nft_compat and make sure
> > > >    that we skip !syn packets in case userspace provides a wrong
> > > >    configuration.
> > > > 
> > > > 3) SYMPROXY6. Don't check for e->ipv6.flags, we can instead check
> > > >    for e->ipv6.proto as other extensions do, if zero then it doesn't
> > > >    fulfill the dependency.
> > > 
> > > But we don't perform a protocol match in ip6_tables if the IP6T_F_PROTO
> > > flag is not given. ip6_tables differs from ip_tables in this regard.
> > 
> > This just makes sure that SYNPROXY6 is not called for non-tcp traffic
> > in the rule loading path, which should be OK.
> 
> Yeah, but for ip6_tables we actually need the check the way it is,
> without IP6T_F_PROTO we will not perform the protocol match.

        if (!(e->ipv6.flags & IP6T_F_PROTO) ||
            e->ipv6.proto != IPPROTO_TCP ||
            e->ipv6.invflags & XT_INV_PROTO)
                return -EINVAL;

e->ipv6.flags & IP6T_F_PROTO seems redundant to me, it just says
e->ipv6.proto is set.

If that flag is not set, then e->ipv6.proto is left unset. But the
effect should be the same since 0 != IPPROTO_TCP.

This is just relaxing the validation in SYNPROXY6 to only check
e->ipv6.proto which is what nft_compat sets.

Am I missing anything?

Re: [PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Patrick McHardy]

On 02.03, Pablo Neira Ayuso wrote:
> On Mon, Mar 02, 2015 at 02:19:00PM +0000, Patrick McHardy wrote:
> > On 02.03, Pablo Neira Ayuso wrote:
> > > On Mon, Mar 02, 2015 at 01:19:01PM +0000, Patrick McHardy wrote:
> > > > > 
> > > > > 3) SYMPROXY6. Don't check for e->ipv6.flags, we can instead check
> > > > >    for e->ipv6.proto as other extensions do, if zero then it doesn't
> > > > >    fulfill the dependency.
> > > > 
> > > > But we don't perform a protocol match in ip6_tables if the IP6T_F_PROTO
> > > > flag is not given. ip6_tables differs from ip_tables in this regard.
> > > 
> > > This just makes sure that SYNPROXY6 is not called for non-tcp traffic
> > > in the rule loading path, which should be OK.
> > 
> > Yeah, but for ip6_tables we actually need the check the way it is,
> > without IP6T_F_PROTO we will not perform the protocol match.
> 
>         if (!(e->ipv6.flags & IP6T_F_PROTO) ||
>             e->ipv6.proto != IPPROTO_TCP ||
>             e->ipv6.invflags & XT_INV_PROTO)
>                 return -EINVAL;
> 
> e->ipv6.flags & IP6T_F_PROTO seems redundant to me, it just says
> e->ipv6.proto is set.

No, it also instructs ip6_tables to actually match on that value.

> If that flag is not set, then e->ipv6.proto is left unset. But the
> effect should be the same since 0 != IPPROTO_TCP.
> 
> This is just relaxing the validation in SYNPROXY6 to only check
> e->ipv6.proto which is what nft_compat sets.
> 
> Am I missing anything?

Yes, ip6_tables only performs the protocol match when that flag is set.
Look:

        /* look for the desired protocol header */
        if((ip6info->flags & IP6T_F_PROTO)) {
		...
		if (ip6info->proto == protohdr) {
	
This is where ip6_tables and ip_tables are different, ip_tables
takes ipinfo->proto as an indicator, ip6_tables the flag.

Re: [PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Pablo Neira Ayuso]

On Mon, Mar 02, 2015 at 02:36:47PM +0000, Patrick McHardy wrote:
> On 02.03, Pablo Neira Ayuso wrote:
> > On Mon, Mar 02, 2015 at 02:19:00PM +0000, Patrick McHardy wrote:
> > > On 02.03, Pablo Neira Ayuso wrote:
> > > > On Mon, Mar 02, 2015 at 01:19:01PM +0000, Patrick McHardy wrote:
> > > > > > 
> > > > > > 3) SYMPROXY6. Don't check for e->ipv6.flags, we can instead check
> > > > > >    for e->ipv6.proto as other extensions do, if zero then it doesn't
> > > > > >    fulfill the dependency.
> > > > > 
> > > > > But we don't perform a protocol match in ip6_tables if the IP6T_F_PROTO
> > > > > flag is not given. ip6_tables differs from ip_tables in this regard.
> > > > 
> > > > This just makes sure that SYNPROXY6 is not called for non-tcp traffic
> > > > in the rule loading path, which should be OK.
> > > 
> > > Yeah, but for ip6_tables we actually need the check the way it is,
> > > without IP6T_F_PROTO we will not perform the protocol match.
> > 
> >         if (!(e->ipv6.flags & IP6T_F_PROTO) ||
> >             e->ipv6.proto != IPPROTO_TCP ||
> >             e->ipv6.invflags & XT_INV_PROTO)
> >                 return -EINVAL;
> > 
> > e->ipv6.flags & IP6T_F_PROTO seems redundant to me, it just says
> > e->ipv6.proto is set.
> 
> No, it also instructs ip6_tables to actually match on that value.
> 
> > If that flag is not set, then e->ipv6.proto is left unset. But the
> > effect should be the same since 0 != IPPROTO_TCP.
> > 
> > This is just relaxing the validation in SYNPROXY6 to only check
> > e->ipv6.proto which is what nft_compat sets.
> > 
> > Am I missing anything?
> 
> Yes, ip6_tables only performs the protocol match when that flag is set.
> Look:
> 
>         /* look for the desired protocol header */
>         if((ip6info->flags & IP6T_F_PROTO)) {
> 		...
> 		if (ip6info->proto == protohdr) {
> 	
> This is where ip6_tables and ip_tables are different, ip_tables
> takes ipinfo->proto as an indicator, ip6_tables the flag.

I'm not altering the ip6_tables core logic.

ip6tables from userspace sets IP6T_F_PROTO, the core handles things as
you described, but SYNPROXY6 only checks if e->ipv6.proto != IPPROTO_TCP.

REJECT6 does exactly the same that I need:

                if (e->ipv6.proto != IPPROTO_TCP ||                              
                    (e->ipv6.invflags & XT_INV_PROTO)) {                         
                        pr_info("TCP_RESET illegal for non-tcp\n");              
                        return -EINVAL;                                          
                }

I just want to relax the check in SYNPROXY6, so it works fine with
nft_compat. I think nothing will break.

Re: [PATCH nf-next] netfilter: x_tables: add context to know if extension runs from nft_compat

[Patrick McHardy]

On 02.03, Pablo Neira Ayuso wrote:
> On Mon, Mar 02, 2015 at 02:36:47PM +0000, Patrick McHardy wrote:
> > > > > This just makes sure that SYNPROXY6 is not called for non-tcp traffic
> > > > > in the rule loading path, which should be OK.
> > > > 
> > > > Yeah, but for ip6_tables we actually need the check the way it is,
> > > > without IP6T_F_PROTO we will not perform the protocol match.
> > > 
> > >         if (!(e->ipv6.flags & IP6T_F_PROTO) ||
> > >             e->ipv6.proto != IPPROTO_TCP ||
> > >             e->ipv6.invflags & XT_INV_PROTO)
> > >                 return -EINVAL;
> > > 
> > > e->ipv6.flags & IP6T_F_PROTO seems redundant to me, it just says
> > > e->ipv6.proto is set.
> > 
> > No, it also instructs ip6_tables to actually match on that value.
> > 
> > > If that flag is not set, then e->ipv6.proto is left unset. But the
> > > effect should be the same since 0 != IPPROTO_TCP.
> > > 
> > > This is just relaxing the validation in SYNPROXY6 to only check
> > > e->ipv6.proto which is what nft_compat sets.
> > > 
> > > Am I missing anything?
> > 
> > Yes, ip6_tables only performs the protocol match when that flag is set.
> > Look:
> > 
> >         /* look for the desired protocol header */
> >         if((ip6info->flags & IP6T_F_PROTO)) {
> > 		...
> > 		if (ip6info->proto == protohdr) {
> > 	
> > This is where ip6_tables and ip_tables are different, ip_tables
> > takes ipinfo->proto as an indicator, ip6_tables the flag.
> 
> I'm not altering the ip6_tables core logic.
> 
> ip6tables from userspace sets IP6T_F_PROTO, the core handles things as
> you described, but SYNPROXY6 only checks if e->ipv6.proto != IPPROTO_TCP.

It will allow an ip6tables rule containing e->ipv6.proto == IPPROTO_TCP
but not IP6T_F_PROTO, meaning that the protocol match is not performed,
ergo any protocol can hit the target at runtime if such a rule is set.

I'm not saying that this will necessarily break something since
ip6tables does set that flag, but we usually do perform full validation
in ip*tables in the kernel, this patch changes that logic.

> REJECT6 does exactly the same that I need:
> 
>                 if (e->ipv6.proto != IPPROTO_TCP ||                              
>                     (e->ipv6.invflags & XT_INV_PROTO)) {                         
>                         pr_info("TCP_RESET illegal for non-tcp\n");              
>                         return -EINVAL;                                          
>                 }

That contains the same problem. I'd say most people have copy and
pasted this from iptables without realizing the difference.

> I just want to relax the check in SYNPROXY6, so it works fine with
> nft_compat. I think nothing will break.

Not with our current userspace, I agree. But its a change in policy
for ip*tables to rely on that.