Re: [PATCH v3] landlock: Add abstract unix socket connect restriction

On Mon, Jun 10, 2024 at 11:49:21PM +0200, Jann Horn wrote:
> On Mon, Jun 10, 2024 at 6:36 PM Mickaël Salaün <[email protected]> wrote:
> > On Fri, Jun 07, 2024 at 01:41:39PM -0600, Tahera Fahimi wrote:
> > > On Fri, Jun 07, 2024 at 10:28:35AM +0200, Günther Noack wrote:
> > > > Is it intentional that you are both restricting the connection and the sending
> > > > with the same flag (security_unix_may_send)?  If an existing Unix Domain Socket
> > > > gets passed in to a program from the outside (e.g. as stdout), shouldn't it
> > > > still be possible that the program enables a Landlock policy and then still
> > > > writes to it?  (Does that work?  Am I mis-reading the patch?)
> >
> > If a passed socket is already connected, then a write/send should work.
> 
> If I'm reading unix_dgram_sendmsg() correctly, we'll always hit
> security_unix_may_send() for any UNIX socket type other than
> SOCK_SEQPACKET (meaning SOCK_STREAM and SOCK_DGRAM), even if the
> socket is already connected, and then we'll do the landlock check.
> That's probably not the intended behavior for Landlock, unless I'm
> misreading the code?
> 
> Maybe to get nice semantics it's necessary to add a parameter to
> security_unix_may_send() that says whether the destination address
> came from the caller or from the socket?

I think it would make sense to ignore connected sockets with
security_unix_may_send() because it should already be controlled by
security_unix_stream_connect().  This would still allow passed/inherited
connected sockets to be used, which is an important feature and would
be consistent with read/write on other passed FDs.  This would not work
with dgram sockets though.

We need tests for this case and with different socket modes (inspired by
the net_test.c:protocol variants).