Re: [PATCH v7 1/4] Landlock: Add abstract unix socket connect restriction

["Mickaël Salaün" <mic@digikod.net>]

On Wed, Jul 17, 2024 at 10:15:19PM -0600, Tahera Fahimi wrote:
> The 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.
> 
> This patch implement two hooks, "unix_stream_connect" and "unix_may_send" to
> enforce this restriction.
> 
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> 
> -------

Only "---"

> v7:

Thanks for the detailed changelog, it helps!

>  - 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 file's FD 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/outreachy/Zmi8Ydz4Z6tYtpY1@tahera-OptiPlex-5000/T/#m8cdf33180d86c7ec22932e2eb4ef7dd4fc94c792


> -------
> 
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>

No need for this hunk.


> ---
>  include/uapi/linux/landlock.h |  29 +++++++++
>  security/landlock/limits.h    |   3 +
>  security/landlock/ruleset.c   |   7 ++-
>  security/landlock/ruleset.h   |  23 ++++++-
>  security/landlock/syscalls.c  |  14 +++--
>  security/landlock/task.c      | 112 ++++++++++++++++++++++++++++++++++
>  6 files changed, 181 insertions(+), 7 deletions(-)

> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 849f5123610b..597d89e54aae 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c
> @@ -13,6 +13,8 @@
>  #include <linux/lsm_hooks.h>
>  #include <linux/rcupdate.h>
>  #include <linux/sched.h>
> +#include <net/sock.h>
> +#include <net/af_unix.h>
>  
>  #include "common.h"
>  #include "cred.h"
> @@ -108,9 +110,119 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
>  	return task_ptrace(parent, current);
>  }
>  
> +static int walk_and_check(const struct landlock_ruleset *const child,
> +			  struct landlock_hierarchy **walker, int i, int j,

We don't know what are "i" and "j" are while reading this function's
signature.  They need a better name.

Also, they are ingegers (signed), whereas l1 and l2 are size_t (unsigned).

> +			  bool check)
> +{
> +	if (!child || i < 0)
> +		return -1;
> +
> +	while (i < j && *walker) {

This would be more readable with a for() loop.

> +		if (check && landlock_get_scope_mask(child, j))

This is correct now but it will be a bug when we'll have other scope.
Instead, you can replace the "check" boolean with a variable containing
LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET.

> +			return -1;
> +		*walker = (*walker)->parent;
> +		j--;
> +	}
> +	if (!*walker)
> +		pr_warn_once("inconsistency in landlock hierarchy and layers");

This must indeed never happen, but WARN_ON_ONCE(!*walker) would be
better than this check+pr_warn.

Anyway, if this happen this pointer will still be dereferenced in
domain_sock_scope() right?  This must not be possible.


> +	return j;

Because j is now equal to i, no need to return it.  This function can
return a boolean instead, or a struct landlock_ruleset pointer/NULL to
avoid the pointer of pointer?

> +}
> +
> +/**
> + * domain_sock_scope - Checks if client domain is scoped in the same
> + *			domain as server.
> + *
> + * @client: Connecting socket domain.
> + * @server: Listening socket domain.
> + *
> + * Checks if the @client domain is scoped, then the server should be
> + * in the same domain to connect. If not, @client can connect to @server.
> + */
> +static bool domain_sock_scope(const struct landlock_ruleset *const client,

This function can have a more generic name if
LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET is passed as argument.  This could
be reused as-is for other kind of scope.

> +			      const struct landlock_ruleset *const server)
> +{
> +	size_t l1, l2;
> +	int scope_layer;
> +	struct landlock_hierarchy *cli_walker, *srv_walker;

We have some room for a bit more characters ;)
client_walker, server_walker;

> +
> +	if (!client)
> +		return true;
> +
> +	l1 = client->num_layers - 1;

Please rename variables in a consistent way, in this case something like
client_layer?

> +	cli_walker = client->hierarchy;
> +	if (server) {
> +		l2 = server->num_layers - 1;
> +		srv_walker = server->hierarchy;
> +	} else
> +		l2 = 0;
> +
> +	if (l1 > l2)
> +		scope_layer = walk_and_check(client, &cli_walker, l2, l1, true);

Instead of mixing the layer number with an error code, walk_and_check()
can return a boolean, take as argument &scope_layer, and update it.

> +	else if (l2 > l1)
> +		scope_layer =
> +			walk_and_check(server, &srv_walker, l1, l2, false);
> +	else
> +		scope_layer = l1;
> +
> +	if (scope_layer == -1)
> +		return false;

All these domains and layers checks are difficult to review. It needs at
least some comments, and preferably also some code refactoring to avoid
potential inconsistencies (checks).

> +
> +	while (scope_layer >= 0 && cli_walker) {

Why srv_walker is not checked?  Could this happen?  What would be the
result?

Please also use a for() loop here.

> +		if (landlock_get_scope_mask(client, scope_layer) &
> +		    LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET) {

The logic needs to be explained.

> +			if (!server)
> +				return false;
> +
> +			if (srv_walker == cli_walker)
> +				return true;
> +
> +			return false;
> +		}
> +		cli_walker = cli_walker->parent;
> +		srv_walker = srv_walker->parent;
> +		scope_layer--;
> +	}
> +	return true;
> +}
> +
> +static bool sock_is_scoped(struct sock *const other)
> +{
> +	const struct landlock_ruleset *dom_other;
> +	const struct landlock_ruleset *const dom =
> +		landlock_get_current_domain();
> +
> +	/* the credentials will not change */
> +	lockdep_assert_held(&unix_sk(other)->lock);
> +	dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain;
> +
> +	/* other is scoped, they connect if they are in the same domain */
> +	return domain_sock_scope(dom, dom_other);
> +}
> +
> +static int hook_unix_stream_connect(struct sock *const sock,
> +				    struct sock *const other,
> +				    struct sock *const newsk)
> +{
> +	if (sock_is_scoped(other))
> +		return 0;
> +
> +	return -EPERM;
> +}
> +
> +static int hook_unix_may_send(struct socket *const sock,
> +			      struct socket *const other)
> +{
> +	if (sock_is_scoped(other->sk))
> +		return 0;
> +
> +	return -EPERM;
> +}
> +
>  static struct security_hook_list landlock_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check),
>  	LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme),
> +	LSM_HOOK_INIT(unix_stream_connect, hook_unix_stream_connect),
> +	LSM_HOOK_INIT(unix_may_send, hook_unix_may_send),
>  };
>  
>  __init void landlock_add_task_hooks(void)
> -- 
> 2.34.1
> 
>