On Mon, Feb 7, 2022 at 12:51 AM Konstantin Meskhidze
<konstantin.meskhidze@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> >>>
> >>>
> >>>
> >>> 1/26/2022 5:15 PM, Willem de Bruijn пишет:
> >>>> On Wed, Jan 26, 2022 at 3:06 AM Konstantin Meskhidze
> >>>> <konstantin.meskhidze@xxxxxxxxxx> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> 1/25/2022 5:17 PM, Willem de Bruijn пишет:
> >>>>>> On Mon, Jan 24, 2022 at 3:02 AM Konstantin Meskhidze
> >>>>>> <konstantin.meskhidze@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>>>>>
> >>>>>>> +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.
