From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
Cc: "Mickaël Salaün" <mic@digikod.net>,
"Willem de Bruijn" <willemdebruijn.kernel@gmail.com>,
linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
netfilter@vger.kernel.org, yusongping@huawei.com,
artem.kuzin@huawei.com
Subject: Re: [RFC PATCH 1/2] landlock: TCP network hooks implementation
Date: Mon, 7 Feb 2022 11:00:34 -0500 [thread overview]
Message-ID: <CA+FuTScaoby-=xRKf_Dz3koSYHqrMN0cauCg4jMmy_nDxwPADA@mail.gmail.com> (raw)
In-Reply-To: <91885a8f-b787-62ff-1abb-700641f7c2cb@huawei.com>
On Mon, Feb 7, 2022 at 12:51 AM Konstantin Meskhidze
<konstantin.meskhidze@huawei.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@huawei.com> wrote:
> >>>
> >>>
> >>>
> >>> 1/26/2022 5:15 PM, Willem de Bruijn пишет:
> >>>> On Wed, Jan 26, 2022 at 3:06 AM Konstantin Meskhidze
> >>>> <konstantin.meskhidze@huawei.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> 1/25/2022 5:17 PM, Willem de Bruijn пишет:
> >>>>>> On Mon, Jan 24, 2022 at 3:02 AM Konstantin Meskhidze
> >>>>>> <konstantin.meskhidze@huawei.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@huawei.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.
next prev parent reply other threads:[~2022-02-07 16:16 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-24 8:02 [RFC PATCH 0/2] landlock network implementation cover letter Konstantin Meskhidze
2022-01-24 8:02 ` [RFC PATCH 1/2] landlock: TCP network hooks implementation Konstantin Meskhidze
2022-01-25 14:17 ` Willem de Bruijn
2022-01-26 8:05 ` Konstantin Meskhidze
2022-01-26 14:15 ` Willem de Bruijn
2022-01-29 3:12 ` Konstantin Meskhidze
2022-01-31 17:14 ` Willem de Bruijn
2022-02-01 12:33 ` Mickaël Salaün
2022-02-07 2:31 ` Konstantin Meskhidze
2022-02-07 16:00 ` Willem de Bruijn [this message]
2022-02-07 16:17 ` Willem de Bruijn
2022-02-10 2:05 ` Konstantin Meskhidze
2022-02-10 2:04 ` Konstantin Meskhidze
2022-02-01 12:28 ` Mickaël Salaün
2022-02-07 2:35 ` Konstantin Meskhidze
2022-02-01 12:13 ` Mickaël Salaün
2022-02-07 13:09 ` Konstantin Meskhidze
2022-02-07 14:17 ` Mickaël Salaün
2022-02-08 7:55 ` Konstantin Meskhidze
2022-02-08 12:09 ` Mickaël Salaün
2022-02-09 3:06 ` Konstantin Meskhidze
2022-01-24 8:02 ` [RFC PATCH 2/2] landlock: selftests for bind and connect hooks Konstantin Meskhidze
2022-02-01 18:31 ` Mickaël Salaün
2022-02-07 7:11 ` Konstantin Meskhidze
2022-02-07 12:49 ` Mickaël Salaün
2022-02-08 3:01 ` Konstantin Meskhidze
2022-02-08 12:17 ` Mickaël Salaün
2022-02-09 3:03 ` Konstantin Meskhidze
2022-02-10 10:16 ` Mickaël Salaün
2022-02-24 3:18 ` Konstantin Meskhidze
2022-02-24 9:55 ` Mickaël Salaün
2022-02-24 12:03 ` Konstantin Meskhidze
2022-02-24 14:15 ` Mickaël Salaün
2022-02-25 2:44 ` Konstantin Meskhidze
2022-02-01 17:53 ` [RFC PATCH 0/2] landlock network implementation cover letter Mickaël Salaün
2022-02-07 13:18 ` Konstantin Meskhidze
2022-02-07 13:35 ` Mickaël Salaün
2022-02-08 3:53 ` Konstantin Meskhidze
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+FuTScaoby-=xRKf_Dz3koSYHqrMN0cauCg4jMmy_nDxwPADA@mail.gmail.com' \
--to=willemdebruijn.kernel@gmail.com \
--cc=artem.kuzin@huawei.com \
--cc=konstantin.meskhidze@huawei.com \
--cc=linux-security-module@vger.kernel.org \
--cc=mic@digikod.net \
--cc=netdev@vger.kernel.org \
--cc=netfilter@vger.kernel.org \
--cc=yusongping@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).