2024-03-28 20:07:15

by Tahera Fahimi

[permalink] [raw]
Subject: [PATCH] landlock: Add abstract unix socket connect restrictions

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;
+ return cred;
+}
+
+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;
+}
+
+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;
+}
+
+/**
+ * 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)
--
2.34.1



2024-03-28 20:43:11

by Casey Schaufler

[permalink] [raw]
Subject: 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)