netdev - Re: [PATCH net] netfilter: bridge: set skb transport_header before entering NF_INET_PRE

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: <20190313115948.zhnov5ze6rdttm7k@breakpoint.cc>
Date:   Wed, 13 Mar 2019 12:59:48 +0100
From:   Florian Westphal <fw@...len.de>
To:     Neil Horman <nhorman@...driver.com>
Cc:     Xin Long <lucien.xin@...il.com>,
        network dev <netdev@...r.kernel.org>,
        netfilter-devel@...r.kernel.org,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        pablo@...filter.org, fw@...len.de
Subject: Re: [PATCH net] netfilter: bridge: set skb transport_header before
 entering NF_INET_PRE_ROUTING

Neil Horman <nhorman@...driver.com> wrote:
> On Wed, Mar 13, 2019 at 04:33:29PM +0800, Xin Long wrote:
> > Since Commit 21d1196a35f5 ("ipv4: set transport header earlier"),
> > skb->transport_header has been always set before entering INET
> > netfilter. This patch is to set skb->transport_header for bridge
> > before entering INET netfilter by bridge-nf-call-iptables.
> > 
> > It also fixes an issue that sctp_error() couldn't compute a right
> > csum due to unset skb->transport_header.
> > 
> > Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> > Reported-by: Li Shuang <shuali@...hat.com>
> > Suggested-by: Pablo Neira Ayuso <pablo@...filter.org>
> > Signed-off-by: Xin Long <lucien.xin@...il.com>
> > ---
> >  net/bridge/br_netfilter_hooks.c | 1 +
> >  net/bridge/br_netfilter_ipv6.c  | 2 ++
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> > index c93c35b..4d09a33 100644
> > --- a/net/bridge/br_netfilter_hooks.c
> > +++ b/net/bridge/br_netfilter_hooks.c
> > @@ -502,6 +502,7 @@ static unsigned int br_nf_pre_routing(void *priv,
> >  	nf_bridge->ipv4_daddr = ip_hdr(skb)->daddr;
> >  
> >  	skb->protocol = htons(ETH_P_IP);
> > +	skb->transport_header = skb->network_header + ip_hdr(skb)->ihl * 4;
> >  
> >  	NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> >  		skb->dev, NULL,
> > diff --git a/net/bridge/br_netfilter_ipv6.c b/net/bridge/br_netfilter_ipv6.c
> > index 564710f..e88d664 100644
> > --- a/net/bridge/br_netfilter_ipv6.c
> > +++ b/net/bridge/br_netfilter_ipv6.c
> > @@ -235,6 +235,8 @@ unsigned int br_nf_pre_routing_ipv6(void *priv,
> >  	nf_bridge->ipv6_daddr = ipv6_hdr(skb)->daddr;
> >  
> >  	skb->protocol = htons(ETH_P_IPV6);
> > +	skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
> > +
> >  	NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING, state->net, state->sk, skb,
> >  		skb->dev, NULL,
> >  		br_nf_pre_routing_finish_ipv6);
> > -- 
> > 2.1.0
> > 
> > 
> Acked-by: Neil Horman <nhorman@...driver.com>

Acked-by: Florian Westphal <fw@...len.de>

... because this fixes discrepancy of normal stack vs. bridge path.

However, I think we still need a fix for the ipv6 case as
there is no guarantee ipv6 nexthdr will be the sctp one.

We already pass the sctp offset as argument to sctp_compute_cksum(),
why can't we just replace sctp_hdr(skb) with skb->data + dataoff, as
Xin Long originally suggested?

IOW, whats the problem with
https://marc.info/?l=linux-netdev&m=155109395226858&w=2 ?

(In theory conntrack can also inspect transport header inside icmp error
 messages like pkttoobig and so on,
 although this won't be the case here as we can't validate csum anyway
 if the packet isn't complete).

Just pointing out that we can't rely on skb transport header being
"correct" in all cases from netfilter point of view.