Re: [PATCH v9 00/12] Network support for Landlock - allowed list of protocols

[Jeff Xu <jeffxu@chromium.org>]

resend.

On Wed, Jun 28, 2023 at 12:29 PM Mickaël Salaün <mic@digikod.net> wrote:
>
>
> On 28/06/2023 19:03, Jeff Xu wrote:
> > 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.
>
> Restriction are applied to actions, not to already existing/opened FDs.
> We could add a way to restrict opened FDs, but I don't think this is the
> right approach because sandboxing is a deliberate action from a process,
> and it should already take care of its FDs.
>
>
> >
> > On Wed, Jun 28, 2023 at 2:09 AM Günther Noack <gnoack@google.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.
>
> seccomp restricts the use of the syscall interface, whereas Landlock
> restricts the use of kernel objects (i.e. the semantic).
>
> We need to find a good tradeoff between a lot of access rights and a few
> grouping different actions. This should make sense from a developer
> point of view according to its knowledge and use of the kernel
> interfaces (potential wrapped with high level libraries), but also to
> the semantic of the sandbox and the security guarantees we want to provide.
>
> We should also keep in mind that high level Landlock libraries can take
> care of potential coarse-grained use of restrictions.
>
>
> >
> >> 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.
>
> Right, seccomp cannot filter TCP ports.
>
> >
> > 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.
>
> Good point. I think it is in line with the rule definition: to allow to
> bind on a specific port. However, if clients want to set the source port
> to a (legitimate) value, then that would be an issue because we cannot
> allow a whole range of ports (e.g., >= 1024). I'm not sure if this
> practice would be deemed "legitimate" though. Do you know client
> applications using bind?
>
> Konstantin, we should have a test for this case anyway.
>
>
> >
> >> (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.
>
> I don't get the semantic of LANDLOCK_ACCESS_SOCKET_PROTOCOL. What does
> PROTOCOL mean?
>
I meant checking family + type of socket, and apply to all of
socket(2),bind(2),accept(2),connect(2),listen(2), maybe
send(2)/recv(2) too.

s/LANDLOCK_ACCESS_SOCKET_CREATE/LANDLOCK_ACCESS_SOCKET_TYPE.

This implies the kernel will check on socket fd's property (family +
type) at those calls, this applies to
a - the socket fd is created within the process, after landlock is applied.
b - created in process prior to landlock is applied.
c - created out of process then passed into this process,

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