Date: Tue, 11 Jun 2024 00:27:58 +0200
From: Jann Horn <jannh@...gle.com>
To: Tahera Fahimi <fahimitahera@...il.com>
Cc: Mickaël Salaün <mic@...ikod.net>, 
	Günther Noack <gnoack@...gle.com>, 
	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>, 
	"Serge E. Hallyn" <serge@...lyn.com>, outreachy@...ts.linux.dev, netdev@...r.kernel.org, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>
Subject: Re: [PATCH v3] landlock: Add abstract unix socket connect restriction

Hi!

Thanks for helping with making Landlock more comprehensive!

On Fri, Jun 7, 2024 at 1:44 AM Tahera Fahimi <fahimitahera@...il.com> 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.)

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

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

[...]
> +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;
> +}

Powered by blists - more mailing lists