RE: [PATCH nf 1/1] netfilter: seqadj: Fix possible non-linear data access for TCP header

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: 高峰 [mailto:fgao@xxxxxxxxxx]
> Sent: Tuesday, April 11, 2017 9:00 AM
> To: 'Pablo Neira Ayuso' <pablo@xxxxxxxxxxxxx>; 'gfree.wind@xxxxxxxxxxx'
> <gfree.wind@xxxxxxxxxxx>
> Cc: 'netfilter-devel@xxxxxxxxxxxxxxx' <netfilter-devel@xxxxxxxxxxxxxxx>
> Subject: RE: [PATCH nf 1/1] netfilter: seqadj: Fix possible non-linear
data access
> for TCP header
> 
> Hi Pablo,
> 
> > -----Original Message-----
> > From: Pablo Neira Ayuso [mailto:pablo@xxxxxxxxxxxxx]
> > Sent: Monday, April 10, 2017 8:07 PM
> > To: gfree.wind@xxxxxxxxxxx
> > Cc: netfilter-devel@xxxxxxxxxxxxxxx; Gao Feng <fgao@xxxxxxxxxx>
> > Subject: Re: [PATCH nf 1/1] netfilter: seqadj: Fix possible non-linear
> > data access for TCP header
> >
> > On Mon, Apr 10, 2017 at 06:36:03PM +0800, gfree.wind@xxxxxxxxxxx
> wrote:
> > > From: Gao Feng <fgao@xxxxxxxxxx>
> > >
> > > The current call path of nf_ct_tcp_seqadj_set is the following.
> > >
> > > nfqnl_recv_verdict->ctnetlink_glue_hook->ctnetlink_glue_seqadj
> > > ->nf_ct_tcp_seqadj_set.
> > >
> > > It couldn't make sure the TCP header is in the linear data part.
> > > So use the skb_header_pointer instead of the current codes.
> > >
> > > BTW, the nf_ct_tcp_seqadj_set is one external function of netfilter
> > > which works in the network layer, it should not assume the transport
> > > header is in the linear data.
> > >
> > > Signed-off-by: Gao Feng <fgao@xxxxxxxxxx>
> > > ---
> > >  net/netfilter/nf_conntrack_seqadj.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/netfilter/nf_conntrack_seqadj.c
> > > b/net/netfilter/nf_conntrack_seqadj.c
> > > index ef7063e..80394ab 100644
> > > --- a/net/netfilter/nf_conntrack_seqadj.c
> > > +++ b/net/netfilter/nf_conntrack_seqadj.c
> > > @@ -61,11 +61,14 @@ void nf_ct_tcp_seqadj_set(struct sk_buff *skb,
> > >  			  s32 off)
> > >  {
> > >  	const struct tcphdr *th;
> > > +	struct tcphdr tcph;
> > >
> > >  	if (nf_ct_protonum(ct) != IPPROTO_TCP)
> > >  		return;
> > >
> > > -	th = (struct tcphdr *)(skb_network_header(skb) + ip_hdrlen(skb));
> > > +	th = skb_header_pointer(skb, ip_hdrlen(skb), sizeof(tcph), &tcph);
> > > +	if (!th)
> > > +		return;
> >
> > This fix is required since your recent upgrade to check for 4 bytes
> > instead of 8 bytes from conntrack. Please confirm this.
> 
> Do you mean the commit e5e693ab49a95e1994979972eea224eefa81eba9
> "netfilter: conntrack: Only need first 4 bytes to get l4proto ports"?
> Sorry, I could not figure out why it would break the rule, and cause
issue.
> 
> Let's look at one change for TCP of that commit as following
> 
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c
> b/net/netfilter/nf_conntrack_proto_tcp.c
> index 70c8381..4abe9e1 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -282,8 +282,8 @@ static bool tcp_pkt_to_tuple(const struct sk_buff
*skb,
> unsigned int dataoff,
>         const struct tcphdr *hp;
>         struct tcphdr _hdr;
> 
> -       /* Actually only need first 8 bytes. */
> -       hp = skb_header_pointer(skb, dataoff, 8, &_hdr);
> +       /* Actually only need first 4 bytes to get ports. */
> +       hp = skb_header_pointer(skb, dataoff, 4, &_hdr);
>         if (hp == NULL)
>                 return false;
> 
> The skb_header_pointer only copies the data, not make the data linear.
> So I think it is ok after 4 bytes instead of 8 bytes.
> It only affect the local variable, not the skb.
> 
> >
> > If so, it would be good to review the NAT code to see if there are
> > more spots that are not broken since that change...
> 
> I also check the other codes of Netfilter.
> 
> grep -in skb_network_header * in nf/net/netfilter and
nf/net/ipv4(6)/netfilter/
> nf_nat_helper.c:41:     data = skb_network_header(skb) + dataoff;
> nf_tables_core.c:99:            ptr = skb_network_header(skb) +
> pkt->xt.thoff;
> xt_TCPMSS.c:104:        tcph = (struct tcphdr *)(skb_network_header(skb) +
> tcphoff);
> xt_TCPMSS.c:163:                tcph = (struct tcphdr
> *)(skb_network_header(skb) + tcphoff);
> xt_TCPOPTSTRIP.c:54:    tcph = (struct tcphdr *)(skb_network_header(skb) +
> tcphoff);
> arpt_mangle.c:23:       arpptr = skb_network_header(skb) + sizeof(*arp);
> 
> all of them make sure the skb is linear, except the nf_tables_core.c which
use
> the skb_tail_pointer to exclude the non-linear data.
> 
> So there is only nf_ct_tcp_seqadj_set which need fixed.
> 
> Best Regards
> Feng

Sorry, please ignore this email. 
I reply the conversation with wrong email account which Outlook select it by
default.
I have replied again with the right email account.

Best Regards
Feng




--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Netfitler Users]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux