Message-ID: <CA+FuTScaoby-=xRKf_Dz3koSYHqrMN0cauCg4jMmy_nDxwPADA@mail.gmail.com>
Date:   Mon, 7 Feb 2022 11:00:34 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
Cc:     Mickaël Salaün <mic@...ikod.net>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
        netfilter@...r.kernel.org, yusongping@...wei.com,
        artem.kuzin@...wei.com
Subject: Re: [RFC PATCH 1/2] landlock: TCP network hooks implementation

On Mon, Feb 7, 2022 at 12:51 AM Konstantin Meskhidze
<konstantin.meskhidze@...wei.com> wrote:
>
>
>
> 2/1/2022 3:33 PM, Mickaël Salaün пишет:
> >
> > On 31/01/2022 18:14, Willem de Bruijn wrote:
> >> On Fri, Jan 28, 2022 at 10:12 PM Konstantin Meskhidze
> >> <konstantin.meskhidze@...wei.com> wrote:
> >>>
> >>>
> >>>
> >>> 1/26/2022 5:15 PM, Willem de Bruijn пишет:
> >>>> On Wed, Jan 26, 2022 at 3:06 AM Konstantin Meskhidze
> >>>> <konstantin.meskhidze@...wei.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> 1/25/2022 5:17 PM, Willem de Bruijn пишет:
> >>>>>> On Mon, Jan 24, 2022 at 3:02 AM Konstantin Meskhidze
> >>>>>> <konstantin.meskhidze@...wei.com> wrote:
> >>>>>>>
> >>>>>>> Support of socket_bind() and socket_connect() hooks.
> >>>>>>> Current prototype can restrict binding and connecting of TCP
> >>>>>>> types of sockets. Its just basic idea how Landlock could support
> >>>>>>> network confinement.
> >>>>>>>
> >>>>>>> Changes:
> >>>>>>> 1. Access masks array refactored into 1D one and changed
> >>>>>>> to 32 bits. Filesystem masks occupy 16 lower bits and network
> >>>>>>> masks reside in 16 upper bits.
> >>>>>>> 2. Refactor API functions in ruleset.c:
> >>>>>>>        1. Add void *object argument.
> >>>>>>>        2. Add u16 rule_type argument.
> >>>>>>> 3. Use two rb_trees in ruleset structure:
> >>>>>>>        1. root_inode - for filesystem objects
> >>>>>>>        2. root_net_port - for network port objects
> >>>>>>>
> >>>>>>> Signed-off-by: Konstantin Meskhidze
> >>>>>>> <konstantin.meskhidze@...wei.com>
> >>>>>>
> >>>>>>> +static int hook_socket_connect(struct socket *sock, struct
> >>>>>>> sockaddr *address, int addrlen)
> >>>>>>> +{
> >>>>>>> +       short socket_type;
> >>>>>>> +       struct sockaddr_in *sockaddr;
> >>>>>>> +       u16 port;
> >>>>>>> +       const struct landlock_ruleset *const dom =
> >>>>>>> landlock_get_current_domain();
> >>>>>>> +
> >>>>>>> +       /* Check if the hook is AF_INET* socket's action */
> >>>>>>> +       if ((address->sa_family != AF_INET) &&
> >>>>>>> (address->sa_family != AF_INET6))
> >>>>>>> +               return 0;
> >>>>>>
> >>>>>> Should this be a check on the socket family (sock->ops->family)
> >>>>>> instead of the address family?
> >>>>>
> >>>>> Actually connect() function checks address family:
> >>>>>
> >>>>> int __inet_stream_connect(... ,struct sockaddr *uaddr ,...) {
> >>>>> ...
> >>>>>           if (uaddr) {
> >>>>>                   if (addr_len < sizeof(uaddr->sa_family))
> >>>>>                   return -EINVAL;
> >>>>>
> >>>>>                   if (uaddr->sa_family == AF_UNSPEC) {
> >>>>>                           err = sk->sk_prot->disconnect(sk, flags);
> >>>>>                           sock->state = err ? SS_DISCONNECTING :
> >>>>>                           SS_UNCONNECTED;
> >>>>>                   goto out;
> >>>>>                   }
> >>>>>           }
> >>>>>
> >>>>> ...
> >>>>> }
> >>>>
> >>>> Right. My question is: is the intent of this feature to be limited to
> >>>> sockets of type AF_INET(6) or to addresses?
> >>>>
> >>>> I would think the first. Then you also want to catch operations on
> >>>> such sockets that may pass a different address family. AF_UNSPEC is
> >>>> the known offender that will effect a state change on AF_INET(6)
> >>>> sockets.
> >>>
> >>>    The intent is to restrict INET sockets to bind/connect to some ports.
> >>>    You can apply some number of Landlock rules with port defenition:
> >>>          1. Rule 1 allows to connect to sockets with port X.
> >>>          2. Rule 2 forbids to connect to socket with port Y.
> >>>          3. Rule 3 forbids to bind a socket to address with port Z.
> >>>
> >>>          and so on...
> >>>>
> >>>>>>
> >>>>>> It is valid to pass an address with AF_UNSPEC to a PF_INET(6) socket.
> >>>>>> And there are legitimate reasons to want to deny this. Such as
> >>>>>> passing
> >>>>>> a connection to a unprivileged process and disallow it from
> >>>>>> disconnect
> >>>>>> and opening a different new connection.
> >>>>>
> >>>>> As far as I know using AF_UNSPEC to unconnect takes effect on
> >>>>> UDP(DATAGRAM) sockets.
> >>>>> To unconnect a UDP socket, we call connect but set the family
> >>>>> member of
> >>>>> the socket address structure (sin_family for IPv4 or sin6_family for
> >>>>> IPv6) to AF_UNSPEC. It is the process of calling connect on an already
> >>>>> connected UDP socket that causes the socket to become unconnected.
> >>>>>
> >>>>> This RFC patch just supports TCP connections. I need to check the
> >>>>> logic
> >>>>> if AF_UNSPEC provided in connenct() function for TCP(STREAM) sockets.
> >>>>> Does it disconnect already established TCP connection?
> >>>>>
> >>>>> Thank you for noticing about this issue. Need to think through how
> >>>>> to manage it with Landlock network restrictions for both TCP and UDP
> >>>>> sockets.
> >>>>
> >>>> AF_UNSPEC also disconnects TCP.
> >>>
> >>> So its possible to call connect() with AF_UNSPEC and make a socket
> >>> unconnected. If you want to establish another connection to a socket
> >>> with port Y, and if there is a landlock rule has applied to a process
> >>> (or container) which restricts to connect to a socket with port Y, it
> >>> will be banned.
> >>> Thats the basic logic.
> >>
> >> Understood, and that works fine for connect. It would be good to also
> >> ensure that a now-bound socket cannot call listen. Possibly for
> >> follow-on work.
> >
> > Are you thinking about a new access right for listen? What would be the
> > use case vs. the bind access right?
> > .
>
>   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.

Powered by blists - more mailing lists