netdev - Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190617224232.55hldt4bw2qcmnll@breakpoint.cc>
Date:   Tue, 18 Jun 2019 00:42:32 +0200
From:   Florian Westphal <fw@...len.de>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     Florian Westphal <fw@...len.de>, wenxu@...oud.cn,
        netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] netfilter: nft_paylaod: add base type
 NFT_PAYLOAD_LL_HEADER_NO_TAG

Pablo Neira Ayuso <pablo@...filter.org> wrote:
> > Subject: Change bridge l3 dependency to meta protocol
> > 
> > This examines skb->protocol instead of ethernet header type, which
> > might be different when vlan is involved.
> >  
> > +	if (ctx->pctx.family == NFPROTO_BRIDGE && desc == &proto_eth) {
> > +		if (expr->payload.desc == &proto_ip ||
> > +		    expr->payload.desc == &proto_ip6)
> > +			desc = &proto_metaeth;
> > +	}i
> 
> Is this sufficient to restrict the matching? Is this still buggy from
> ingress?

This is what netdev family uses as well (skb->protocol i mean).
I'm not sure it will work for output however (haven't checked).

> I wonder if an explicit NFT_PAYLOAD_CHECK_VLAN flag would be useful in
> the kernel, if so we could rename NFTA_PAYLOAD_CSUM_FLAGS to
> NFTA_PAYLOAD_FLAGS and place it there. Just an idea.

What would NFT_PAYLOAD_CHECK_VLAN do?
You mean disable/enable the 'vlan is there' illusion that nft_payload
provides?  That would work as well of course, but I would prefer to
switch to meta dependencies where possible so we don't rely on
particular layout of a different header class (e.g. meta l4proto doesn't
depend on ip version, and meta protocol won't depend on particular
ethernet frame).

What might be useful is an nft switch to turn off dependeny
insertion, this would also avoid the problem (if users restrict the
matching properly...).

Another unresolved issue is presence of multiple vlan tags, so we might
have to add yet another meta key to retrieve the l3 protocol in use

(the problem at hand was 'ip protocol icmp' not matching traffic inside
 a vlan).

The other issue is lack of vlan awareness in some bridge/netdev
expressions, e.g. reject.

I think we could apply this patch to nft after making sure it works
for output as thats probably the only solution that won't need
changes in the kernel.

If it doesn't, we will need to find a different solution in any case.