Date: Fri, 7 Jun 2024 10:28:35 +0200
From: "Günther Noack" <gnoack@...gle.com>
To: Tahera Fahimi <fahimitahera@...il.com>
Cc: "Mickaël Salaün" <mic@...ikod.net>, 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, 
	Jann Horn <jannh@...gle.com>, netdev@...r.kernel.org, 
	"Björn Roy Baron" <bjorn3_gh@...tonmail.com>
Subject: Re: [PATCH v3] landlock: Add abstract unix socket connect restriction

Hello Tahera!

Thanks for sending another revision of your patch set!

On Thu, Jun 06, 2024 at 05:44:46PM -0600, Tahera Fahimi 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.
> 
> 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)
> 
> Closes: https://github.com/landlock-lsm/linux/issues/7
> Signed-off-by: Tahera Fahimi <fahimitahera@...il.com>
> 
> -------
> V3: Added "scoped" field to landlock_ruleset_attr
> V2: Remove wrapper functions
> 
> -------
> ---
>  include/uapi/linux/landlock.h | 28 +++++++++++++++++++++++
>  security/landlock/limits.h    |  5 ++++
>  security/landlock/ruleset.c   | 15 ++++++++----
>  security/landlock/ruleset.h   | 28 +++++++++++++++++++++--
>  security/landlock/syscalls.c  | 12 +++++++---
>  security/landlock/task.c      | 43 +++++++++++++++++++++++++++++++++++
>  6 files changed, 121 insertions(+), 10 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 68625e728f43..d887e67dc0ed 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -37,6 +37,12 @@ struct landlock_ruleset_attr {
>  	 * rule explicitly allow them.
>  	 */
>  	__u64 handled_access_net;
> +	/**
> +	 * scoped: Bitmask of actions (cf. `Scope access flags`_)
> +	 * that is handled by this ruleset and should be permitted
> +	 * by default if no rule explicitly deny them.
> +	 */
> +	__u64 scoped;

I have trouble understanding what this docstring means.

If those are "handled" things, shouldn't the name also start with "handled_", in
line with the other fields?  Also, I don't see any way to manipulate these
rights with a Landlock rule in this ?

How about:

/**
 * handled_scoped: Bitmask of IPC actions (cf. `Scoped access flags`_)
 * which are confined to only affect the current Landlock domain.
 */
__u64 handled_scoped;

>  };
>  
>  /*
> @@ -266,4 +272,26 @@ struct landlock_net_port_attr {
>  #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>  #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
>  /* clang-format on */
> +
> +/**
> + * DOC: scoped
> + *
> + * Scoped handles a set of restrictions on kernel IPCs.
> + *
> + * Scope access flags

Scoped with a "d"?

> + * ~~~~~~~~~~~~~~~~~~~~
> + * 
> + * These flags enable to restrict a sandboxed process from a set of
> + * inter-process communications actions. Setting a flag in a landlock
> + * domain will isolate the Landlock domain to forbid connections
> + * to resources outside the domain.
> + *
> + * IPCs with scoped actions:
> + * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandbox process to
> + *   connect to a process outside of the sandbox domain through abstract
> + *   unix sockets.
> + */
> +/* clang-format off */
> +#define LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET		(1ULL << 0)

Should the name of this #define indicate the direction that we are restricting?
If I understand your documentation correctly, this is about *connecting out* of
the current Landlock domain, but incoming connections from more privileged
domains are OK, right?


Also:

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

The way that write access is normally checked for other files is at the time
when you open the file, not during write(), and I believe it would be more in
line with that normal "check at open" behaviour if we did the same here?


> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 20fdb5ff3514..7b794b81ef05 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -28,6 +28,11 @@
>  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>  #define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
>  
> +#define LANDLOCK_LAST_ACCESS_SCOPE       LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
> +#define LANDLOCK_MASK_ACCESS_SCOPE	((LANDLOCK_LAST_ACCESS_SCOPE << 1) - 1)
> +#define LANDLOCK_NUM_ACCESS_SCOPE         __const_hweight64(LANDLOCK_MASK_ACCESS_SCOPE)
> +#define LANDLOCK_SHIFT_ACCESS_SCOPE      LANDLOCK_SHIFT_ACCESS_NET
                                            ^^^^^^^^^^^^^^^^^^^^^^^^^

I believe this #define has the wrong value, and as a consequence, the code
suffers from the same problem as we already had on the other patch set from
Mikhail Ivanov -- see [1] for that discussion.

The LANDLOCK_SHIFT_ACCESS_FOO variable is used for determining the position of
your flag in the access_masks_t type, where all access masks are combined
together in one big bit vector.  If you are defining this the same for _SCOPE as
for _NET, I believe that we will start using the same bits in that vector for
both the _NET flags and the _SCOPE flags, and that will manifest in unwanted
interactions between the different types of restrictions.  (e.g. you will create
a policy to restrict _SCOPE, and you will find yourself unable to do some things
with TCP ports)

Please also see the other thread for more discussions about how we can avoid
such problems in the future.  (This code is easy to get wrong,
apparently... When we don't test what happens across multiple types of
restrictions, everything looks fine.)

[1] https://lore.kernel.org/all/ebd680cc-25d6-ee14-4856-310f5e5e28e4@huawei-partners.com/

—Günther

Powered by blists - more mailing lists