[PATCH nf] netfilter: nf_tables: set pktinfo->thoff at AH header if found

[PATCH nf] netfilter: nf_tables: set pktinfo->thoff at AH header if found

[Pablo Neira Ayuso]

Phil Sutter reports that IPv6 AH header matching is broken. From
userspace, nft generates bytecode that expects to find the AH header at
NFT_PAYLOAD_TRANSPORT_HEADER both for IPv4 and IPv6. However,
pktinfo->thoff is set to the inner header after the AH header in IPv6,
while in IPv4 pktinfo->thoff points to the AH header indeed. This
behaviour is inconsistent. This patch fixes this problem by updating
ipv6_find_hdr() to get the IP6_FH_F_AUTH flag so this function stops at
the AH header, so both IPv4 and IPv6 pktinfo->thoff point to the AH
header.

Reported-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
I prefer to fix this now, rather than living with this inconsistency
forever. nft IPv6 AH matching has been broken since day 1, so we're sure
that nobody has been using this.

For those willing match to inner headers, I prefer we explore inner
matching via explicit syntax and likely new payload base definition in
enum nft_payload_bases.

 include/net/netfilter/nf_tables_ipv6.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/net/netfilter/nf_tables_ipv6.h b/include/net/netfilter/nf_tables_ipv6.h
