net: tcp: Zero Window Probes are not supported/handled properly

[pfalcon]

TCP receive window handling implementation was discussed in #3440.

The initial implementation proposed to perform receive window handling on the level of IP stack (i.e. by exposing via native API): #81 . But there was resistance to it, so instead a generic hooks were added on the level on native API (so its behavior didn't change), and the actual receive window handling was implemented on the level of BSD Sockets shim: #1002 .

There were 2 requirements for this initial implementation:

1. Make sure that receive window handling is performed at all, so non-toy TCP communication can happen.
2. Disallow bypassing receive window checks as means of a DoS influence (see e.g. [WIP] net: tcp: Explicitly manage TCP receive window. #81 (comment)).

[pfalcon]

@rlubos: FYI

[pfalcon]

2. Disallow bypassing receive window checks as means of a DoS influence (see e.g. [WIP] net: tcp: Explicitly manage TCP receive window. #81 (comment)).

So, the intended behavior of achieving this was: if a peer sends us more data than we advertised with our receive window, we just NET_DROP that entire packet. (The idea being: we asked not to send us too much data. If the peer still sends us too much data, that can be only for bad motives, and we'll resist attempts to pull us into packet amplification.)

However, that completely overlooked the concept of Zero Window Probes: if we communicated a window=0 to the peer, and don't communicate anything else for a while, the TCP allows peer to proactively send us "Zero Window Probe", and we expected to reply to it - by sending ACK, where we either confirm that window is still 0, or well, that it's no longer.

"Zero Window Probe" is actually a fully normal TCP packet (segment). Per TCP 793 3.7 page 42:

The sending TCP must be prepared to accept from the user and send at
least one octet of new data even if the send window is zero.

However, in real world, IP stacks add some constraints on ZWPs. For example, Linux appears to send exactly one octet in them.

[pfalcon]

With this info, we now have a bugreport which shows (by the means of Wireshark capture) that the actual behavior is not as described above: #7818

After receiving ZWP (which is again, just a normal TCP segment, just sent to a peer with window=0), Zephyr instead of silently dropping it, sends an ACK, with an updated ack sequence, which Wireshark notes as "TCP ACKed unseen segment". That apparently quite confuses the peer, that it stops further ZWPs and the TCP communication stalls.

That was speculative part of explaining the behavior based on external symptoms, next step is to confirm this behavior by the code review.

[pfalcon]

Related RFC: https://tools.ietf.org/html/rfc1122#page-92

[pfalcon]

So, the check is still there: https://github.com/zephyrproject-rtos/zephyr/blob/master/subsys/net/ip/tcp.c#L2046 , it results in error message (should be enabled by default) and NET_DROP. Sending an ack happens only after it: https://github.com/zephyrproject-rtos/zephyr/blob/master/subsys/net/ip/tcp.c#L2069 .

So, it's unclear what happens here.

[pfalcon]

After the last comment, I switched to the practical part of trying to reproduce Zero Window Probes issues, logged here: #7818 (comment)

[pfalcon]

Cross-posting from #7818 (comment) :

As a final test, I'm applying following test changes:

--- a/samples/net/sockets/big_http_download/src/big_http_download.c
+++ b/samples/net/sockets/big_http_download/src/big_http_download.c
@@ -29,7 +29,7 @@
 
 /* This URL is parsed in-place, so buffer must be non-const. */
 static char download_url[] =
-    "http://archive.ubuntu.com/ubuntu/dists/xenial/main/installer-amd64/current/images/hd-media/vmlinuz";
+    "http://microsoft.com/";
 /* Quick testing. */
 /*    "http://google.com/foo";*/
 
diff --git a/subsys/net/ip/tcp.c b/subsys/net/ip/tcp.c
index fec12c550..c39205875 100644
--- a/subsys/net/ip/tcp.c
+++ b/subsys/net/ip/tcp.c
@@ -274,7 +274,7 @@ struct net_tcp *net_tcp_alloc(struct net_context *context)
 	tcp_context[i].context = context;
 
 	tcp_context[i].send_seq = tcp_init_isn();
-	tcp_context[i].recv_wnd = min(NET_TCP_MAX_WIN, NET_TCP_BUF_MAX_LEN);
+	tcp_context[i].recv_wnd = 40; //min(NET_TCP_MAX_WIN, NET_TCP_BUF_MAX_LEN);
 	tcp_context[i].send_mss = NET_TCP_DEFAULT_MSS;
 
 	tcp_context[i].accept_cb = NULL;
diff --git a/subsys/net/lib/sockets/sockets.c b/subsys/net/lib/sockets/sockets.c
index 5bcd771f5..5f672245e 100644
--- a/subsys/net/lib/sockets/sockets.c
+++ b/subsys/net/lib/sockets/sockets.c
@@ -443,7 +443,7 @@ static inline ssize_t zsock_recv_stream(struct net_context *ctx,
 	} while (recv_len == 0);
 
 	if (!(flags & ZSOCK_MSG_PEEK)) {
-		net_context_update_recv_wnd(ctx, recv_len);
+//		net_context_update_recv_wnd(ctx, recv_len);
 	}
 
 	return recv_len;

With it, I get the following console output:

***** Booting Zephyr OS v1.11.0-1619-g5b20350a7 *****
[net/app] [INF] net_app_init: Initializing network
[net/app] [INF] setup_dhcpv4: Running dhcpv4 client...
Preparing HTTP GET request for http://microsoft.com:80/
addrinfo @0x401040: ai_family=2, ai_socktype=1, ai_protocol=6, sa_family=2, sin_port=5000
sock = 4199520
[net/tcp] [ERR] _tcp_established: Context 0x00401460: overflow of recv window (0 vs 1), pkt dropped
[net/tcp] [ERR] _tcp_established: Context 0x00401460: overflow of recv window (0 vs 1), pkt dropped

in full accordance with the analysis described in #7876.

So, that's the only stable and reproducible issue on our side that I see, and which needs to be fixed. (The next issue on our side is "TCP handling is poor", but unfortunately one can't submit a few lines patch and have that fixed).

[ssharks]

@rlubos: Isn't this already fixed by eb0364d