Re: [RFC PATCH 1/2] landlock: TCP network hooks implementation

[Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx>]

> >   If bind() function has already been restricted so the following
> > listen() function is automatically banned. I agree with Mickaёl about
> > the usecase here. Why do we need new-bound socket with restricted listening?
>
> The intended use-case is for a privileged process to open a connection
> (i.e., bound and connected socket) and pass that to a restricted
> process. The intent is for that process to only be allowed to
> communicate over this pre-established channel.
>
> In practice, it is able to disconnect (while staying bound) and
> elevate its privileges to that of a listening server:
>
> static void child_process(int fd)
> {
>         struct sockaddr addr = { .sa_family = AF_UNSPEC };
>         int client_fd;
>
>         if (listen(fd, 1) == 0)
>                 error(1, 0, "listen succeeded while connected");
>
>         if (connect(fd, &addr, sizeof(addr)))
>                 error(1, errno, "disconnect");
>
>         if (listen(fd, 1))
>                 error(1, errno, "listen");
>
>         client_fd = accept(fd, NULL, NULL);
>         if (client_fd == -1)
>                 error(1, errno, "accept");
>
>         if (close(client_fd))
>                 error(1, errno, "close client");
> }
>
> int main(int argc, char **argv)
> {
>         struct sockaddr_in6 addr = { 0 };
>         pid_t pid;
>         int fd;
>
>         fd = socket(PF_INET6, SOCK_STREAM, 0);
>         if (fd == -1)
>                 error(1, errno, "socket");
>
>         addr.sin6_family = AF_INET6;
>         addr.sin6_addr = in6addr_loopback;
>
>         addr.sin6_port = htons(8001);
>         if (bind(fd, (void *)&addr, sizeof(addr)))
>                 error(1, errno, "bind");
>
>         addr.sin6_port = htons(8000);
>         if (connect(fd, (void *)&addr, sizeof(addr)))
>                 error(1, errno, "connect");
>
>         pid = fork();
>         if (pid == -1)
>                 error(1, errno, "fork");
>
>         if (pid)
>                 wait(NULL);
>         else
>                 child_process(fd);
>
>         if (close(fd))
>                 error(1, errno, "close");
>
>         return 0;
> }
>
> It's fine to not address this case in this patch series directly, of
> course. But we should be aware of the AF_UNSPEC loophole.

I did just notice that with autobind (i.e., without the explicit call
to bind), the socket gets a different local port after listen. So
internally a bind call may be made, which may or may not be correctly
handled by the current landlock implementation already:

+static void show_local_port(int fd)
+{
+       char addr_str[INET6_ADDRSTRLEN];
+       struct sockaddr_in6 addr = { 0 };
+       socklen_t alen;
+
+       alen = sizeof(addr);
+       if (getsockname(fd, (void *)&addr, &alen))
+               error(1, errno, "getsockname");
+
+       if (addr.sin6_family != AF_INET6)
+               error(1, 0, "getsockname: family");
+
+       if (!inet_ntop(AF_INET6, &addr.sin6_addr, addr_str, sizeof(addr_str)))
+               error(1, errno, "inet_ntop");
+       fprintf(stderr, "server: %s:%hu\n", addr_str, ntohs(addr.sin6_port));
+
+}
+
@@ -23,6 +42,8 @@ static void child_process(int fd)
        if (connect(fd, &addr, sizeof(addr)))
                error(1, errno, "disconnect");

+       show_local_port(fd);
+
        if (listen(fd, 1))
                error(1, errno, "listen");

+       show_local_port(fd);
+

@@ -47,10 +68,6 @@ int main(int argc, char **argv)
        addr.sin6_family = AF_INET6;
        addr.sin6_addr = in6addr_loopback;

-       addr.sin6_port = htons(8001);
-       if (bind(fd, (void *)&addr, sizeof(addr)))
-               error(1, errno, "bind");
-
        addr.sin6_port = htons(8000);
        if (connect(fd, (void *)&addr, sizeof(addr)))
                error(1, errno, "connect");