[PATCH nf v2] netfilter: synproxy: fix rst sequence number mismatch

[PATCH nf v2] netfilter: synproxy: fix rst sequence number mismatch

[Fernando Fernandez Mancera]

14:51:00.024418 IP 192.168.122.1.41462 > netfilter.90: Flags [S], seq
4023580551, win 64240, options [mss 1460,sackOK,TS val 2149563785 ecr
0,nop,wscale 7], length 0
14:51:00.024454 IP netfilter.90 > 192.168.122.1.41462: Flags [S.], seq
727560212, ack 4023580552, win 0, options [mss 1460,sackOK,TS val 355031 ecr
2149563785,nop,wscale 7], length 0
14:51:00.024524 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack 1, win
502, options [nop,nop,TS val 2149563785 ecr 355031], length 0
14:51:00.024550 IP netfilter.90 > 192.168.122.1.41462: Flags [R.], seq
3567407084, ack 1, win 0, length 0
14:51:00.231196 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack 1, win
502, options [nop,nop,TS val 2149563992 ecr 355031], length 0
14:51:00.647911 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack 1, win
502, options [nop,nop,TS val 2149564408 ecr 355031], length 0
14:51:01.474395 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack 1, win
502, options [nop,nop,TS val 2149565235 ecr 355031], length 0

Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target")
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
 net/netfilter/nf_synproxy_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
