Message-ID: <CABi2SkX-dzUO6NnbyqfrAg7Bbn+Ne=Xi1qC1XMrzHqVEVucQ0Q@mail.gmail.com>
Date: Wed, 28 Jun 2023 10:03:08 -0700
From: Jeff Xu <jeffxu@...omium.org>
To: Günther Noack <gnoack@...gle.com>
Cc: Mickaël Salaün <mic@...ikod.net>, 
	"Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>, Günther Noack <gnoack3000@...il.com>, 
	willemdebruijn.kernel@...il.com, linux-security-module@...r.kernel.org, 
	netdev@...r.kernel.org, netfilter-devel@...r.kernel.org, 
	yusongping@...wei.com, artem.kuzin@...wei.com, Jeff Xu <jeffxu@...gle.com>, 
	Jorge Lucangeli Obes <jorgelo@...omium.org>, Allen Webb <allenwebb@...gle.com>, 
	Dmitry Torokhov <dtor@...gle.com>
Subject: Re: [PATCH v9 00/12] Network support for Landlock - allowed list of protocols

Hello,

Thanks for writing up the example for an incoming TCP connection ! It
helps with the context.

Since I'm late to this thread, one thing I want to ask:  all the APIs
proposed so far are at the process level, we don't have any API that
applies restriction to socket fd itself, right ? this is what I
thought, but I would like to get confirmation.

On Wed, Jun 28, 2023 at 2:09 AM Günther Noack <gnoack@...gle.com> wrote:
>
> Hello!
>
> On Mon, Jun 26, 2023 at 05:29:34PM +0200, Mickaël Salaün wrote:
> > Here is a design to be able to only allow a set of network protocols and
> > deny everything else. This would be complementary to Konstantin's patch
> > series which addresses fine-grained access control.
> >
> > First, I want to remind that Landlock follows an allowed list approach with
> > a set of (growing) supported actions (for compatibility reasons), which is
> > kind of an allow-list-on-a-deny-list. But with this proposal, we want to be
> > able to deny everything, which means: supported, not supported, known and
> > unknown protocols.
> >
> > We could add a new "handled_access_socket" field to the landlock_ruleset
> > struct, which could contain a LANDLOCK_ACCESS_SOCKET_CREATE flag.
> >
> > If this field is set, users could add a new type of rules:
> > struct landlock_socket_attr {
> >     __u64 allowed_access;
> >     int domain; // see socket(2)
> >     int type; // see socket(2)
> > }
> >
> > The allowed_access field would only contain LANDLOCK_ACCESS_SOCKET_CREATE at
> > first, but it could grow with other actions (which cannot be handled with
> > seccomp):
> > - use: walk through all opened FDs and mark them as allowed or denied
> > - receive: hook on received FDs
> > - send: hook on sent FDs
> >
> > We might also use the same approach for non-socket objects that can be
> > identified with some meaningful properties.
> >
> > What do you think?
>
> This sounds like a good plan to me - it would make it possible to restrict new
> socket creation using protocols that were not intended to be used, and I also
> think it would fit the Landlock model nicely.
>
> Small remark on the side: The security_socket_create() hook does not only get
> invoked as a result of socket(2), but also as a part of accept(2) - so this
> approach might already prevent new connections very effectively.
>
That is an interesting aspect that might be worth discussing more.
seccomp is per syscall, landlock doesn't necessarily follow the same,
another design is to add more logic in Landlock, e.g.
LANDLOCK_ACCESS_SOCKET_PROTOCOL which will apply to all of the socket
calls (socket/bind/listen/accept/connect). App dev might feel it is
easier to use.

> Spelling out some scenarios, so that we are sure that we are on the same page:
>
> A)
>
> A program that does not need networking could specify a ruleset where
> LANDLOCK_ACCESS_SOCKET_CREATE is handled, and simply not permit anything.
>
> B)
>
> A program that runs a TCP server could specify a ruleset where
> LANDLOCK_NET_BIND_TCP, LANDLOCK_NET_CONNECT_TCP and
> LANDLOCK_ACCESS_SOCKET_CREATE are handled, and where the following rules are added:
>
>   /* From Konstantin's patch set */
>   struct landlock_net_service_attr bind_attr = {
>     .allowed_access = LANDLOCK_NET_BIND_TCP,
>     .port = 8080,
>   };
>
>   /* From Mickaël's proposal */
>   struct landlock_socket_attr sock_inet_attr = {
>     .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>     .domain = AF_INET,
>     .type = SOCK_STREAM,
>   }
>
>   struct landlock_socket_attr sock_inet6_attr = {
>     .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>     .domain = AF_INET6,
>      .type = SOCK_STREAM,
>   }
>
> That should then be enough to bind and listen on ports, whereas outgoing
> connections with TCP and anything using other network protocols would not be
> permitted.
>
TCP server is an interesting case. From a security perspective, a
process cares if it is acting as a server or client in TCP, a server
might only want to accept an incoming TCP connection, never initiate
an outgoing TCP connection, and a client is the opposite.

Processes can restrict outgoing/incoming TCP connection by seccomp for
accept(2) or connect(2),  though I feel Landlock can do this more
naturally for app dev, and at per-protocol level.  seccomp doesn't
provide per-protocol granularity.

For bind(2), iirc, it can be used for a server to assign dst port of
incoming TCP connection, also by a client to assign a src port of an
outgoing TCP connection. LANDLOCK_NET_BIND_TCP will apply to both
cases, right ? this might not be a problem, just something to keep
note.

> (Alternatively, it could bind() the socket early, *then enable Landlock* and
> leave out the rule for BIND_TCP, only permitting SOCKET_CREATE for IPv4 and
> IPv6, so that listen() and accept() work on the already-bound socket.)
>
For this approach, LANDLOCK_ACCESS_SOCKET_PROTOCOL is a better name,
so dev is fully aware it is not just applied to socket create.

> Overall, this sounds like an excellent approach to me. 👍
>
> —Günther
>
> --
> Sent using Mutt 🐕 Woof Woof

-Jeff

Powered by blists - more mailing lists