Re: [PATCH v11 1/8] Landlock: Add abstract UNIX socket restriction

Re: [PATCH v11 1/8] Landlock: Add abstract UNIX socket restriction

[Mickaël Salaün]

On Wed, Sep 04, 2024 at 06:13:55PM -0600, Tahera Fahimi wrote:
> This patch introduces a new "scoped" attribute to the
> landlock_ruleset_attr that can specify
> "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope abstract UNIX sockets
> from connecting to a process outside of the same Landlock domain. It
> implements two hooks, unix_stream_connect and unix_may_send to enforce
> this restriction.
> 
> Closes: https://github.com/landlock-lsm/linux/issues/7
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>

> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 849f5123610b..b9390445d242 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c

> +static int hook_unix_stream_connect(struct sock *const sock,
> +				    struct sock *const other,
> +				    struct sock *const newsk)
> +{
> +	const struct landlock_ruleset *const dom =
> +		landlock_get_current_domain();
> +
> +	/* quick return for non-sandboxed processes */
> +	if (!dom)
> +		return 0;
> +
> +	if (is_abstract_socket(other) && sock_is_scoped(other, dom))
> +		return -EPERM;

I was wondering if it would make more sense to return -EACCES here.
EACCES is usually related to file permission, but send(2)/sendto(2)
don't return EPERM according to man pages.  Well, according to the
kernel code they can return EPERM so I think we are good with EPERM.

It looks like other LSMs always use EACCES though...

> +
> +	return 0;
> +}
> +
> +static int hook_unix_may_send(struct socket *const sock,
> +			      struct socket *const other)
> +{
> +	const struct landlock_ruleset *const dom =
> +		landlock_get_current_domain();
> +
> +	if (!dom)
> +		return 0;
> +
> +	if (is_abstract_socket(other->sk)) {
> +		/*
> +		 * Checks if this datagram socket was already allowed to
> +		 * be connected to other.
> +		 */
> +		if (unix_peer(sock->sk) == other->sk)
> +			return 0;
> +
> +		if (sock_is_scoped(other->sk, dom))
> +			return -EPERM;

ditto

Re: [PATCH v11 1/8] Landlock: Add abstract UNIX socket restriction

[Mickaël Salaün]

On Wed, Sep 04, 2024 at 06:13:55PM -0600, Tahera Fahimi wrote:
> This patch introduces a new "scoped" attribute to the
> landlock_ruleset_attr that can specify
> "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to scope abstract UNIX sockets
> from connecting to a process outside of the same Landlock domain. It
> implements two hooks, unix_stream_connect and unix_may_send to enforce
> this restriction.
> 
> Closes: https://github.com/landlock-lsm/linux/issues/7
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> 
> ---
> v11:
> - For a connected abstract datagram socket, the hook_unix_may_send
>   allows the socket to send a data. (it is treated as a connected stream
>   socket)
> - Minor comment revision.
> v10:
> - Minor code improvement based on reviews on v9.
> v9:
> - Editting inline comments.
> - Major refactoring in domain_is_scoped() and is_abstract_socket
> v8:
> - Code refactoring (improve code readability, renaming variable, etc.)
>   based on reviews by Mickaël Salaün on version 7.
> - Adding warn_on_once to check (impossible) inconsistencies.
> - Adding inline comments.
> - Adding check_unix_address_format to check if the scoping socket is an
>   abstract UNIX sockets.
> v7:
> - Using socket's file credentials for both connected(STREAM) and
>   non-connected(DGRAM) sockets.
> - Adding "domain_sock_scope" instead of the domain scoping mechanism
>   used in ptrace ensures that if a server's domain is accessible from
>   the client's domain (where the client is more privileged than the
>   server), the client can connect to the server in all edge cases.
> - Removing debug codes.
> v6:
> - Removing curr_ruleset from landlock_hierarchy, and switching back to
>   use the same domain scoping as ptrace.
> - code clean up.
> v5:
> - Renaming "LANDLOCK_*_ACCESS_SCOPE" to "LANDLOCK_*_SCOPE"
> - Adding curr_ruleset to hierarachy_ruleset structure to have access
>   from landlock_hierarchy to its respective landlock_ruleset.
> - Using curr_ruleset to check if a domain is scoped while walking in the
>   hierarchy of domains.
> - Modifying inline comments.
> v4:
> - Rebased on Günther's Patch:
>   https://lore.kernel.org/all/20240610082115.1693267-1-gnoack@google.com/
>   so there is no need for "LANDLOCK_SHIFT_ACCESS_SCOPE", then it is
>   removed.
> - Adding get_scope_accesses function to check all scoped access masks in
>   a ruleset.
> - Using socket's file credentials instead of credentials stored in
>   peer_cred for datagram sockets. (see discussion in [1])
> - Modifying inline comments.
> V3:
> - Improving commit description.
> - Introducing "scoped" attribute to landlock_ruleset_attr for IPC
>   scoping purpose, and adding related functions.
> - Changing structure of ruleset based on "scoped".
> - Removing rcu lock and using unix_sk lock instead.
> - Introducing scoping for datagram sockets in unix_may_send.
> V2:
> - Removing wrapper functions
> 
> [1]https://lore.kernel.org/all/20240610.Aifee5ingugh@digikod.net/
> ---
>  include/uapi/linux/landlock.h                |  28 ++++
>  security/landlock/limits.h                   |   3 +
>  security/landlock/ruleset.c                  |   7 +-
>  security/landlock/ruleset.h                  |  24 +++-
>  security/landlock/syscalls.c                 |  17 ++-
>  security/landlock/task.c                     | 136 +++++++++++++++++++
>  tools/testing/selftests/landlock/base_test.c |   2 +-
>  7 files changed, 208 insertions(+), 9 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 2c8dbc74b955..dfd48d722834 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -44,6 +44,12 @@ struct landlock_ruleset_attr {
>  	 * flags`_).
>  	 */
>  	__u64 handled_access_net;
> +	/**
> +	 * @scoped: Bitmask of scopes (cf. `Scope flags`_)
> +	 * restricting a Landlock domain from accessing outside
> +	 * resources(e.g. IPCs).
> +	 */
> +	__u64 scoped;
>  };
>  
>  /*
> @@ -274,4 +280,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: scope
> + *
> + * Scope flags
> + * ~~~~~~~~~~~
> + *
> + * These flags enable to restrict a sandboxed process from a set of IPC
> + * actions. Setting a flag for a ruleset will isolate the Landlock domain
> + * to forbid connections to resources outside the domain.
> + *
> + * IPCs with scoped actions:
> + *
> + * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandboxed process
> + *   from connecting to an abstract unix socket created by a process
> + *   outside the related Landlock domain (e.g. a parent domain or a
> + *   non-sandboxed process).
> + */
> +/* clang-format off */
> +#define LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET		(1ULL << 0)

Thinking more about it, it makes more sense to rename it to
LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET (s/SCOPED/SCOPE/) because it
express a scope (not a "scoped") and it allign with the current
LANDLOCK_ACCESS_* and other internal variables such as
LANDLOCK_LAST_SCOPE...

However, it still makes sense to keep the "scoped" ruleset's field,
which is pretty similar to the "handled_*" semantic: it describes what
will be *scoped* by the ruleset.

> +/* clang-format on*/
> +
>  #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 4eb643077a2a..eb01d0fb2165 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -26,6 +26,9 @@
>  #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
>  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>  
> +#define LANDLOCK_LAST_SCOPE		LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
> +#define LANDLOCK_MASK_SCOPE		((LANDLOCK_LAST_SCOPE << 1) - 1)
> +#define LANDLOCK_NUM_SCOPE		__const_hweight64(LANDLOCK_MASK_SCOPE)
>  /* clang-format on */