Re: [PATCH] tcp: prevent loss of bytes when using syncookie

On Tue, Apr 20, 2021 at 2:00 PM zhaoya [off-list ref] 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 )

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.

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.