Message-ID: <Zp-q9zxmCmGjR2-N@google.com>
Date: Tue, 23 Jul 2024 15:07:53 +0200
From: "Günther Noack" <gnoack@...gle.com>
To: "Mickaël Salaün" <mic@...ikod.net>
Cc: Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>, 
	Konstantin Meskhidze <konstantin.meskhidze@...wei.com>, Paul Moore <paul@...l-moore.com>, 
	Casey Schaufler <casey@...aufler-ca.com>, Jeff Xu <jeffxu@...gle.com>, 
	Kees Cook <keescook@...omium.org>, "Serge E . Hallyn" <serge@...lyn.com>, 
	Shervin Oloumi <enlightened@...omium.org>, Tahera Fahimi <fahimitahera@...il.com>, 
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [RFC PATCH v1 0/3] Use socket's Landlock domain

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.)

 * 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.

 * 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?

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

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.
   - 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.

 - 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.

   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?

—Günther

[1] https://wiki.gnoack.org/LandlockTcpServerExample

Powered by blists - more mailing lists