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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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?