Re: [PATCH nf-next] netfilter: conntrack: tcp: use UNACK timeout for non-closing RST packets

[Florian Westphal <fw@strlen.de>]

Minghao Zhang <zhangmh25@mails.tsinghua.edu.cn> wrote:
> Commit be0502a3f2e9 ("netfilter: conntrack: tcp: only close if RST
> matches exact sequence") keeps an established conntrack entry in
> ESTABLISHED when an in-window RST does not match the expected sequence
> number exactly, so the endpoint can validate the RST with a challenge
> ACK.
> 
> The timeout selection nevertheless uses the CLOSE timeout for every RST
> packet.  The bug is that timeout selection is based on the packet type,
> not on the state transition result: even when RST validation keeps
> new_state in ESTABLISHED, the timeout is still forced to
> TCP_CONNTRACK_CLOSE.
> 
> Linux TCP independently rate limits challenge ACKs per socket.  A second
> non-exact RST can therefore arrive after the first challenge ACK has
> restored the timeout but before the rate limit expires.  The second RST
> lowers the timeout to 10 seconds again while the endpoint suppresses the
> second challenge ACK, allowing the conntrack entry to expire while both
> TCP endpoints remain established.
> 
> Using the ESTABLISHED timeout for non-exact RSTs would avoid this short
> expiration window, but it could also retain stale entries for the
> five-day default because conntrack cannot reliably match the endpoint's
> exact TCP state.
> 
> Use the UNACK timeout for RST packets that do not move the conntrack
> entry to TCP_CONNTRACK_CLOSE.  Exact-match RSTs and accepted RST packet
> trains still use the CLOSE timeout because their state transition result
> is CLOSE.  This avoids the exploitable 10-second expiration window for
> non-exact RSTs while preserving the short timeout for RSTs that
> conntrack accepts as closing the flow.

Hmm.

>  		timeout = timeouts[TCP_CONNTRACK_RETRANS];
> -	else if (unlikely(index == TCP_RST_SET))
> +	else if (unlikely(index == TCP_RST_SET &&
> +			  new_state == TCP_CONNTRACK_CLOSE))
>  		timeout = timeouts[TCP_CONNTRACK_CLOSE];

Is this needed?  If new state is TCP_CONNTRACK_CLOSE, the existing
else branch will:
        else
                timeout = timeouts[new_state];

> +	else if (unlikely(index == TCP_RST_SET))
> +		timeout = timeouts[TCP_CONNTRACK_UNACK];

AFAICS this is enough:
-       else if (unlikely(index == TCP_RST_SET))
-               timeout = timeouts[TCP_CONNTRACK_CLOSE];
+       else if (unlikely(index == TCP_RST_SET && new_state != TCP_CONNTRACK_ESTABLISHED))
+               timeout = timeouts[TCP_CONNTRACK_UNACK];

Could you test this?  Thanks!