netdev - Re: [PATCH net-next] netfilter: conntrack: avoid sending RST to reply out-of-window skb

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: <CAL+tcoBqBaHxSU9NQqVxhRzzsaJr4=0=imtyCo4p8+DuXPL5AA@mail.gmail.com>
Date: Thu, 7 Mar 2024 21:33:11 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Florian Westphal <fw@...len.de>
Cc: edumazet@...gle.com, pablo@...filter.org, kadlec@...filter.org, 
	kuba@...nel.org, pabeni@...hat.com, davem@...emloft.net, 
	netfilter-devel@...r.kernel.org, coreteam@...filter.org, 
	netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next] netfilter: conntrack: avoid sending RST to reply
 out-of-window skb

On Thu, Mar 7, 2024 at 8:00 PM Florian Westphal <fw@...len.de> wrote:
>
> Jason Xing <kerneljasonxing@...il.com> wrote:
> > > This change disables most of the tcp_in_window() test, this will
> > > pretend everything is fine even though tcp_in_window says otherwise.
> >
> > Thanks for the information. It does make sense.
> >
> > What I've done is quite similar to nf_conntrack_tcp_be_liberal sysctl
> > knob which you also pointed out. It also pretends to ignore those
> > out-of-window skbs.
> >
> > >
> > > You could:
> > >  - drop invalid tcp packets in input hook
> >
> > How about changing the return value only as below? Only two cases will
> > be handled:
> >
> > diff --git a/net/netfilter/nf_conntrack_proto_tcp.c
> > b/net/netfilter/nf_conntrack_proto_tcp.c
> > index ae493599a3ef..c88ce4cd041e 100644
> > --- a/net/netfilter/nf_conntrack_proto_tcp.c
> > +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> > @@ -1259,7 +1259,7 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
> >         case NFCT_TCP_INVALID:
> >                 nf_tcp_handle_invalid(ct, dir, index, skb, state);
> >                 spin_unlock_bh(&ct->lock);
> > -               return -NF_ACCEPT;
> > +               return -NF_DROP;
>
> Lets not do this.  conntrack should never drop packets and defer to ruleset
> whereever possible.

Hmm, sorry, it is against my understanding.

If we cannot return -NF_DROP, why have we already added some 'return
NF_DROP' in the nf_conntrack_handle_packet() function? And why does
this test statement exist?

nf_conntrack_in()
  -> nf_conntrack_handle_packet()
  -> if (ret <= 0) {
         if (ret == -NF_DROP) NF_CT_STAT_INC_ATOMIC(state->net, drop);

>
> > >  - set nf_conntrack_tcp_be_liberal=1
> >
> > Sure, it can workaround this case, but I would like to refuse the
> > out-of-window in netfilter or TCP layer as default instead of turning
> > on this sysctl knob. If I understand wrong, please correct me.
>
> Thats contradictory, you make a patch to always accept, then another
> patch to always drop such packets?

My only purpose is not to let the TCP layer sending strange RST to the
right flow.

Besides, resorting to turning on nf_conntrack_tcp_be_liberal sysctl
knob seems odd to me though it can workaround :S

I would like to prevent sending such an RST as default behaviour.

I wonder why we have to send RST at last due to out-of-window skbs. It
should not happen, right? As I said before, It can be set as default
without relying on some sysctl knob.

Forgive my superficial knowledge :(

>
> You can get the drop behaviour via '-m conntrack --ctstate DROP' in
> prerouting or inut hooks.
>
> You can get the 'accept + do nat processing' via
> nf_conntrack_tcp_be_liberal=1.

Sure. Just turning on the sysctl knob can be helpful because I've
tested it in production. After all, it roughly returns NFCT_TCP_ACCEPT
in nf_tcp_log_invalid() without considering those various
out-of-window cases.

Thanks,
Jason