On Tue, Apr 20, 2021 at 2:00 PM zhaoya <zhaoya.ga...@bytedance.com> wrote:
>
> When using syncookie, since $MSSID is spliced into cookie and
> the legal index of msstab is 0,1,2,3, this gives client 3 bytes
> of freedom, resulting in at most 3 bytes of silent loss.
>
> C ------------seq=12345-------------> S
> C <------seq=cookie/ack=12346-------- S S generated the cookie
> [RFC4987 Appendix A]
> C ---seq=123456/ack=cookie+1-->X S The first byte was loss.
> C -----seq=123457/ack=cookie+1------> S The second byte was received and
> cookie-check was still okay and
> handshake was finished.
> C <--------seq=.../ack=12348--------- S acknowledge the second byte.
>
> Here is a POC:
>
> $ cat poc.c
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> #include <pthread.h>
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <netinet/in.h>
> #include <arpa/inet.h>
>
> void *serverfunc(void *arg)
> {
> int sd = -1;
> int csd = -1;
> struct sockaddr_in servaddr, cliaddr;
> int len = sizeof(cliaddr);
>
> sd = socket(AF_INET, SOCK_STREAM, 0);
> servaddr.sin_family = AF_INET;
> servaddr.sin_addr.s_addr = htonl(INADDR_ANY);
> servaddr.sin_port = htons(1234);
> bind(sd, (struct sockaddr *)&servaddr, sizeof(servaddr));
> listen(sd, 1);
>
> while (1) {
> char buf[2];
> int ret;
> csd = accept(sd, (struct sockaddr *)&cliaddr, &len);
> memset(buf, 0, 2);
> ret = recv(csd, buf, 1, 0);
> // but unexpected char is 'b'
> if (ret && strncmp(buf, "a", 1)) {
> printf("unexpected:%s\n", buf);
> close(csd);
> exit(0);
> }
> close(csd);
> }
> }
>
> void *connectfunc(void *arg)
> {
> struct sockaddr_in addr;
> int sd;
> int i;
>
> for (i = 0; i < 500; i++) {
> sd = socket(AF_INET, SOCK_STREAM, 0);
> addr.sin_family = AF_INET;
> addr.sin_addr.s_addr = inet_addr("127.0.0.1");
> addr.sin_port = htons(1234);
>
> connect(sd, (struct sockaddr *)&addr, sizeof(addr));
>
> send(sd, "a", 1, 0); // expected char is 'a'
> send(sd, "b", 1, 0);
> close(sd);
> }
> return NULL;
> }
>
> int main(int argc, char *argv[])
> {
> int i;
> pthread_t id;
>
> pthread_create(&id, NULL, serverfunc, NULL);
> sleep(1);
> for (i = 0; i < 500; i++) {
> pthread_create(&id, NULL, connectfunc, NULL);
> }
> sleep(5);
> }
>
> $ sudo gcc poc.c -lpthread
> $ sudo sysctl -w net.ipv4.tcp_syncookies=1
> $ sudo ./a.out # please try as many times.
>
POC is distracting, you could instead give a link to
https://kognitio.com/blog/syn-cookies-ate-my-dog-breaking-tcp-on-linux/
where all the details can be found.
Also it gives credits to past work.
If you feel a program needs to be written and recorded for posterity,
add a selftest. (in tools/testing/selftests/net )
> This problem can be fixed by having $SSEQ itself participate in the
> hash operation.
>
> The feature is protected by a sysctl so that admins can choose
> the preferred behavior.
>
> Signed-off-by: zhaoya <zhaoya.ga...@bytedance.com>
> ---
> Documentation/networking/ip-sysctl.rst | 9 ++++++++
> include/linux/netfilter_ipv6.h | 24 +++++++++++++-------
> include/net/netns/ipv4.h | 1 +
> include/net/tcp.h | 20 ++++++++++-------
> net/core/filter.c | 6 +++--
> net/ipv4/syncookies.c | 31 ++++++++++++++++----------
> net/ipv4/sysctl_net_ipv4.c | 7 ++++++
> net/ipv4/tcp_ipv4.c | 4 +++-
> net/ipv6/syncookies.c | 29 +++++++++++++++---------
> net/ipv6/tcp_ipv6.c | 3 ++-
> net/netfilter/nf_synproxy_core.c | 10 +++++----
> 11 files changed, 97 insertions(+), 47 deletions(-)
>
> diff --git a/Documentation/networking/ip-sysctl.rst
> b/Documentation/networking/ip-sysctl.rst
> index c7952ac5bd2f..a430e8736c2b 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -732,6 +732,15 @@ tcp_syncookies - INTEGER
> network connections you can set this knob to 2 to enable
> unconditionally generation of syncookies.
>
> +tcp_syncookies_enorder - INTEGER
enorder ? What does it mean ?
> + Only valid when the kernel was compiled with CONFIG_SYN_COOKIES
> + Prevent the loss of at most 3 bytes of which sent by client when
> + syncookies as generated to complete TCP-Handshake.
> + Default: 0
> +
> + Note that if it was enabled, any out-of-order bytes sent by client
> + to complete TCP-Handshake could get the session killed.
Technically speaking, the client does not send out-of-order packets.
The issue here is that loss of packets sent after 3WHS can lead to RST
and session being killed.
> +
> tcp_fastopen - INTEGER
> Enable TCP Fast Open (RFC7413) to send and accept data in the opening
> SYN packet.
> diff --git a/include/linux/netfilter_ipv6.h b/include/linux/netfilter_ipv6.h
> index 48314ade1506..97b67672dee7 100644
> --- a/include/linux/netfilter_ipv6.h
> +++ b/include/linux/netfilter_ipv6.h
> @@ -49,9 +49,11 @@ struct nf_ipv6_ops {
> int (*route)(struct net *net, struct dst_entry **dst, struct flowi
> *fl,
> bool strict);
> u32 (*cookie_init_sequence)(const struct ipv6hdr *iph,
> - const struct tcphdr *th, u16 *mssp);
> + const struct tcphdr *th, u16 *mssp,
> + int enorder);
>
Patch is too big, and passing either a ' struct net' or an ' int
enorder' is not pretty/consistent.
Please send a patch series :
1) Add ' struct net' pointers to all helpers which will need it
later. This is a pure preparation work, easy to review.
2) Patch itself, adding the sysctl.
I would prefer you add a helper like this and use it every where
you need to use the sysctl so that it is factorized
static inline u32 tcp_cond_seq(const struct net *net, u32 seq)
{
return net->ipv4.sysctl_tcp_syncookie_no_eat ? seq : 0;
}
And use this helper in functions right before using (or not) the seq
in hash functions/
(Do not pass around the result of the helper)
Thanks.
