Re: [RFC PATCH v1 0/3] Use socket's Landlock domain

[Mickaël Salaün]

On Tue, Jul 23, 2024 at 03:07:53PM +0200, Günther Noack wrote:
> Hello Mickaël!
> 
> On Fri, Jul 19, 2024 at 05:06:15PM +0200, Mickaël Salaün wrote:
> > While the current approach works, I think we should change the way
> > Landlock restricts network actions.  Because this feature is relatively
> > new, we can still fix this inconsistency.  In a nutshell, let's follow a
> > more capability-based model.  Please let me know what you think.
> 
> Thanks for sending the patch.  The implementation with ->f_cred is much simpler
> than I had thought it would be.  Some higher level questions:
> 
>  * I assume that the plan is to backport this as a fix to older kernels that
>    already have the feature?  (Otherwise, we would potentially have backwards
>    compatibility issues.)

Correct, if this patch is merged it must be backported too, but there
might be better alternatives, or we might just stick to the initial
approach.

> 
>  * I believe it clashes a little bit with the TCP server example [1],
>    which I found a useful use case for the TCP connect/bind and socket
>    restriction features.

Indeed, because the socket is created before the sandboxing, the socket
could be reused to bind (or connect) to other ports.  This is a good
example of why using current's instead of socket's credentials may be
less surprising.  From my point of view, the main issue is that a socket
can be reconfigured.

> 
>  * accept(2) on a passive (listen-mode) socket will give you a new socket FD
>    -- does that new socket inherit its f_cred from the server socket,
>    or does it inherit its f_cred from the thread?

According to sock_alloc_file(), the newly created socket inherits the
caller's credentials, which is similar to a call to openat2(2) with a
directory file descriptor.

> 
> Regarding the TCP server example, the current implementation is *very* simple,
> and does the following steps:
> 
>  1. create socket with socket(2)
>  2. bind(2) the socket to the desired port
>  3. enforce a Landlock ruleset that disables all networking features
>     (TCP bind, TCP connect and socket creation with the new patch set)
>  4. listen(2) on the socket
>  5. go into the accept(2) loop
> 
> With the old behaviour, step 3 is going to affect the existing passive socket:
> It will not be possible any more to bind(2) that passive socket to another port.
> 
> With the new behaviour (after your patch), step 3 does *not* affect the existing
> socket, and the server socket can be reused to bind(2) to other ports.
> 
> Or, in other words: If the relevant domain is tied to the socket at creation
> time, that means that a future client connection which takes over the process
> might be able to use that socket's Landlock domain, which potentially grants
> more permissions than the thread's domain

Yes, that's why it might be more risky, but I wanted to have this
discussion.  Whatever the outcome, it should be explained in
Documentation/security/landlock.rst

One thing to keep in mind and that contrary to other LSMs, Landlock,
like seccomp, enables processes to (only) drop privileges, and this is
done by the process itself (not at execve time).  This was also one
argument for the initial approach.

A thing that bothered me was related to the restrictions of sockets,
especially with the WIP scoping feature.  Datagram (unix) sockets could
work before sandboxing, and suddenly become broken after sandboxing
(because the security check would be done at send time instead of
connect time).  This kind of issue should be identified quite early and
easily though.

However, there is still an inconsistency between connected stream
sockets and datagram sockets.  From a security point of view, this looks
like a good thing though.

> 
> I think it would be nice if a use case like in the TCP server example would
> still be possible with Landlock; there are multiple ways to reach that:
> 
>  - We could enforce two layers of Landlock rules, one before socket creation
>    that restricts bind(2) to a given port, and one after socket creation that
>    restricts other bind(2), create(2) and socket(2) operations.
> 
>    Drawbacks:
> 
>    - One Landlock layer more, and needs to add a Landlock rule:
>      This is a bit more complicated to implement.

Right, I think it's too complex for users.

>    - The bind(2) restriction on the socket is still only per port *number*,
>      but it is still possible to bind with that port number on different IP
>      addresses.

Good point.  That's another argument for the initial approach and the
way you sandboxed the example: dropping the *_TCP access rights, it is
not possible to rebind or reconnect a socket (to another address).

Actually, I'm not sure if using the socket's credential would not
confuse users to understand why an access is denied (or allowed).

> 
>  - Alternatively, I wish we could just lock the passive server socket in, so
>    that it can't be made to reconnect anywhere else.  After all, the socket
>    disassociation with connect(2) with AF_UNSPEC seems to be a somewhat obscure
>    and seldomly used feature - if we could just disallow that operation, we
>    could ensure that this socket gets reused for such a nefarious purpose.

We could also add a new "scope" for socket reconfiguration of sockets
created by a parent or sibling domain, similar to the ptrace
restrictions.

> 
>    It would still require two nested Landlock rulesets to make the TCP server
>    example work, but they would be a bit simpler than in the alternative above.
> 
>  - There are probably more alternatives...?
> 
> What do you think?

I see other alternatives:

- We could have a new ruleset's attribute to specify if network
  restrictions should apply on the caller or the socket.  That might be
  confusing for users though.

- We could just stick to the initial approach and add new access rights
  (denied by default, similar to FS_REFER) that will only apply to newly
  created sockets.  This is close to the previous alternative but more
  explicit. Both use cases could then be used, with a default secure
  approach (i.e. the initial one).  However, we need to have a clear
  rationale for the WIP scoping restrictions: should the caller or the
  socket be checked as the client?

- We could extend the current approach and check both the caller's
  credential and the socket's credential.  This could be confusing to
  users though.

- We could have a new fcntl(2) command to (securely) transition a file
  descriptor's credential to the caller's one (e.g. approved by
  ptrace_may_access).  That could be generic to all Linux access control
  systems.

- According to a new ruleset's attribute, we could revalidate (at use
  time) file descriptors not opened by the caller's Landlock domain, but
  users would have to be explicit (e.g. stdio issue). And how to handle
  partially allowed accesses?

> 
> —Günther
> 
> [1] https://wiki.gnoack.org/LandlockTcpServerExample
>