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

[Mickaël Salaün]

On Tue, Jun 11, 2024 at 12:27:58AM +0200, Jann Horn wrote:
> Hi!
> 
> Thanks for helping with making Landlock more comprehensive!
> 
> On Fri, Jun 7, 2024 at 1:44 AM Tahera Fahimi <fahimitahera@xxxxxxxxx> wrote:
> > Abstract unix sockets are used for local inter-process communications
> > without on a filesystem. Currently a sandboxed process can connect to a
> > socket outside of the sandboxed environment, since landlock has no
> > restriction for connecting to a unix socket in the abstract namespace.
> > Access to such sockets for a sandboxed process should be scoped the same
> > way ptrace is limited.
> 
> This reminds me - from what I remember, Landlock also doesn't restrict
> access to filesystem-based unix sockets yet... I'm I'm right about
> that, we should probably at some point add code at some point to
> restrict that as part of the path-based filesystem access rules? (But
> to be clear, I'm not saying I expect you to do that as part of your
> patch, just commenting for context.)

Yes, I totally agree.  For now, unix socket binding requires to create
the LANDLOCK_ACCESS_FS_MAKE_SOCK right, but connecting to an existing
socket is not controlled.  The abstract unix socket scoping is
orthogonal and extends Landlock with unix socket LSM hooks, which are
required to extend the "filesystem" access rights to control path-based
unix socket.

> 
> > Because of compatibility reasons and since landlock should be flexible,
> > we extend the user space interface by adding a new "scoped" field. This
> > field optionally contains a "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to
> > specify that the ruleset will deny any connection from within the
> > sandbox to its parents(i.e. any parent sandbox or non-sandbox processes)
> 
> You call the feature "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET", but I
> don't see anything in this code that actually restricts it to abstract
> unix sockets (as opposed to path-based ones and unnamed ones, see the
> "Three types of address are distinguished" paragraph of
> https://man7.org/linux/man-pages/man7/unix.7.html). If the feature is
> supposed to be limited to abstract unix sockets, I guess you'd maybe
> have to inspect the unix_sk(other)->addr, check that it's non-NULL,
> and then check that `unix_sk(other)->addr->name->sun_path[0] == 0`,
> similar to what unix_seq_show() does? (unix_seq_show() shows abstract
> sockets with an "@".)

Right, that should be part of the next series.  Tests should check that
too.

> 
> Separately, I wonder if it would be useful to have another mode for
> forbidding access to abstract unix sockets entirely; or alternatively
> to change the semantics of LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET so
> that it also forbids access from outside the landlocked domain as was
> discussed elsewhere in the thread. If a landlocked process starts
> listening on something like "@/tmp/.X11-unix/X0", maybe X11 clients
> elsewhere on my system shouldn't be confused into connecting to that
> landlocked socket...

In this case, I think we should have a (light) Landlock domain for a
user session to make sure apps only connect to the legitimate X11 socket
(either in the same domain, or through a path-based socket).

There is also ongoing work to restrict socket creation according to their
type:
https://lore.kernel.org/all/20240524093015.2402952-1-ivanov.mikhail1@xxxxxxxxxxxxxxxxxxx/
This will make possible to control abstract unix socket creation and
avoid this kind of issue too.

> 
> [...]
> > +static bool sock_is_scoped(struct sock *const other)
> > +{
> > +       bool is_scoped = true;
> > +       const struct landlock_ruleset *dom_other;
> > +       const struct cred *cred_other;
> > +
> > +       const struct landlock_ruleset *const dom =
> > +               landlock_get_current_domain();
> > +       if (!dom)
> > +               return true;
> > +
> > +       lockdep_assert_held(&unix_sk(other)->lock);
> > +       /* the credentials will not change */
> > +       cred_other = get_cred(other->sk_peer_cred);
> > +       dom_other = landlock_cred(cred_other)->domain;
> > +       is_scoped = domain_scope_le(dom, dom_other);
> > +       put_cred(cred_other);
> 
> You don't have to use get_cred()/put_cred() here; as the comment says,
> the credentials will not change, so we don't need to take another
> reference to them.
> 
> > +       return is_scoped;
> > +}
>