index 6676a3842a0c..b0930d4aba22 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -687,7 +687,7 @@ ipv4_synproxy_hook(void *priv, struct sk_buff *skb,
 	state = &ct->proto.tcp;
 	switch (state->state) {
 	case TCP_CONNTRACK_CLOSE:
-		if (th->rst && !test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) {
+		if (th->rst && CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL) {
 			nf_ct_seqadj_init(ct, ctinfo, synproxy->isn -
 						      ntohl(th->seq) + 1);
 			break;
@@ -1111,7 +1111,7 @@ ipv6_synproxy_hook(void *priv, struct sk_buff *skb,
 	state = &ct->proto.tcp;
 	switch (state->state) {
 	case TCP_CONNTRACK_CLOSE:
-		if (th->rst && !test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) {
+		if (th->rst && CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL) {
 			nf_ct_seqadj_init(ct, ctinfo, synproxy->isn -
 						      ntohl(th->seq) + 1);
 			break;
-- 
2.20.1

Re: [PATCH nf v2] netfilter: synproxy: fix rst sequence number mismatch

[Florian Westphal]

Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:
> 14:51:00.024418 IP 192.168.122.1.41462 > netfilter.90: Flags [S], seq
> 4023580551, win 64240, options [mss 1460,sackOK,TS val 2149563785 ecr
> 0,nop,wscale 7], length 0

Could you please trim this down to the relevant parts
and add a more human-readable description as to where the problem is,
under which circumstances this happens and why the
!SEEN_REPLY_BIT test is bogus?

Keep in mind that you know more about synproxy than I do, so its
harder for me to follow what you're doing when the commit message consists
of tcpdump output.

> 14:51:00.024454 IP netfilter.90 > 192.168.122.1.41462: Flags [S.], seq
> 727560212, ack 4023580552, win 0, options [mss 1460,sackOK,TS val 355031 ecr
> 2149563785,nop,wscale 7], length 0
> 14:51:00.024524 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack 1, win
> 502, options [nop,nop,TS val 2149563785 ecr 355031], length 0
> 14:51:00.024550 IP netfilter.90 > 192.168.122.1.41462: Flags [R.], seq
> 3567407084, ack 1, win 0, length 0

... its not obvious to me why a reset is generated here in first place,
and why changing code in TCP_CLOSE case helps?
(I could guess the hook was called in postrouting and close transition
 came from rst that was sent, but that still doesn't explain why it
 was sent to begin with).

I assume the hostname "netfilter" is the synproxy machine, and
192.168.122.1 is a client we're proxying for, right?

Re: [PATCH nf v2] netfilter: synproxy: fix rst sequence number mismatch

[Fernando Fernandez Mancera]

El 14 de julio de 2019 0:26:24 CEST, Florian Westphal <fw@strlen.de> escribió:
>Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:
>> 14:51:00.024418 IP 192.168.122.1.41462 > netfilter.90: Flags [S], seq
>> 4023580551, win 64240, options [mss 1460,sackOK,TS val 2149563785 ecr
>> 0,nop,wscale 7], length 0
>
>Could you please trim this down to the relevant parts
>and add a more human-readable description as to where the problem is,
>under which circumstances this happens and why the
>!SEEN_REPLY_BIT test is bogus?
>
>Keep in mind that you know more about synproxy than I do, so its
>harder for me to follow what you're doing when the commit message
>consists
>of tcpdump output.
>
>> 14:51:00.024454 IP netfilter.90 > 192.168.122.1.41462: Flags [S.],
>seq
>> 727560212, ack 4023580552, win 0, options [mss 1460,sackOK,TS val
>355031 ecr
>> 2149563785,nop,wscale 7], length 0
>> 14:51:00.024524 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack
>1, win
>> 502, options [nop,nop,TS val 2149563785 ecr 355031], length 0
>> 14:51:00.024550 IP netfilter.90 > 192.168.122.1.41462: Flags [R.],
>seq
>> 3567407084, ack 1, win 0, length 0
>
>... its not obvious to me why a reset is generated here in first place,
>and why changing code in TCP_CLOSE case helps?
>(I could guess the hook was called in postrouting and close transition
> came from rst that was sent, but that still doesn't explain why it
> was sent to begin with).
>
>I assume the hostname "netfilter" is the synproxy machine, and
>192.168.122.1 is a client we're proxying for, right?

Sure, I will prepare a detailed description of the problem. Sorry about that and thanks!

Re: [PATCH nf v2] netfilter: synproxy: fix rst sequence number mismatch

[Fernando Fernandez Mancera]

Hi Florian,

On 7/14/19 1:25 AM, Fernando Fernandez Mancera wrote:
> El 14 de julio de 2019 0:26:24 CEST, Florian Westphal <fw@strlen.de> escribió:
>> Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:
>>> 14:51:00.024418 IP 192.168.122.1.41462 > netfilter.90: Flags [S], seq
>>> 4023580551, win 64240, options [mss 1460,sackOK,TS val 2149563785 ecr
>>> 0,nop,wscale 7], length 0
>>
>> Could you please trim this down to the relevant parts
>> and add a more human-readable description as to where the problem is,
>> under which circumstances this happens and why the
>> !SEEN_REPLY_BIT test is bogus?
>>
>> Keep in mind that you know more about synproxy than I do, so its
>> harder for me to follow what you're doing when the commit message
>> consists
>> of tcpdump output.
>>
>>> 14:51:00.024454 IP netfilter.90 > 192.168.122.1.41462: Flags [S.],
>> seq
>>> 727560212, ack 4023580552, win 0, options [mss 1460,sackOK,TS val
>> 355031 ecr
>>> 2149563785,nop,wscale 7], length 0
>>> 14:51:00.024524 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack
>> 1, win
>>> 502, options [nop,nop,TS val 2149563785 ecr 355031], length 0
>>> 14:51:00.024550 IP netfilter.90 > 192.168.122.1.41462: Flags [R.],
>> seq
>>> 3567407084, ack 1, win 0, length 0
>>
>> ... its not obvious to me why a reset is generated here in first place,
>> and why changing code in TCP_CLOSE case helps?
>> (I could guess the hook was called in postrouting and close transition
>> came from rst that was sent, but that still doesn't explain why it
>> was sent to begin with).
>>
>> I assume the hostname "netfilter" is the synproxy machine, and
>> 192.168.122.1 is a client we're proxying for, right?
> 
> Sure, I will prepare a detailed description of the problem. Sorry about that and thanks!
> 

When there is no service listening in the specified port in the backend,
we get a reset packet from the backend that is sent to the client but
the sequence number mismatches the tcp stream one so there is a loop in
which the client is requesting it until the timeout.

To solve this we need to adjust the sequence number, we cannot use the
!SEEN_REPLY_BIT test because it is always false at this point and then
we never get into the if statement. Instead of check the !SEEN_REPLY_BIT
we need to check if the CT IP address is different to the original CT IP.

I hope that answers your questions, here is the tcpdump output with only
the important information. Please note that "netfilter" is the synproxy
machine and 192.168.122.1 is the client. If that is fine to you, I will
include this description into the commit message and send a v3 patch.
Thanks Florian! :-)

TCPDUMP output:

14:51:00.024418 IP 192.168.122.1.41462 > netfilter.90: Flags [S], seq
4023580551,
14:51:00.024454 IP netfilter.90 > 192.168.122.1.41462: Flags [S.], seq
727560212, ack 4023580552,
14:51:00.024524 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack 1,
14:51:00.024550 IP netfilter.90 > 192.168.122.1.41462: Flags [R.], seq
3567407084,
14:51:00.231196 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack 1,
14:51:00.647911 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack 1,
14:51:01.474395 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack 1,

Re: [PATCH nf v2] netfilter: synproxy: fix rst sequence number mismatch

[Florian Westphal]

Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:
> >> ... its not obvious to me why a reset is generated here in first place,
> >> and why changing code in TCP_CLOSE case helps?
> >> (I could guess the hook was called in postrouting and close transition
> >> came from rst that was sent, but that still doesn't explain why it
> >> was sent to begin with).
> >>
> >> I assume the hostname "netfilter" is the synproxy machine, and
> >> 192.168.122.1 is a client we're proxying for, right?
> > 
> > Sure, I will prepare a detailed description of the problem. Sorry about that and thanks!
> > 
> 
> When there is no service listening in the specified port in the backend,
> we get a reset packet from the backend that is sent to the client but
> the sequence number mismatches the tcp stream one so there is a loop in
> which the client is requesting it until the timeout.

Oh, no listener on the backend -- now this tcpdump makes sense :-)

I wondered where synproxy would generate a reset.  It doesn't.  Doh.

> To solve this we need to adjust the sequence number, we cannot use the
> !SEEN_REPLY_BIT test because it is always false at this point and then

Right, that part now makes sense too, we only sent the syn to the
backend, so we have not seen a reply yet.  The reset switches the
conntrack to CLOSED immediately, so that part now makes sense to me too.

> we never get into the if statement. Instead of check the !SEEN_REPLY_BIT
> we need to check if the CT IP address is different to the original CT IP.

Yes indeed.

> I hope that answers your questions, here is the tcpdump output with only
> the important information. Please note that "netfilter" is the synproxy
> machine and 192.168.122.1 is the client. If that is fine to you, I will
> include this description into the commit message and send a v3 patch.
> Thanks Florian! :-)

Yes, perhaps also add a small comment to the tcpdump that the reset
is not coming from the synproxy machine but is originally received from
client.

Perhaps something like this:
> TCPDUMP output:
> 
> 14:51:00.024418 IP 192.168.122.1.41462 > netfilter.90: Flags [S], seq
> 4023580551,
> 14:51:00.024454 IP netfilter.90 > 192.168.122.1.41462: Flags [S.], seq
> 727560212, ack 4023580552,
> 14:51:00.024524 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack 1,
# here, SYNPROXY will send a SYN to the real server, as the 3whs was
# completed sucessfully.
# instead of a syn/ack that we can intercept, we instead received a
# reset packet from the real backend, that we forward to the original
# client.  However, we don't use the correct sequence number, so
# the reset is not effective in closing the connection coming from
# the client.
> 14:51:01.474395 IP 192.168.122.1.41462 > netfilter.90: Flags [.], ack 1,

Client then retries until timeout.


Thats just a suggestion so that its more obvious that we see only
one leg of the connection in the tcpdump.

This explanation makes sense, thanks a lot for elaborating on this.