| 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
| ||
|
Message-ID: <CAL+tcoCVS-thL79WjN5mSxnUFNVrp1bB+JF9MUcsGuodcETPFg@mail.gmail.com>
Date: Thu, 7 Nov 2024 15:44:55 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com,
horms@...nel.org, kernelxing@...cent.com, kuba@...nel.org,
netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next] tcp: avoid RST in 3-way shakehands due to
failure in tcp_timewait_state_process
On Thu, Nov 7, 2024 at 3:11 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Jason Xing <kerneljasonxing@...il.com>
> Date: Thu, 7 Nov 2024 14:51:35 +0800
> > On Thu, Nov 7, 2024 at 1:43 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> > >
> > > From: Jason Xing <kerneljasonxing@...il.com>
> > > Date: Thu, 7 Nov 2024 13:23:50 +0800
> > > > On Thu, Nov 7, 2024 at 12:15 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> > > > >
> > > > > From: Jason Xing <kerneljasonxing@...il.com>
> > > > > Date: Thu, 7 Nov 2024 11:16:04 +0800
> > > > > > > Here is how things happen in production:
> > > > > > > Time Client(A) Server(B)
> > > > > > > 0s SYN-->
> > > > > > > ...
> > > > > > > 132s <-- FIN
> > > > > > > ...
> > > > > > > 169s FIN-->
> > > > > > > 169s <-- ACK
> > > > > > > 169s SYN-->
> > > > > > > 169s <-- ACK
> > > > > >
> > > > > > I noticed the above ACK doesn't adhere to RFC 6191. It says:
> > > > > > "If the previous incarnation of the connection used Timestamps, then:
> > > > > > if ...
> > > > > > ...
> > > > > > * Otherwise, silently drop the incoming SYN segment, thus leaving
> > > > > > the previous incarnation of the connection in the TIME-WAIT
> > > > > > state.
> > > > > > "
> > > > > > But the timewait socket sends an ACK because of this code snippet:
> > > > > > tcp_timewait_state_process()
> > > > > > -> // the checks of SYN packet failed.
> > > > > > -> if (!th->rst) {
> > > > > > -> return TCP_TW_ACK; // this line can be traced back to 2005
> > > > >
> > > > > This is a challenge ACK following RFC 5961.
> > > >
> > > > Please note the idea of challenge ack was proposed in 2010. But this
> > > > code snippet has already existed before 2005. If it is a challenge
> > > > ack, then at least we need to count it (by using NET_INC_STATS(net,
> > > > LINUX_MIB_TCPCHALLENGEACK);).
> > >
> > > The word was not accurate, the behaviour is compliant with RFC 5961.
> > > RFC is often formalised based on real implementations.
> > >
> > > Incrementing the count makes sense to me.
> > >
> > > >
> > > > >
> > > > > If SYN is returned here, the client may lose the chance to RST the
> > > > > previous connection in TIME_WAIT.
> > > > >
> > > > > https://www.rfc-editor.org/rfc/rfc9293.html#section-3.10.7.4-2.4.1
> > > > > ---8<---
> > > > > - TIME-WAIT STATE
> > > > >
> > > > > o If the SYN bit is set in these synchronized states, it may
> > > > > be either a legitimate new connection attempt (e.g., in the
> > > > > case of TIME-WAIT), an error where the connection should be
> > > > > reset, or the result of an attack attempt, as described in
> > > > > RFC 5961 [9]. For the TIME-WAIT state, new connections can
> > > > > be accepted if the Timestamp Option is used and meets
> > > > > expectations (per [40]). For all other cases, RFC 5961
> > > > > provides a mitigation with applicability to some situations,
> > > > > though there are also alternatives that offer cryptographic
> > > > > protection (see Section 7). RFC 5961 recommends that in
> > > > > these synchronized states, if the SYN bit is set,
> > > > > irrespective of the sequence number, TCP endpoints MUST send
> > > > > a "challenge ACK" to the remote peer:
> > > > >
> > > > > <SEQ=SND.NXT><ACK=RCV.NXT><CTL=ACK>
> > > > > ---8<---
> > > > >
> > > > > https://datatracker.ietf.org/doc/html/rfc5961#section-4
> > > > > ---8<---
> > > > > 1) If the SYN bit is set, irrespective of the sequence number, TCP
> > > > > MUST send an ACK (also referred to as challenge ACK) to the remote
> > > > > peer:
> > > > >
> > > > > <SEQ=SND.NXT><ACK=RCV.NXT><CTL=ACK>
> > > > >
> > > > > After sending the acknowledgment, TCP MUST drop the unacceptable
> > > > > segment and stop processing further.
> > > > > ---8<---
> > > >
> > > > The RFC 5961 4.2 was implemented in tcp_validate_incoming():
> > > > /* step 4: Check for a SYN
> > > > * RFC 5961 4.2 : Send a challenge ack
> > > > */
> > > > if (th->syn) {
> > > > if (sk->sk_state == TCP_SYN_RECV && sk->sk_socket && th->ack &&
> > > > TCP_SKB_CB(skb)->seq + 1 == TCP_SKB_CB(skb)->end_seq &&
> > > > TCP_SKB_CB(skb)->seq + 1 == tp->rcv_nxt &&
> > > > TCP_SKB_CB(skb)->ack_seq == tp->snd_nxt)
> > > > goto pass;
> > > > syn_challenge:
> > > > if (syn_inerr)
> > > > TCP_INC_STATS(sock_net(sk), TCP_MIB_INERRS);
> > > > NET_INC_STATS(sock_net(sk),
> > > > LINUX_MIB_TCPSYNCHALLENGE);
> > > > tcp_send_challenge_ack(sk);
> > > > SKB_DR_SET(reason, TCP_INVALID_SYN);
> > > > goto discard;
> > > > }
> > > >
> > > > Also, this quotation you mentioned obviously doesn't match the kernel
> > > > implementation:
> > > > "If the SYN bit is set, irrespective of the sequence number, TCP MUST
> > > > send an ACK"
> > > > The tcp_timewait_state_process() does care about the seq number, or
> > > > else timewait socket would refuse every SYN packet.
> > >
> > > That's why I pasted RFC 9293 first that clearly states that we
> > > should check seq number and then return ACK for all other cases.
> >
> > I don't think so.
> >
> > RFC 9293 only states that RFC 5691 provides an approach that mitigates
> > the risk by rejecting all the SYN packets if the socket stays in
> > synchronized state. It's "For all other cases" in RFC 9293.
>
> RFC 9293 states which RFC to prioritise. You will find the
> link [40] is RFC 6191.
>
> ---8<---
> For the TIME-WAIT state, new connections can
> be accepted if the Timestamp Option is used and meets
> expectations (per [40]). For all other cases, RFC 5961
> ...
> ---8<---
>
> > Please loot at "irrespective of the sequence number" in RFC 5691 4.2
> > [1]. It means no matter what the seq is we MUST send back an ACK
> > instead of establishing a new connection.
>
> RFC 9293 mentions accepatble cases first, so this is only applied
> to "all other cases"
>
>
> > Actually the tcp_timewait_state_process() checks the seq or timestamp
> > in the SYN packet.
>
> and this part takes precedence than "all other cases".
>
> Also, you missed that the pasted part is the 4th step of incoming
> segment processing.
>
> https://www.rfc-editor.org/rfc/rfc9293.html#section-3.10.7.4
> ---8<---
> First, check sequence number: ...
> Second, check the RST bit: ...
> Third, check security: ...
> Fourth, check the SYN bit:
> ...
> TIME-WAIT STATE
> If the SYN bit is set in these synchronized states...
> ---8<---
>
> So, RFC 9293 says "check seq number, RST, security, then
> if the connection is still accepatable for TIME_WAIT based on
> RFC 6191, accept it, otherwise, return ACK based on RFC 5691".
I see what you mean here. If that is so, tcp_timewait_state_process()
has already implemented the challenge ack even before the challenge
ack showed up in 2010.
Interesting. If this part refers to RFC 5691, then we need to copy
part of tcp_send_challenge_ack() in this case to send the challenge
ack, like testing sysctl_tcp_challenge_ack_limit, etc.
Thanks,
Jason
Powered by blists - more mailing lists