Message-ID: <1b1aebc0-2915-0720-24c6-ba67ac0255f0@huawei.com>
Date:   Thu, 10 Feb 2022 05:04:46 +0300
From:   Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC:     Mickaël Salaün <mic@...ikod.net>,
        <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



2/7/2022 7:00 PM, Willem de Bruijn пишет:
> 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.

  Thank you so much. I will check it and think about a test logic.
> .

Powered by blists - more mailing lists