index d150b5066201..97983d1c05e4 100644
--- a/include/net/netfilter/nf_tables_ipv6.h
+++ b/include/net/netfilter/nf_tables_ipv6.h
@@ -9,12 +9,13 @@ nft_set_pktinfo_ipv6(struct nft_pktinfo *pkt,
 		     struct sk_buff *skb,
 		     const struct nf_hook_state *state)
 {
+	unsigned int flags = IP6_FH_F_AUTH;
 	int protohdr, thoff = 0;
 	unsigned short frag_off;
 
 	nft_set_pktinfo(pkt, skb, state);
 
-	protohdr = ipv6_find_hdr(pkt->skb, &thoff, -1, &frag_off, NULL);
+	protohdr = ipv6_find_hdr(pkt->skb, &thoff, -1, &frag_off, &flags);
 	if (protohdr < 0) {
 		nft_set_pktinfo_proto_unspec(pkt, skb);
 		return;
@@ -32,6 +33,7 @@ __nft_set_pktinfo_ipv6_validate(struct nft_pktinfo *pkt,
 				const struct nf_hook_state *state)
 {
 #if IS_ENABLED(CONFIG_IPV6)
+	unsigned int flags = IP6_FH_F_AUTH;
 	struct ipv6hdr *ip6h, _ip6h;
 	unsigned int thoff = 0;
 	unsigned short frag_off;
@@ -50,7 +52,7 @@ __nft_set_pktinfo_ipv6_validate(struct nft_pktinfo *pkt,
 	if (pkt_len + sizeof(*ip6h) > skb->len)
 		return -1;
 
-	protohdr = ipv6_find_hdr(pkt->skb, &thoff, -1, &frag_off, NULL);
+	protohdr = ipv6_find_hdr(pkt->skb, &thoff, -1, &frag_off, &flags);
 	if (protohdr < 0)
 		return -1;
 
-- 
2.1.4

Re: [PATCH nf] netfilter: nf_tables: set pktinfo->thoff at AH header if found

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Phil Sutter reports that IPv6 AH header matching is broken. From
> userspace, nft generates bytecode that expects to find the AH header at
> NFT_PAYLOAD_TRANSPORT_HEADER both for IPv4 and IPv6. However,
> pktinfo->thoff is set to the inner header after the AH header in IPv6,
> while in IPv4 pktinfo->thoff points to the AH header indeed. This
> behaviour is inconsistent. This patch fixes this problem by updating
> ipv6_find_hdr() to get the IP6_FH_F_AUTH flag so this function stops at
> the AH header, so both IPv4 and IPv6 pktinfo->thoff point to the AH
> header.

This looks wrong.  We need to search until we find a l4 header,
after this patch "tcp dort 22" won't match anymore if an AH header
exists.

Re: [PATCH nf] netfilter: nf_tables: set pktinfo->thoff at AH header if found

[Pablo Neira Ayuso]

On Sat, Mar 04, 2017 at 08:43:42PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Phil Sutter reports that IPv6 AH header matching is broken. From
> > userspace, nft generates bytecode that expects to find the AH header at
> > NFT_PAYLOAD_TRANSPORT_HEADER both for IPv4 and IPv6. However,
> > pktinfo->thoff is set to the inner header after the AH header in IPv6,
> > while in IPv4 pktinfo->thoff points to the AH header indeed. This
> > behaviour is inconsistent. This patch fixes this problem by updating
> > ipv6_find_hdr() to get the IP6_FH_F_AUTH flag so this function stops at
> > the AH header, so both IPv4 and IPv6 pktinfo->thoff point to the AH
> > header.
> 
> This looks wrong.  We need to search until we find a l4 header,
> after this patch "tcp dort 22" won't match anymore if an AH header
> exists.

Look Florian:

# nft --debug=netlink add rule inet x y tcp dport 22 counter
inet x y 
  [ meta load l4proto => reg 1 ]
  [ cmp eq reg 1 0x00000006 ]
  [ payload load 2b @ transport header + 2 => reg 1 ]
  [ cmp eq reg 1 0x00001600 ]
  [ counter pkts 0 bytes 0 ]

<cmdline>:1:1-39: Error: Could not process rule: No such file or directory

If we leave things the way they are:

1) A packet that looks like IPv4 + AH + TCP dport 22 will match.
2) A packet that looks like IPv6 + AH + TCP dport 22 will *not* match.

If you really want to keep this inconsistent behaviour around, I let
you inaugurate the errata section of the nft manpage ;-)

Seriously, I think it's better if we just fix this and focus on
designing a way to match at inner headers if you really need to match
on tcp ports encapsulated in ah traffic.

Re: [PATCH nf] netfilter: nf_tables: set pktinfo->thoff at AH header if found

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sat, Mar 04, 2017 at 08:43:42PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Phil Sutter reports that IPv6 AH header matching is broken. From
> > > userspace, nft generates bytecode that expects to find the AH header at
> > > NFT_PAYLOAD_TRANSPORT_HEADER both for IPv4 and IPv6. However,
> > > pktinfo->thoff is set to the inner header after the AH header in IPv6,
> > > while in IPv4 pktinfo->thoff points to the AH header indeed. This
> > > behaviour is inconsistent. This patch fixes this problem by updating
> > > ipv6_find_hdr() to get the IP6_FH_F_AUTH flag so this function stops at
> > > the AH header, so both IPv4 and IPv6 pktinfo->thoff point to the AH
> > > header.
> > 
> > This looks wrong.  We need to search until we find a l4 header,
> > after this patch "tcp dort 22" won't match anymore if an AH header
> > exists.
> 
> Look Florian:
> 
> # nft --debug=netlink add rule inet x y tcp dport 22 counter
> inet x y 
>   [ meta load l4proto => reg 1 ]
>   [ cmp eq reg 1 0x00000006 ]
>   [ payload load 2b @ transport header + 2 => reg 1 ]
>   [ cmp eq reg 1 0x00001600 ]
>   [ counter pkts 0 bytes 0 ]
> 
> <cmdline>:1:1-39: Error: Could not process rule: No such file or directory
> 
> If we leave things the way they are:
> 
> 1) A packet that looks like IPv4 + AH + TCP dport 22 will match.

It won't match (since iph->protocol is ah, not tcp).

> 2) A packet that looks like IPv6 + AH + TCP dport 22 will *not* match.

We skip all exthdrs and set thoff to the tcp header, so yes, this is
indeed inconsitent and should be fixed.

> If you really want to keep this inconsistent behaviour around, I let
> you inaugurate the errata section of the nft manpage ;-)

Yuck :)

> Seriously, I think it's better if we just fix this and focus on
> designing a way to match at inner headers if you really need to match
> on tcp ports encapsulated in ah traffic.

Hmm, question is if it should match by default or not (i.e. if the
ipv4 or the ipv6 behaviour is the 'better' one).

One could argue that 'tcp dport 22' should in fact match ip/ah/tcp as
well, OTOH same could be said for gre and other tunnel encaps so
treating AH as a 'final' ipv6 exthdr seems to be the right thing to do
(we can't possibly guess other tunnel headers to skip anyway).

So I am fine with this patch after all, thanks for explaining.