Re: [PATCH] landlock: Add abstract unix socket connect restrictions

On 3/28/2024 1:07 PM, TaheraFahimi wrote:
> Abstract unix sockets are used for local interprocess communication without
> relying on filesystem. Since landlock has no restriction for connecting to
> a UNIX socket in the abstract namespace, a sandboxed process can connect to
> a socket outside the sandboxed environment. Access to such sockets should
> be scoped the same way ptrace access is limited.
>
> For a landlocked process to be allowed to connect to a target process, it
> must have a subset of the target process’s rules (the connecting socket
> must be in a sub-domain of the listening socket). This patch adds a new
> LSM hook for connect function in unix socket with the related access rights.
>
> Signed-off-by: Tahera Fahimi <[email protected]>
> ---
>  security/landlock/task.c | 70 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 849f5123610b..7f4155fc6174 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c
> @@ -13,6 +13,7 @@
>  #include <linux/lsm_hooks.h>
>  #include <linux/rcupdate.h>
>  #include <linux/sched.h>
> +#include <net/sock.h>
>  
>  #include "common.h"
>  #include "cred.h"
> @@ -108,9 +109,78 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
>  	return task_ptrace(parent, current);
>  }
>  
> +static const struct cred *sk_get_cred(struct sock *sk)
> +{
> +	const struct cred *cred = get_cred(sk->sk_peer_cred);
> +
> +	if (!cred)
> +		return NULL;

This makes no sense. If cred is NULL, why not just return it?

> +	return cred;
> +}

This function devolves into a call to get_cred(sk->sk_peer_cred).
What value does it add?

> +
> +static const struct landlock_ruleset *get_current_sock_domain(void)
> +{
> +	const struct landlock_ruleset *const dom =
> +		landlock_get_current_domain();
> +
> +	if (!dom)
> +		return NULL;
> +
> +	return dom;
> +}

Same here. Just return landlock_get_current_domain().

> +
> +static bool unix_sock_is_scoped(struct sock *const sock,
> +				struct sock *const other)
> +{
> +	bool is_scoped = true;
> +
> +	/* get the ruleset of connecting sock*/
> +	const struct landlock_ruleset *const dom_sock =
> +		get_current_sock_domain();
> +
> +	if (!dom_sock)
> +		return true;
> +
> +	/* get credential of listening sock*/
> +	const struct cred *cred_other = sk_get_cred(other);
> +
> +	if (!cred_other)
> +		return true;
> +
> +	/* retrieve the landlock_rulesets*/
> +	const struct landlock_ruleset *dom_parent;
> +
> +	rcu_read_lock();
> +	dom_parent = landlock_cred(cred_other)->domain;
> +	is_scoped = domain_scope_le(dom_parent, dom_sock);
> +	rcu_read_unlock();
> +
> +	return is_scoped;
> +}
> +
> +static int task_unix_stream_connect(struct sock *const sock,
> +				    struct sock *const other,
> +				    struct sock *const newsk)
> +{
> +	if (unix_sock_is_scoped(sock, other))
> +		return 0;
> +	return -EPERM;
> +}

Again, a function that does nothing but wrap another function
adds no value and consumes stack and processing resources.

> +
> +/**
> + * hook_unix_stream_connect
> + */
> +static int hook_unix_stream_connect(struct sock *const sock,
> +				    struct sock *const other,
> +				    struct sock *const newsk)
> +{
> +	return task_unix_stream_connect(sock, other, newsk);
> +}
> +
>  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),
>  };
>  
>  __init void landlock_add_task_hooks(void)