2/7/2022 7:00 PM, Willem de Bruijn пишет:

On Mon, Feb 7, 2022 at 12:51 AM Konstantin Meskhidze
[off-list ref] wrote:

quoted

2/1/2022 3:33 PM, Mickaël Salaün пишет:

quoted

On 31/01/2022 18:14, Willem de Bruijn wrote:

quoted

On Fri, Jan 28, 2022 at 10:12 PM Konstantin Meskhidze
[off-list ref] wrote:

quoted

1/26/2022 5:15 PM, Willem de Bruijn пишет:

quoted

On Wed, Jan 26, 2022 at 3:06 AM Konstantin Meskhidze
[off-list ref] wrote:

quoted

1/25/2022 5:17 PM, Willem de Bruijn пишет:

quoted

On Mon, Jan 24, 2022 at 3:02 AM Konstantin Meskhidze
[off-list ref] wrote:

quoted

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
[off-list ref]

quoted

+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...

quoted

quoted

quoted

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.

.