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>
---
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 | 30 +++++++
security/landlock/limits.h | 3 +
security/landlock/ruleset.c | 7 +-
security/landlock/ruleset.h | 23 ++++-
security/landlock/syscalls.c | 14 ++-
security/landlock/task.c | 155 ++++++++++++++++++++++++++++++++++
6 files changed, 225 insertions(+), 7 deletions(-)
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 68625e728f43..ab31e9f53e55 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 scopes (cf. `Scope flags`_)
+ * restricting a Landlock domain from accessing outside
+ * resources(e.g. IPCs).
+ */
+ __u64 scoped;
};
/*
@@ -266,4 +272,28 @@ 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
+ *
+ * scoped attribute handles a set of restrictions on kernel IPCs through
+ * the following flags.
+ *
+ * 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)
+/* 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 */
#endif /* _SECURITY_LANDLOCK_LIMITS_H */
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 6ff232f58618..a93bdbf52fff 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -52,12 +52,13 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
struct landlock_ruleset *
landlock_create_ruleset(const access_mask_t fs_access_mask,
- const access_mask_t net_access_mask)
+ const access_mask_t net_access_mask,
+ const access_mask_t scope_mask)
{
struct landlock_ruleset *new_ruleset;
/* Informs about useless ruleset. */
- if (!fs_access_mask && !net_access_mask)
+ if (!fs_access_mask && !net_access_mask && !scope_mask)
return ERR_PTR(-ENOMSG);
new_ruleset = create_ruleset(1);
if (IS_ERR(new_ruleset))
@@ -66,6 +67,8 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
if (net_access_mask)
landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
+ if (scope_mask)
+ landlock_add_scope_mask(new_ruleset, scope_mask, 0);
return new_ruleset;
}
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index 0f1b5b4c8f6b..c749fa0b3ecd 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -35,6 +35,8 @@ typedef u16 access_mask_t;
static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
/* Makes sure all network access rights can be stored. */
static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
+/* Makes sure all scoped rights can be stored*/
+static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
/* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
@@ -42,6 +44,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
struct access_masks {
access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
+ access_mask_t scoped : LANDLOCK_NUM_SCOPE;
};
typedef u16 layer_mask_t;
@@ -233,7 +236,8 @@ struct landlock_ruleset {
struct landlock_ruleset *
landlock_create_ruleset(const access_mask_t access_mask_fs,
- const access_mask_t access_mask_net);
+ const access_mask_t access_mask_net,
+ const access_mask_t scope_mask);
void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
@@ -280,6 +284,16 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
ruleset->access_masks[layer_level].net |= net_mask;
}
+static inline void
+landlock_add_scope_mask(struct landlock_ruleset *const ruleset,
+ const access_mask_t scope_mask, const u16 layer_level)
+{
+ access_mask_t scoped_mask = scope_mask & LANDLOCK_MASK_SCOPE;
+
+ WARN_ON_ONCE(scope_mask != scoped_mask);
+ ruleset->access_masks[layer_level].scoped |= scoped_mask;
+}
+
static inline access_mask_t
landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
const u16 layer_level)
@@ -303,6 +317,13 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
return ruleset->access_masks[layer_level].net;
}
+static inline access_mask_t
+landlock_get_scope_mask(const struct landlock_ruleset *const ruleset,
+ const u16 layer_level)
+{
+ return ruleset->access_masks[layer_level].scoped;
+}
+
bool landlock_unmask_layers(const struct landlock_rule *const rule,
const access_mask_t access_request,
layer_mask_t (*const layer_masks)[],
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 03b470f5a85a..f51b29521d6b 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -97,8 +97,9 @@ static void build_check_abi(void)
*/
ruleset_size = sizeof(ruleset_attr.handled_access_fs);
ruleset_size += sizeof(ruleset_attr.handled_access_net);
+ ruleset_size += sizeof(ruleset_attr.scoped);
BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
- BUILD_BUG_ON(sizeof(ruleset_attr) != 16);
+ BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
path_beneath_size = sizeof(path_beneath_attr.allowed_access);
path_beneath_size += sizeof(path_beneath_attr.parent_fd);
@@ -149,7 +150,7 @@ static const struct file_operations ruleset_fops = {
.write = fop_dummy_write,
};
-#define LANDLOCK_ABI_VERSION 5
+#define LANDLOCK_ABI_VERSION 6
/**
* sys_landlock_create_ruleset - Create a new ruleset
@@ -170,7 +171,7 @@ static const struct file_operations ruleset_fops = {
* Possible returned errors are:
*
* - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
- * - %EINVAL: unknown @flags, or unknown access, or too small @size;
+ * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size;
* - %E2BIG or %EFAULT: @attr or @size inconsistencies;
* - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs.
*/
@@ -213,9 +214,14 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
LANDLOCK_MASK_ACCESS_NET)
return -EINVAL;
+ /* Checks IPC scoping content (and 32-bits cast). */
+ if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
+ return -EINVAL;
+
/* Checks arguments and transforms to kernel struct. */
ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
- ruleset_attr.handled_access_net);
+ ruleset_attr.handled_access_net,
+ ruleset_attr.scoped);
if (IS_ERR(ruleset))
return PTR_ERR(ruleset);
diff --git a/security/landlock/task.c b/security/landlock/task.c
index 849f5123610b..7e8579ebae83 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,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
return task_ptrace(parent, current);
}
+static bool walk_and_check(const struct landlock_ruleset *const child,
+ struct landlock_hierarchy **walker,
+ size_t base_layer, size_t deep_layer,
+ access_mask_t check_scoping)
+{
+ if (!child || base_layer < 0 || !(*walker))
+ return false;
+
+ for (deep_layer; base_layer < deep_layer; deep_layer--) {
+ if (check_scoping & landlock_get_scope_mask(child, deep_layer))
+ return false;
+ *walker = (*walker)->parent;
+ if (WARN_ON_ONCE(!*walker))
+ /* there is an inconsistency between num_layers
+ * and landlock_hierarchy in the ruleset
+ */
+ return false;
+ }
+ return true;
+}
+
+/**
+ * domain_IPC_scope - Checks if the client domain is scoped in the same
+ * domain as the server.
+ *
+ * @client: IPC sender domain.
+ * @server: IPC receiver domain.
+ *
+ * Check if the @client domain is scoped to access the @server; the @server
+ * must be scoped in the same domain.
+ */
+static bool domain_IPC_scope(const struct landlock_ruleset *const client,
+ const struct landlock_ruleset *const server,
+ access_mask_t ipc_type)
+{
+ size_t client_layer, server_layer = 0;
+ int base_layer;
+ struct landlock_hierarchy *client_walker, *server_walker;
+ bool is_scoped;
+
+ /* Quick return if client has no domain */
+ if (!client)
+ return true;
+
+ client_layer = client->num_layers - 1;
+ client_walker = client->hierarchy;
+ if (server) {
+ server_layer = server->num_layers - 1;
+ server_walker = server->hierarchy;
+ }
+ base_layer = (client_layer > server_layer) ? server_layer :
+ client_layer;
+
+ /* For client domain, walk_and_check ensures the client domain is
+ * not scoped until gets to base_layer.
+ * For server_domain, it only ensures that the server domain exist.
+ */
+ if (client_layer != server_layer) {
+ if (client_layer > server_layer)
+ is_scoped = walk_and_check(client, &client_walker,
+ server_layer, client_layer,
+ ipc_type);
+ else
+ is_scoped = walk_and_check(server, &server_walker,
+ client_layer, server_layer,
+ ipc_type & 0);
+ if (!is_scoped)
+ return false;
+ }
+ /* client and server are at the same level in hierarchy. If client is
+ * scoped, the server must be scoped in the same domain
+ */
+ for (base_layer; base_layer >= 0; base_layer--) {
+ if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
+ /* This check must be here since access would be denied only if
+ * the client is scoped and the server has no domain, so
+ * if the client has a domain but is not scoped and the server
+ * has no domain, access is guaranteed.
+ */
+ if (!server)
+ return false;
+
+ if (server_walker == client_walker)
+ return true;
+
+ return false;
+ }
+ client_walker = client_walker->parent;
+ server_walker = server_walker->parent;
+ /* Warn if there is an incosistenncy between num_layers and
+ * landlock_hierarchy in each of rulesets
+ */
+ if (WARN_ON_ONCE(base_layer > 0 &&
+ (!server_walker || !client_walker)))
+ return false;
+ }
+ 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;
+ return domain_IPC_scope(dom, dom_other,
+ LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET);
+}
+
+static bool check_unix_address_format(struct sock *const sock)
+{
+ struct unix_address *addr = unix_sk(sock)->addr;
+
+ if (!addr)
+ return true;
+
+ if (addr->len > sizeof(AF_UNIX)) {
+ /* handling unspec sockets */
+ if (!addr->name[0].sun_path)
+ return true;
+
+ if (addr->name[0].sun_path[0] == '\0')
+ if (!sock_is_scoped(sock))
+ return false;
+ }
+
+ return true;
+}
+
+static int hook_unix_stream_connect(struct sock *const sock,
+ struct sock *const other,
+ struct sock *const newsk)
+{
+ if (check_unix_address_format(other))
+ return 0;
+
+ return -EPERM;
+}
+
+static int hook_unix_may_send(struct socket *const sock,
+ struct socket *const other)
+{
+ if (check_unix_address_format(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
On Fri, Aug 2, 2024 at 6:03 AM Tahera Fahimi <fahimitahera@gmail.com> 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.
[...]
> +static bool check_unix_address_format(struct sock *const sock)
> +{
> + struct unix_address *addr = unix_sk(sock)->addr;
> +
> + if (!addr)
> + return true;
> +
> + if (addr->len > sizeof(AF_UNIX)) {
> + /* handling unspec sockets */
> + if (!addr->name[0].sun_path)
> + return true;
addr->name[0] is a "struct sockaddr_un", whose member "sun_path" is an
array member, not a pointer. If "addr" is a valid pointer,
"addr->name[0].sun_path" can't be NULL.
> + if (addr->name[0].sun_path[0] == '\0')
> + if (!sock_is_scoped(sock))
> + return false;
> + }
> +
> + return true;
> +}
On Thu, Aug 01, 2024 at 10:02:33PM -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>
>
> ---
> 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 | 30 +++++++
> security/landlock/limits.h | 3 +
> security/landlock/ruleset.c | 7 +-
> security/landlock/ruleset.h | 23 ++++-
> security/landlock/syscalls.c | 14 ++-
> security/landlock/task.c | 155 ++++++++++++++++++++++++++++++++++
> 6 files changed, 225 insertions(+), 7 deletions(-)
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 849f5123610b..7e8579ebae83 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,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> return task_ptrace(parent, current);
> }
>
> +static bool walk_and_check(const struct landlock_ruleset *const child,
> + struct landlock_hierarchy **walker,
> + size_t base_layer, size_t deep_layer,
> + access_mask_t check_scoping)
s/check_scoping/scope/
> +{
> + if (!child || base_layer < 0 || !(*walker))
I guess it should be:
WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))
> + return false;
> +
> + for (deep_layer; base_layer < deep_layer; deep_layer--) {
No need to pass deep_layer as argument:
deep_layer = child->num_layers - 1
> + if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> + return false;
> + *walker = (*walker)->parent;
> + if (WARN_ON_ONCE(!*walker))
> + /* there is an inconsistency between num_layers
Please use full sentences starting with a capital letter and ending with
a dot, and in this case start with "/*"
> + * and landlock_hierarchy in the ruleset
> + */
> + return false;
> + }
> + return true;
> +}
> +
> +/**
> + * domain_IPC_scope - Checks if the client domain is scoped in the same
> + * domain as the server.
Actually, you can remove IPC from the function name.
> + *
> + * @client: IPC sender domain.
> + * @server: IPC receiver domain.
> + *
> + * Check if the @client domain is scoped to access the @server; the @server
> + * must be scoped in the same domain.
Returns true if...
> + */
> +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> + const struct landlock_ruleset *const server,
> + access_mask_t ipc_type)
> +{
> + size_t client_layer, server_layer = 0;
> + int base_layer;
> + struct landlock_hierarchy *client_walker, *server_walker;
> + bool is_scoped;
> +
> + /* Quick return if client has no domain */
> + if (!client)
> + return true;
> +
> + client_layer = client->num_layers - 1;
> + client_walker = client->hierarchy;
> + if (server) {
> + server_layer = server->num_layers - 1;
> + server_walker = server->hierarchy;
> + }
} else {
server_layer = 0;
server_walker = NULL;
}
> + base_layer = (client_layer > server_layer) ? server_layer :
> + client_layer;
> +
> + /* For client domain, walk_and_check ensures the client domain is
> + * not scoped until gets to base_layer.
until gets?
> + * For server_domain, it only ensures that the server domain exist.
> + */
> + if (client_layer != server_layer) {
bool is_scoped;
> + if (client_layer > server_layer)
> + is_scoped = walk_and_check(client, &client_walker,
> + server_layer, client_layer,
> + ipc_type);
> + else
server_walker may be uninitialized and still read here, and maybe later
in the for loop. The whole code should maks sure this cannot happen,
and a test case should check this.
> + is_scoped = walk_and_check(server, &server_walker,
> + client_layer, server_layer,
> + ipc_type & 0);
"ipc_type & 0" is the same as "0"
> + if (!is_scoped)
The name doesn't reflect the semantic. walk_and_check() should return
the inverse.
> + return false;
> + }
This code would be simpler:
if (client_layer > server_layer) {
base_layer = server_layer;
// TODO: inverse boolean logic
if (!walk_and_check(client, &client_walker,
base_layer, ipc_type))
return false;
} else (client_layer < server_layer) {
base_layer = client_layer;
// TODO: inverse boolean logic
if (!walk_and_check(server, &server_walker,
base_layer, 0))
return false;
} else {
base_layer = client_layer;
}
I think we can improve more to make sure there is no path/risk of
inconsistent pointers.
> + /* client and server are at the same level in hierarchy. If client is
> + * scoped, the server must be scoped in the same domain
> + */
> + for (base_layer; base_layer >= 0; base_layer--) {
> + if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
With each multi-line comment, the first line should be empty:
/*
* This check must be here since access would be denied only if
> + /* This check must be here since access would be denied only if
> + * the client is scoped and the server has no domain, so
> + * if the client has a domain but is not scoped and the server
> + * has no domain, access is guaranteed.
> + */
> + if (!server)
> + return false;
> +
> + if (server_walker == client_walker)
> + return true;
> +
> + return false;
> + }
> + client_walker = client_walker->parent;
> + server_walker = server_walker->parent;
> + /* Warn if there is an incosistenncy between num_layers and
Makes sure there is no inconsistency between num_layers and
> + * landlock_hierarchy in each of rulesets
> + */
> + if (WARN_ON_ONCE(base_layer > 0 &&
> + (!server_walker || !client_walker)))
> + return false;
> + }
> + return true;
> +}
On Sat, Aug 03, 2024 at 01:29:04PM +0200, Mickaël Salaün wrote:
> On Thu, Aug 01, 2024 at 10:02:33PM -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>
> >
> > ---
> > 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 | 30 +++++++
> > security/landlock/limits.h | 3 +
> > security/landlock/ruleset.c | 7 +-
> > security/landlock/ruleset.h | 23 ++++-
> > security/landlock/syscalls.c | 14 ++-
> > security/landlock/task.c | 155 ++++++++++++++++++++++++++++++++++
> > 6 files changed, 225 insertions(+), 7 deletions(-)
>
> > diff --git a/security/landlock/task.c b/security/landlock/task.c
> > index 849f5123610b..7e8579ebae83 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,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> > return task_ptrace(parent, current);
> > }
> >
> > +static bool walk_and_check(const struct landlock_ruleset *const child,
> > + struct landlock_hierarchy **walker,
> > + size_t base_layer, size_t deep_layer,
> > + access_mask_t check_scoping)
>
> s/check_scoping/scope/
>
> > +{
> > + if (!child || base_layer < 0 || !(*walker))
>
> I guess it should be:
> WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))
>
> > + return false;
> > +
> > + for (deep_layer; base_layer < deep_layer; deep_layer--) {
>
> No need to pass deep_layer as argument:
> deep_layer = child->num_layers - 1
>
> > + if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> > + return false;
> > + *walker = (*walker)->parent;
> > + if (WARN_ON_ONCE(!*walker))
> > + /* there is an inconsistency between num_layers
>
> Please use full sentences starting with a capital letter and ending with
> a dot, and in this case start with "/*"
>
> > + * and landlock_hierarchy in the ruleset
> > + */
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > +/**
> > + * domain_IPC_scope - Checks if the client domain is scoped in the same
> > + * domain as the server.
>
> Actually, you can remove IPC from the function name.
>
> > + *
> > + * @client: IPC sender domain.
> > + * @server: IPC receiver domain.
> > + *
> > + * Check if the @client domain is scoped to access the @server; the @server
> > + * must be scoped in the same domain.
>
> Returns true if...
>
> > + */
> > +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> > + const struct landlock_ruleset *const server,
> > + access_mask_t ipc_type)
> > +{
> > + size_t client_layer, server_layer = 0;
> > + int base_layer;
> > + struct landlock_hierarchy *client_walker, *server_walker;
> > + bool is_scoped;
> > +
> > + /* Quick return if client has no domain */
> > + if (!client)
> > + return true;
> > +
> > + client_layer = client->num_layers - 1;
> > + client_walker = client->hierarchy;
> > + if (server) {
> > + server_layer = server->num_layers - 1;
> > + server_walker = server->hierarchy;
> > + }
>
> } else {
> server_layer = 0;
> server_walker = NULL;
> }
>
> > + base_layer = (client_layer > server_layer) ? server_layer :
> > + client_layer;
> > +
> > + /* For client domain, walk_and_check ensures the client domain is
> > + * not scoped until gets to base_layer.
>
> until gets?
>
> > + * For server_domain, it only ensures that the server domain exist.
> > + */
> > + if (client_layer != server_layer) {
>
> bool is_scoped;
It is defined above.
> > + if (client_layer > server_layer)
> > + is_scoped = walk_and_check(client, &client_walker,
> > + server_layer, client_layer,
> > + ipc_type);
> > + else
>
> server_walker may be uninitialized and still read here, and maybe later
> in the for loop. The whole code should maks sure this cannot happen,
> and a test case should check this.
I think this case never happens, since the server_walker can be read
here if there are more than one layers in server domain which means that
the server_walker is not unintialized.
> > + is_scoped = walk_and_check(server, &server_walker,
> > + client_layer, server_layer,
> > + ipc_type & 0);
>
> "ipc_type & 0" is the same as "0"
>
> > + if (!is_scoped)
>
> The name doesn't reflect the semantic. walk_and_check() should return
> the inverse.
>
> > + return false;
> > + }
>
> This code would be simpler:
>
> if (client_layer > server_layer) {
> base_layer = server_layer;
> // TODO: inverse boolean logic
> if (!walk_and_check(client, &client_walker,
> base_layer, ipc_type))
> return false;
> } else (client_layer < server_layer) {
> base_layer = client_layer;
> // TODO: inverse boolean logic
> if (!walk_and_check(server, &server_walker,
> base_layer, 0))
> return false;
> } else {
> base_layer = client_layer;
> }
>
>
> I think we can improve more to make sure there is no path/risk of
> inconsistent pointers.
>
>
> > + /* client and server are at the same level in hierarchy. If client is
> > + * scoped, the server must be scoped in the same domain
> > + */
> > + for (base_layer; base_layer >= 0; base_layer--) {
> > + if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
>
> With each multi-line comment, the first line should be empty:
> /*
> * This check must be here since access would be denied only if
>
> > + /* This check must be here since access would be denied only if
> > + * the client is scoped and the server has no domain, so
> > + * if the client has a domain but is not scoped and the server
> > + * has no domain, access is guaranteed.
> > + */
> > + if (!server)
> > + return false;
> > +
> > + if (server_walker == client_walker)
> > + return true;
> > +
> > + return false;
> > + }
> > + client_walker = client_walker->parent;
> > + server_walker = server_walker->parent;
> > + /* Warn if there is an incosistenncy between num_layers and
>
> Makes sure there is no inconsistency between num_layers and
>
>
> > + * landlock_hierarchy in each of rulesets
> > + */
> > + if (WARN_ON_ONCE(base_layer > 0 &&
> > + (!server_walker || !client_walker)))
> > + return false;
> > + }
> > + return true;
> > +}
On Wed, Aug 07, 2024 at 09:37:15AM -0600, Tahera Fahimi wrote:
> On Sat, Aug 03, 2024 at 01:29:04PM +0200, Mickaël Salaün wrote:
> > On Thu, Aug 01, 2024 at 10:02:33PM -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>
> > >
> > > ---
> > > 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 | 30 +++++++
> > > security/landlock/limits.h | 3 +
> > > security/landlock/ruleset.c | 7 +-
> > > security/landlock/ruleset.h | 23 ++++-
> > > security/landlock/syscalls.c | 14 ++-
> > > security/landlock/task.c | 155 ++++++++++++++++++++++++++++++++++
> > > 6 files changed, 225 insertions(+), 7 deletions(-)
> >
> > > diff --git a/security/landlock/task.c b/security/landlock/task.c
> > > index 849f5123610b..7e8579ebae83 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,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> > > return task_ptrace(parent, current);
> > > }
> > >
> > > +static bool walk_and_check(const struct landlock_ruleset *const child,
> > > + struct landlock_hierarchy **walker,
> > > + size_t base_layer, size_t deep_layer,
> > > + access_mask_t check_scoping)
> >
> > s/check_scoping/scope/
> >
> > > +{
> > > + if (!child || base_layer < 0 || !(*walker))
> >
> > I guess it should be:
> > WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))
> >
> > > + return false;
> > > +
> > > + for (deep_layer; base_layer < deep_layer; deep_layer--) {
> >
> > No need to pass deep_layer as argument:
> > deep_layer = child->num_layers - 1
> >
> > > + if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> > > + return false;
> > > + *walker = (*walker)->parent;
> > > + if (WARN_ON_ONCE(!*walker))
> > > + /* there is an inconsistency between num_layers
> >
> > Please use full sentences starting with a capital letter and ending with
> > a dot, and in this case start with "/*"
> >
> > > + * and landlock_hierarchy in the ruleset
> > > + */
> > > + return false;
> > > + }
> > > + return true;
> > > +}
> > > +
> > > +/**
> > > + * domain_IPC_scope - Checks if the client domain is scoped in the same
> > > + * domain as the server.
> >
> > Actually, you can remove IPC from the function name.
> >
> > > + *
> > > + * @client: IPC sender domain.
> > > + * @server: IPC receiver domain.
> > > + *
> > > + * Check if the @client domain is scoped to access the @server; the @server
> > > + * must be scoped in the same domain.
> >
> > Returns true if...
> >
> > > + */
> > > +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> > > + const struct landlock_ruleset *const server,
> > > + access_mask_t ipc_type)
> > > +{
> > > + size_t client_layer, server_layer = 0;
> > > + int base_layer;
> > > + struct landlock_hierarchy *client_walker, *server_walker;
> > > + bool is_scoped;
> > > +
> > > + /* Quick return if client has no domain */
> > > + if (!client)
> > > + return true;
> > > +
> > > + client_layer = client->num_layers - 1;
> > > + client_walker = client->hierarchy;
> > > + if (server) {
> > > + server_layer = server->num_layers - 1;
> > > + server_walker = server->hierarchy;
> > > + }
> >
> > } else {
> > server_layer = 0;
> > server_walker = NULL;
> > }
> >
> > > + base_layer = (client_layer > server_layer) ? server_layer :
> > > + client_layer;
> > > +
> > > + /* For client domain, walk_and_check ensures the client domain is
> > > + * not scoped until gets to base_layer.
> >
> > until gets?
> >
> > > + * For server_domain, it only ensures that the server domain exist.
> > > + */
> > > + if (client_layer != server_layer) {
> >
> > bool is_scoped;
> It is defined above.
Yes, but it should be defined here because it is not used outside of
this block.
> > > + if (client_layer > server_layer)
> > > + is_scoped = walk_and_check(client, &client_walker,
> > > + server_layer, client_layer,
> > > + ipc_type);
> > > + else
> >
> > server_walker may be uninitialized and still read here, and maybe later
> > in the for loop. The whole code should maks sure this cannot happen,
> > and a test case should check this.
> I think this case never happens, since the server_walker can be read
> here if there are more than one layers in server domain which means that
> the server_walker is not unintialized.
Yes, but this code makes it more difficult to convince yourself. The
proposed refactoring should help.
>
> > > + is_scoped = walk_and_check(server, &server_walker,
> > > + client_layer, server_layer,
> > > + ipc_type & 0);
> >
> > "ipc_type & 0" is the same as "0"
> >
> > > + if (!is_scoped)
> >
> > The name doesn't reflect the semantic. walk_and_check() should return
> > the inverse.
> >
> > > + return false;
> > > + }
> >
> > This code would be simpler:
> >
> > if (client_layer > server_layer) {
> > base_layer = server_layer;
> > // TODO: inverse boolean logic
> > if (!walk_and_check(client, &client_walker,
> > base_layer, ipc_type))
> > return false;
> > } else (client_layer < server_layer) {
> > base_layer = client_layer;
> > // TODO: inverse boolean logic
> > if (!walk_and_check(server, &server_walker,
> > base_layer, 0))
> > return false;
> > } else {
> > base_layer = client_layer;
> > }
> >
> >
> > I think we can improve more to make sure there is no path/risk of
> > inconsistent pointers.
> >
> >
> > > + /* client and server are at the same level in hierarchy. If client is
> > > + * scoped, the server must be scoped in the same domain
> > > + */
> > > + for (base_layer; base_layer >= 0; base_layer--) {
> > > + if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
> >
> > With each multi-line comment, the first line should be empty:
> > /*
> > * This check must be here since access would be denied only if
> >
> > > + /* This check must be here since access would be denied only if
> > > + * the client is scoped and the server has no domain, so
> > > + * if the client has a domain but is not scoped and the server
> > > + * has no domain, access is guaranteed.
> > > + */
> > > + if (!server)
> > > + return false;
> > > +
> > > + if (server_walker == client_walker)
> > > + return true;
> > > +
> > > + return false;
> > > + }
> > > + client_walker = client_walker->parent;
> > > + server_walker = server_walker->parent;
> > > + /* Warn if there is an incosistenncy between num_layers and
> >
> > Makes sure there is no inconsistency between num_layers and
> >
> >
> > > + * landlock_hierarchy in each of rulesets
> > > + */
> > > + if (WARN_ON_ONCE(base_layer > 0 &&
> > > + (!server_walker || !client_walker)))
> > > + return false;
> > > + }
> > > + return true;
> > > +}
>
On Sat, Aug 03, 2024 at 01:29:09PM +0200, Mickaël Salaün wrote:
> On Thu, Aug 01, 2024 at 10:02:33PM -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>
> >
> > ---
> > 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 | 30 +++++++
> > security/landlock/limits.h | 3 +
> > security/landlock/ruleset.c | 7 +-
> > security/landlock/ruleset.h | 23 ++++-
> > security/landlock/syscalls.c | 14 ++-
> > security/landlock/task.c | 155 ++++++++++++++++++++++++++++++++++
> > 6 files changed, 225 insertions(+), 7 deletions(-)
>
> > diff --git a/security/landlock/task.c b/security/landlock/task.c
> > index 849f5123610b..7e8579ebae83 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,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> > return task_ptrace(parent, current);
> > }
> >
> > +static bool walk_and_check(const struct landlock_ruleset *const child,
> > + struct landlock_hierarchy **walker,
> > + size_t base_layer, size_t deep_layer,
> > + access_mask_t check_scoping)
>
> s/check_scoping/scope/
>
> > +{
> > + if (!child || base_layer < 0 || !(*walker))
>
> I guess it should be:
> WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))
>
> > + return false;
> > +
> > + for (deep_layer; base_layer < deep_layer; deep_layer--) {
>
> No need to pass deep_layer as argument:
> deep_layer = child->num_layers - 1
>
> > + if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> > + return false;
> > + *walker = (*walker)->parent;
> > + if (WARN_ON_ONCE(!*walker))
> > + /* there is an inconsistency between num_layers
>
> Please use full sentences starting with a capital letter and ending with
> a dot, and in this case start with "/*"
>
> > + * and landlock_hierarchy in the ruleset
> > + */
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > +/**
> > + * domain_IPC_scope - Checks if the client domain is scoped in the same
> > + * domain as the server.
>
> Actually, you can remove IPC from the function name.
>
> > + *
> > + * @client: IPC sender domain.
> > + * @server: IPC receiver domain.
> > + *
> > + * Check if the @client domain is scoped to access the @server; the @server
> > + * must be scoped in the same domain.
>
> Returns true if...
>
> > + */
> > +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> > + const struct landlock_ruleset *const server,
> > + access_mask_t ipc_type)
> > +{
> > + size_t client_layer, server_layer = 0;
> > + int base_layer;
> > + struct landlock_hierarchy *client_walker, *server_walker;
> > + bool is_scoped;
> > +
> > + /* Quick return if client has no domain */
> > + if (!client)
> > + return true;
> > +
> > + client_layer = client->num_layers - 1;
> > + client_walker = client->hierarchy;
> > + if (server) {
> > + server_layer = server->num_layers - 1;
> > + server_walker = server->hierarchy;
> > + }
>
> } else {
> server_layer = 0;
> server_walker = NULL;
> }
>
> > + base_layer = (client_layer > server_layer) ? server_layer :
> > + client_layer;
> > +
> > + /* For client domain, walk_and_check ensures the client domain is
> > + * not scoped until gets to base_layer.
>
> until gets?
>
> > + * For server_domain, it only ensures that the server domain exist.
> > + */
> > + if (client_layer != server_layer) {
>
> bool is_scoped;
>
> > + if (client_layer > server_layer)
> > + is_scoped = walk_and_check(client, &client_walker,
> > + server_layer, client_layer,
> > + ipc_type);
> > + else
>
> server_walker may be uninitialized and still read here, and maybe later
> in the for loop. The whole code should maks sure this cannot happen,
> and a test case should check this.
>
> > + is_scoped = walk_and_check(server, &server_walker,
> > + client_layer, server_layer,
> > + ipc_type & 0);
>
> "ipc_type & 0" is the same as "0"
>
> > + if (!is_scoped)
>
> The name doesn't reflect the semantic. walk_and_check() should return
> the inverse.
>
> > + return false;
> > + }
>
> This code would be simpler:
>
> if (client_layer > server_layer) {
> base_layer = server_layer;
> // TODO: inverse boolean logic
> if (!walk_and_check(client, &client_walker,
> base_layer, ipc_type))
> return false;
> } else (client_layer < server_layer) {
> base_layer = client_layer;
> // TODO: inverse boolean logic
> if (!walk_and_check(server, &server_walker,
> base_layer, 0))
> return false;
> } else {
> base_layer = client_layer;
> }
>
>
> I think we can improve more to make sure there is no path/risk of
> inconsistent pointers.
>
>
> > + /* client and server are at the same level in hierarchy. If client is
> > + * scoped, the server must be scoped in the same domain
> > + */
> > + for (base_layer; base_layer >= 0; base_layer--) {
> > + if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
>
> With each multi-line comment, the first line should be empty:
> /*
> * This check must be here since access would be denied only if
>
> > + /* This check must be here since access would be denied only if
> > + * the client is scoped and the server has no domain, so
> > + * if the client has a domain but is not scoped and the server
> > + * has no domain, access is guaranteed.
> > + */
> > + if (!server)
> > + return false;
> > +
> > + if (server_walker == client_walker)
> > + return true;
> > +
> > + return false;
> > + }
> > + client_walker = client_walker->parent;
> > + server_walker = server_walker->parent;
> > + /* Warn if there is an incosistenncy between num_layers and
>
> Makes sure there is no inconsistency between num_layers and
>
>
> > + * landlock_hierarchy in each of rulesets
> > + */
> > + if (WARN_ON_ONCE(base_layer > 0 &&
> > + (!server_walker || !client_walker)))
> > + return false;
> > + }
> > + return true;
> > +}
Here is a refactoring that is easier to read and avoid potential pointer
misuse:
static bool domain_is_scoped(const struct landlock_ruleset *const client,
const struct landlock_ruleset *const server,
access_mask_t scope)
{
int client_layer, server_layer;
struct landlock_hierarchy *client_walker, *server_walker;
if (WARN_ON_ONCE(!client))
return false;
client_layer = client->num_layers - 1;
client_walker = client->hierarchy;
/*
* client_layer must be a signed integer with greater capacity than
* client->num_layers to ensure the following loop stops.
*/
BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
if (!server) {
/*
* Walks client's parent domains and checks that none of these
* domains are scoped.
*/
for (; client_layer >= 0; client_layer--) {
if (landlock_get_scope_mask(client, client_layer) &
scope)
return true;
}
return false;
}
server_layer = server->num_layers - 1;
server_walker = server->hierarchy;
/*
* Walks client's parent domains down to the same hierarchy level as
* the server's domain, and checks that none of these client's parent
* domains are scoped.
*/
for (; client_layer > server_layer; client_layer--) {
if (landlock_get_scope_mask(client, client_layer) & scope)
return true;
client_walker = client_walker->parent;
}
/*
* Walks server's parent domains down to the same hierarchy level as
* the client's domain.
*/
for (; server_layer > client_layer; server_layer--)
server_walker = server_walker->parent;
for (; client_layer >= 0; client_layer--) {
if (landlock_get_scope_mask(client, client_layer) & scope) {
/*
* Client and server are at the same level in the
* hierarchy. If the client is scoped, the request is
* only allowed if this domain is also a server's
* ancestor.
*/
if (server_walker == client_walker)
return false;
return true;
}
client_walker = client_walker->parent;
server_walker = server_walker->parent;
}
return false;
}
On Tue, Aug 6, 2024 at 9:36 PM Mickaël Salaün <mic@digikod.net> wrote:
> On Sat, Aug 03, 2024 at 01:29:09PM +0200, Mickaël Salaün wrote:
> > On Thu, Aug 01, 2024 at 10:02:33PM -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.
[...]
> Here is a refactoring that is easier to read and avoid potential pointer
> misuse:
>
> static bool domain_is_scoped(const struct landlock_ruleset *const client,
> const struct landlock_ruleset *const server,
> access_mask_t scope)
> {
> int client_layer, server_layer;
> struct landlock_hierarchy *client_walker, *server_walker;
>
> if (WARN_ON_ONCE(!client))
> return false;
>
> client_layer = client->num_layers - 1;
> client_walker = client->hierarchy;
>
> /*
> * client_layer must be a signed integer with greater capacity than
> * client->num_layers to ensure the following loop stops.
> */
> BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
>
> if (!server) {
> /*
> * Walks client's parent domains and checks that none of these
> * domains are scoped.
> */
> for (; client_layer >= 0; client_layer--) {
> if (landlock_get_scope_mask(client, client_layer) &
> scope)
> return true;
> }
> return false;
> }
>
> server_layer = server->num_layers - 1;
> server_walker = server->hierarchy;
>
> /*
> * Walks client's parent domains down to the same hierarchy level as
> * the server's domain, and checks that none of these client's parent
> * domains are scoped.
> */
> for (; client_layer > server_layer; client_layer--) {
> if (landlock_get_scope_mask(client, client_layer) & scope)
> return true;
>
> client_walker = client_walker->parent;
> }
>
> /*
> * Walks server's parent domains down to the same hierarchy level as
> * the client's domain.
> */
> for (; server_layer > client_layer; server_layer--)
> server_walker = server_walker->parent;
>
> for (; client_layer >= 0; client_layer--) {
> if (landlock_get_scope_mask(client, client_layer) & scope) {
> /*
> * Client and server are at the same level in the
> * hierarchy. If the client is scoped, the request is
> * only allowed if this domain is also a server's
> * ancestor.
> */
> if (server_walker == client_walker)
> return false;
>
> return true;
> }
> client_walker = client_walker->parent;
> server_walker = server_walker->parent;
> }
> return false;
> }
I think adding something like this change on top of your code would
make it more concise (though this is entirely untested):
--- /tmp/a 2024-08-06 22:37:33.800158308 +0200
+++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
@@ -15,25 +15,12 @@
* client_layer must be a signed integer with greater capacity than
* client->num_layers to ensure the following loop stops.
*/
BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
- if (!server) {
- /*
- * Walks client's parent domains and checks that none of these
- * domains are scoped.
- */
- for (; client_layer >= 0; client_layer--) {
- if (landlock_get_scope_mask(client, client_layer) &
- scope)
- return true;
- }
- return false;
- }
-
- server_layer = server->num_layers - 1;
- server_walker = server->hierarchy;
+ server_layer = server ? (server->num_layers - 1) : -1;
+ server_walker = server ? server->hierarchy : NULL;
/*
* Walks client's parent domains down to the same hierarchy level as
* the server's domain, and checks that none of these client's parent
* domains are scoped.
On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> On Tue, Aug 6, 2024 at 9:36 PM Mickaël Salaün <mic@digikod.net> wrote:
> > On Sat, Aug 03, 2024 at 01:29:09PM +0200, Mickaël Salaün wrote:
> > > On Thu, Aug 01, 2024 at 10:02:33PM -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.
> [...]
> > Here is a refactoring that is easier to read and avoid potential pointer
> > misuse:
> >
> > static bool domain_is_scoped(const struct landlock_ruleset *const client,
> > const struct landlock_ruleset *const server,
> > access_mask_t scope)
> > {
> > int client_layer, server_layer;
> > struct landlock_hierarchy *client_walker, *server_walker;
> >
> > if (WARN_ON_ONCE(!client))
> > return false;
> >
> > client_layer = client->num_layers - 1;
> > client_walker = client->hierarchy;
> >
> > /*
> > * client_layer must be a signed integer with greater capacity than
> > * client->num_layers to ensure the following loop stops.
> > */
> > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> >
> > if (!server) {
> > /*
> > * Walks client's parent domains and checks that none of these
> > * domains are scoped.
> > */
> > for (; client_layer >= 0; client_layer--) {
> > if (landlock_get_scope_mask(client, client_layer) &
> > scope)
> > return true;
> > }
> > return false;
> > }
> >
> > server_layer = server->num_layers - 1;
> > server_walker = server->hierarchy;
> >
> > /*
> > * Walks client's parent domains down to the same hierarchy level as
> > * the server's domain, and checks that none of these client's parent
> > * domains are scoped.
> > */
> > for (; client_layer > server_layer; client_layer--) {
> > if (landlock_get_scope_mask(client, client_layer) & scope)
> > return true;
> >
> > client_walker = client_walker->parent;
> > }
> >
> > /*
> > * Walks server's parent domains down to the same hierarchy level as
> > * the client's domain.
> > */
> > for (; server_layer > client_layer; server_layer--)
> > server_walker = server_walker->parent;
> >
> > for (; client_layer >= 0; client_layer--) {
> > if (landlock_get_scope_mask(client, client_layer) & scope) {
> > /*
> > * Client and server are at the same level in the
> > * hierarchy. If the client is scoped, the request is
> > * only allowed if this domain is also a server's
> > * ancestor.
> > */
We can move this comment before the if and...
> > if (server_walker == client_walker)
> > return false;
> >
> > return true;
...add this without the curly braces:
return server_walker != client_walker;
> > }
> > client_walker = client_walker->parent;
> > server_walker = server_walker->parent;
> > }
> > return false;
> > }
>
> I think adding something like this change on top of your code would
> make it more concise (though this is entirely untested):
>
> --- /tmp/a 2024-08-06 22:37:33.800158308 +0200
> +++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
> @@ -15,25 +15,12 @@
> * client_layer must be a signed integer with greater capacity than
> * client->num_layers to ensure the following loop stops.
> */
> BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
>
> - if (!server) {
> - /*
> - * Walks client's parent domains and checks that none of these
> - * domains are scoped.
> - */
> - for (; client_layer >= 0; client_layer--) {
> - if (landlock_get_scope_mask(client, client_layer) &
> - scope)
> - return true;
> - }
> - return false;
> - }
This loop is redundant with the following one, but it makes sure there
is no issue nor inconsistencies with the server or server_walker
pointers. That's the only approach I found to make sure we don't go
through a path that could use an incorrect pointer, and makes the code
easy to review.
> -
> - server_layer = server->num_layers - 1;
> - server_walker = server->hierarchy;
> + server_layer = server ? (server->num_layers - 1) : -1;
> + server_walker = server ? server->hierarchy : NULL;
We would need to change the last loop to avoid a null pointer deref.
>
> /*
> * Walks client's parent domains down to the same hierarchy level as
> * the server's domain, and checks that none of these client's parent
> * domains are scoped.
>
On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > I think adding something like this change on top of your code would
> > make it more concise (though this is entirely untested):
> >
> > --- /tmp/a 2024-08-06 22:37:33.800158308 +0200
> > +++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
> > @@ -15,25 +15,12 @@
> > * client_layer must be a signed integer with greater capacity than
> > * client->num_layers to ensure the following loop stops.
> > */
> > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> >
> > - if (!server) {
> > - /*
> > - * Walks client's parent domains and checks that none of these
> > - * domains are scoped.
> > - */
> > - for (; client_layer >= 0; client_layer--) {
> > - if (landlock_get_scope_mask(client, client_layer) &
> > - scope)
> > - return true;
> > - }
> > - return false;
> > - }
>
> This loop is redundant with the following one, but it makes sure there
> is no issue nor inconsistencies with the server or server_walker
> pointers. That's the only approach I found to make sure we don't go
> through a path that could use an incorrect pointer, and makes the code
> easy to review.
My view is that this is a duplication of logic for one particular
special case - after all, you can also end up walking up to the same
state (client_layer==-1, server_layer==-1, client_walker==NULL,
server_walker==NULL) with the loop at the bottom.
But I guess my preference for more concise code is kinda subjective -
if you prefer the more verbose version, I'm fine with that too.
> > -
> > - server_layer = server->num_layers - 1;
> > - server_walker = server->hierarchy;
> > + server_layer = server ? (server->num_layers - 1) : -1;
> > + server_walker = server ? server->hierarchy : NULL;
>
> We would need to change the last loop to avoid a null pointer deref.
Why? The first loop would either exit or walk the client_walker up
until client_layer is -1 and client_walker is NULL; the second loop
wouldn't do anything because the walkers are at the same layer; the
third loop's body wouldn't be executed because client_layer is -1.
The case where the server is not in any Landlock domain is just one
subcase of the more general case "client and server do not have a
common ancestor domain".
> >
> > /*
> > * Walks client's parent domains down to the same hierarchy level as
> > * the server's domain, and checks that none of these client's parent
> > * domains are scoped.
> >
On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > I think adding something like this change on top of your code would
> > > make it more concise (though this is entirely untested):
> > >
> > > --- /tmp/a 2024-08-06 22:37:33.800158308 +0200
> > > +++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
> > > @@ -15,25 +15,12 @@
> > > * client_layer must be a signed integer with greater capacity than
> > > * client->num_layers to ensure the following loop stops.
> > > */
> > > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > >
> > > - if (!server) {
> > > - /*
> > > - * Walks client's parent domains and checks that none of these
> > > - * domains are scoped.
> > > - */
> > > - for (; client_layer >= 0; client_layer--) {
> > > - if (landlock_get_scope_mask(client, client_layer) &
> > > - scope)
> > > - return true;
> > > - }
> > > - return false;
> > > - }
> >
> > This loop is redundant with the following one, but it makes sure there
> > is no issue nor inconsistencies with the server or server_walker
> > pointers. That's the only approach I found to make sure we don't go
> > through a path that could use an incorrect pointer, and makes the code
> > easy to review.
>
> My view is that this is a duplication of logic for one particular
> special case - after all, you can also end up walking up to the same
> state (client_layer==-1, server_layer==-1, client_walker==NULL,
> server_walker==NULL) with the loop at the bottom.
Indeed
>
> But I guess my preference for more concise code is kinda subjective -
> if you prefer the more verbose version, I'm fine with that too.
>
> > > -
> > > - server_layer = server->num_layers - 1;
> > > - server_walker = server->hierarchy;
> > > + server_layer = server ? (server->num_layers - 1) : -1;
> > > + server_walker = server ? server->hierarchy : NULL;
> >
> > We would need to change the last loop to avoid a null pointer deref.
>
> Why? The first loop would either exit or walk the client_walker up
> until client_layer is -1 and client_walker is NULL; the second loop
> wouldn't do anything because the walkers are at the same layer; the
> third loop's body wouldn't be executed because client_layer is -1.
Correct, I missed that client_layer would always be greater than
server_layer (-1).
Tahera, could you please take Jann's proposal?
>
> The case where the server is not in any Landlock domain is just one
> subcase of the more general case "client and server do not have a
> common ancestor domain".
>
> > >
> > > /*
> > > * Walks client's parent domains down to the same hierarchy level as
> > > * the server's domain, and checks that none of these client's parent
> > > * domains are scoped.
> > >
>
On Wed, Aug 07, 2024 at 04:44:36PM +0200, Mickaël Salaün wrote:
> On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> > On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > > I think adding something like this change on top of your code would
> > > > make it more concise (though this is entirely untested):
> > > >
> > > > --- /tmp/a 2024-08-06 22:37:33.800158308 +0200
> > > > +++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
> > > > @@ -15,25 +15,12 @@
> > > > * client_layer must be a signed integer with greater capacity than
> > > > * client->num_layers to ensure the following loop stops.
> > > > */
> > > > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > > >
> > > > - if (!server) {
> > > > - /*
> > > > - * Walks client's parent domains and checks that none of these
> > > > - * domains are scoped.
> > > > - */
> > > > - for (; client_layer >= 0; client_layer--) {
> > > > - if (landlock_get_scope_mask(client, client_layer) &
> > > > - scope)
> > > > - return true;
> > > > - }
> > > > - return false;
> > > > - }
> > >
> > > This loop is redundant with the following one, but it makes sure there
> > > is no issue nor inconsistencies with the server or server_walker
> > > pointers. That's the only approach I found to make sure we don't go
> > > through a path that could use an incorrect pointer, and makes the code
> > > easy to review.
> >
> > My view is that this is a duplication of logic for one particular
> > special case - after all, you can also end up walking up to the same
> > state (client_layer==-1, server_layer==-1, client_walker==NULL,
> > server_walker==NULL) with the loop at the bottom.
>
> Indeed
>
> >
> > But I guess my preference for more concise code is kinda subjective -
> > if you prefer the more verbose version, I'm fine with that too.
> >
> > > > -
> > > > - server_layer = server->num_layers - 1;
> > > > - server_walker = server->hierarchy;
> > > > + server_layer = server ? (server->num_layers - 1) : -1;
> > > > + server_walker = server ? server->hierarchy : NULL;
> > >
> > > We would need to change the last loop to avoid a null pointer deref.
> >
> > Why? The first loop would either exit or walk the client_walker up
> > until client_layer is -1 and client_walker is NULL; the second loop
> > wouldn't do anything because the walkers are at the same layer; the
> > third loop's body wouldn't be executed because client_layer is -1.
>
> Correct, I missed that client_layer would always be greater than
> server_layer (-1).
>
> Tahera, could you please take Jann's proposal?
Done.
We will have duplicate logic, but it would be easier to read and review.
>
> >
> > The case where the server is not in any Landlock domain is just one
> > subcase of the more general case "client and server do not have a
> > common ancestor domain".
> >
> > > >
> > > > /*
> > > > * Walks client's parent domains down to the same hierarchy level as
> > > > * the server's domain, and checks that none of these client's parent
> > > > * domains are scoped.
> > > >
> >
On Thu, Aug 08, 2024 at 05:17:10PM -0600, Tahera Fahimi wrote:
> On Wed, Aug 07, 2024 at 04:44:36PM +0200, Mickaël Salaün wrote:
> > On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> > > On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > > > I think adding something like this change on top of your code would
> > > > > make it more concise (though this is entirely untested):
> > > > >
> > > > > --- /tmp/a 2024-08-06 22:37:33.800158308 +0200
> > > > > +++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
> > > > > @@ -15,25 +15,12 @@
> > > > > * client_layer must be a signed integer with greater capacity than
> > > > > * client->num_layers to ensure the following loop stops.
> > > > > */
> > > > > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > > > >
> > > > > - if (!server) {
> > > > > - /*
> > > > > - * Walks client's parent domains and checks that none of these
> > > > > - * domains are scoped.
> > > > > - */
> > > > > - for (; client_layer >= 0; client_layer--) {
> > > > > - if (landlock_get_scope_mask(client, client_layer) &
> > > > > - scope)
> > > > > - return true;
> > > > > - }
> > > > > - return false;
> > > > > - }
> > > >
> > > > This loop is redundant with the following one, but it makes sure there
> > > > is no issue nor inconsistencies with the server or server_walker
> > > > pointers. That's the only approach I found to make sure we don't go
> > > > through a path that could use an incorrect pointer, and makes the code
> > > > easy to review.
> > >
> > > My view is that this is a duplication of logic for one particular
> > > special case - after all, you can also end up walking up to the same
> > > state (client_layer==-1, server_layer==-1, client_walker==NULL,
> > > server_walker==NULL) with the loop at the bottom.
> >
> > Indeed
> >
> > >
> > > But I guess my preference for more concise code is kinda subjective -
> > > if you prefer the more verbose version, I'm fine with that too.
> > >
> > > > > -
> > > > > - server_layer = server->num_layers - 1;
> > > > > - server_walker = server->hierarchy;
> > > > > + server_layer = server ? (server->num_layers - 1) : -1;
> > > > > + server_walker = server ? server->hierarchy : NULL;
> > > >
> > > > We would need to change the last loop to avoid a null pointer deref.
> > >
> > > Why? The first loop would either exit or walk the client_walker up
> > > until client_layer is -1 and client_walker is NULL; the second loop
> > > wouldn't do anything because the walkers are at the same layer; the
> > > third loop's body wouldn't be executed because client_layer is -1.
> >
> > Correct, I missed that client_layer would always be greater than
> > server_layer (-1).
> >
> > Tahera, could you please take Jann's proposal?
> Done.
> We will have duplicate logic, but it would be easier to read and review.
With Jann's proposal we don't have duplicate logic.
> >
> > >
> > > The case where the server is not in any Landlock domain is just one
> > > subcase of the more general case "client and server do not have a
> > > common ancestor domain".
> > >
> > > > >
> > > > > /*
> > > > > * Walks client's parent domains down to the same hierarchy level as
> > > > > * the server's domain, and checks that none of these client's parent
> > > > > * domains are scoped.
> > > > >
> > >
>
On Fri, Aug 09, 2024 at 10:49:17AM +0200, Mickaël Salaün wrote:
> On Thu, Aug 08, 2024 at 05:17:10PM -0600, Tahera Fahimi wrote:
> > On Wed, Aug 07, 2024 at 04:44:36PM +0200, Mickaël Salaün wrote:
> > > On Wed, Aug 07, 2024 at 03:45:18PM +0200, Jann Horn wrote:
> > > > On Wed, Aug 7, 2024 at 9:21 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > > On Tue, Aug 06, 2024 at 10:46:43PM +0200, Jann Horn wrote:
> > > > > > I think adding something like this change on top of your code would
> > > > > > make it more concise (though this is entirely untested):
> > > > > >
> > > > > > --- /tmp/a 2024-08-06 22:37:33.800158308 +0200
> > > > > > +++ /tmp/b 2024-08-06 22:44:49.539314039 +0200
> > > > > > @@ -15,25 +15,12 @@
> > > > > > * client_layer must be a signed integer with greater capacity than
> > > > > > * client->num_layers to ensure the following loop stops.
> > > > > > */
> > > > > > BUILD_BUG_ON(sizeof(client_layer) > sizeof(client->num_layers));
> > > > > >
> > > > > > - if (!server) {
> > > > > > - /*
> > > > > > - * Walks client's parent domains and checks that none of these
> > > > > > - * domains are scoped.
> > > > > > - */
> > > > > > - for (; client_layer >= 0; client_layer--) {
> > > > > > - if (landlock_get_scope_mask(client, client_layer) &
> > > > > > - scope)
> > > > > > - return true;
> > > > > > - }
> > > > > > - return false;
> > > > > > - }
> > > > >
> > > > > This loop is redundant with the following one, but it makes sure there
> > > > > is no issue nor inconsistencies with the server or server_walker
> > > > > pointers. That's the only approach I found to make sure we don't go
> > > > > through a path that could use an incorrect pointer, and makes the code
> > > > > easy to review.
> > > >
> > > > My view is that this is a duplication of logic for one particular
> > > > special case - after all, you can also end up walking up to the same
> > > > state (client_layer==-1, server_layer==-1, client_walker==NULL,
> > > > server_walker==NULL) with the loop at the bottom.
> > >
> > > Indeed
> > >
> > > >
> > > > But I guess my preference for more concise code is kinda subjective -
> > > > if you prefer the more verbose version, I'm fine with that too.
> > > >
> > > > > > -
> > > > > > - server_layer = server->num_layers - 1;
> > > > > > - server_walker = server->hierarchy;
> > > > > > + server_layer = server ? (server->num_layers - 1) : -1;
> > > > > > + server_walker = server ? server->hierarchy : NULL;
> > > > >
> > > > > We would need to change the last loop to avoid a null pointer deref.
> > > >
> > > > Why? The first loop would either exit or walk the client_walker up
> > > > until client_layer is -1 and client_walker is NULL; the second loop
> > > > wouldn't do anything because the walkers are at the same layer; the
> > > > third loop's body wouldn't be executed because client_layer is -1.
> > >
> > > Correct, I missed that client_layer would always be greater than
> > > server_layer (-1).
> > >
> > > Tahera, could you please take Jann's proposal?
> > Done.
> > We will have duplicate logic, but it would be easier to read and review.
>
> With Jann's proposal we don't have duplicate logic.
Still the first two for loops apply the same logic for client and server
domains, but I totally understand that it is much easier to review and
understand.
> > >
> > > >
> > > > The case where the server is not in any Landlock domain is just one
> > > > subcase of the more general case "client and server do not have a
> > > > common ancestor domain".
> > > >
> > > > > >
> > > > > > /*
> > > > > > * Walks client's parent domains down to the same hierarchy level as
> > > > > > * the server's domain, and checks that none of these client's parent
> > > > > > * domains are scoped.
> > > > > >
> > > >
> >
On Thu, Aug 01, 2024 at 10:02:33PM -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>
>
> ---
> 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 | 30 +++++++
> security/landlock/limits.h | 3 +
> security/landlock/ruleset.c | 7 +-
> security/landlock/ruleset.h | 23 ++++-
> security/landlock/syscalls.c | 14 ++-
> security/landlock/task.c | 155 ++++++++++++++++++++++++++++++++++
> 6 files changed, 225 insertions(+), 7 deletions(-)
>
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 68625e728f43..ab31e9f53e55 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 scopes (cf. `Scope flags`_)
> + * restricting a Landlock domain from accessing outside
> + * resources(e.g. IPCs).
> + */
> + __u64 scoped;
> };
>
> /*
> @@ -266,4 +272,28 @@ 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
> + *
> + * scoped attribute handles a set of restrictions on kernel IPCs through
> + * the following flags.
If present, this previous sentence should be after the "Scope flags"
section, otherwise it shows in the previous section. I don't think this
sentence is useful because of the next one, so you can remove it.
> + *
> + * 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)
> +/* 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 */
>
> #endif /* _SECURITY_LANDLOCK_LIMITS_H */
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index 6ff232f58618..a93bdbf52fff 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -52,12 +52,13 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>
> struct landlock_ruleset *
> landlock_create_ruleset(const access_mask_t fs_access_mask,
> - const access_mask_t net_access_mask)
> + const access_mask_t net_access_mask,
> + const access_mask_t scope_mask)
> {
> struct landlock_ruleset *new_ruleset;
>
> /* Informs about useless ruleset. */
> - if (!fs_access_mask && !net_access_mask)
> + if (!fs_access_mask && !net_access_mask && !scope_mask)
> return ERR_PTR(-ENOMSG);
> new_ruleset = create_ruleset(1);
> if (IS_ERR(new_ruleset))
> @@ -66,6 +67,8 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
> landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
> if (net_access_mask)
> landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
> + if (scope_mask)
> + landlock_add_scope_mask(new_ruleset, scope_mask, 0);
> return new_ruleset;
> }
>
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 0f1b5b4c8f6b..c749fa0b3ecd 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -35,6 +35,8 @@ typedef u16 access_mask_t;
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
> /* Makes sure all network access rights can be stored. */
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
> +/* Makes sure all scoped rights can be stored*/
> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
> /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
> static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>
> @@ -42,6 +44,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
> struct access_masks {
> access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
> access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
> + access_mask_t scoped : LANDLOCK_NUM_SCOPE;
> };
>
> typedef u16 layer_mask_t;
> @@ -233,7 +236,8 @@ struct landlock_ruleset {
>
> struct landlock_ruleset *
> landlock_create_ruleset(const access_mask_t access_mask_fs,
> - const access_mask_t access_mask_net);
> + const access_mask_t access_mask_net,
> + const access_mask_t scope_mask);
>
> void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
> void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
> @@ -280,6 +284,16 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
> ruleset->access_masks[layer_level].net |= net_mask;
> }
>
> +static inline void
> +landlock_add_scope_mask(struct landlock_ruleset *const ruleset,
> + const access_mask_t scope_mask, const u16 layer_level)
> +{
> + access_mask_t scoped_mask = scope_mask & LANDLOCK_MASK_SCOPE;
> +
> + WARN_ON_ONCE(scope_mask != scoped_mask);
> + ruleset->access_masks[layer_level].scoped |= scoped_mask;
> +}
> +
> static inline access_mask_t
> landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
> const u16 layer_level)
> @@ -303,6 +317,13 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
> return ruleset->access_masks[layer_level].net;
> }
>
> +static inline access_mask_t
> +landlock_get_scope_mask(const struct landlock_ruleset *const ruleset,
> + const u16 layer_level)
> +{
> + return ruleset->access_masks[layer_level].scoped;
> +}
> +
> bool landlock_unmask_layers(const struct landlock_rule *const rule,
> const access_mask_t access_request,
> layer_mask_t (*const layer_masks)[],
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 03b470f5a85a..f51b29521d6b 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -97,8 +97,9 @@ static void build_check_abi(void)
> */
> ruleset_size = sizeof(ruleset_attr.handled_access_fs);
> ruleset_size += sizeof(ruleset_attr.handled_access_net);
> + ruleset_size += sizeof(ruleset_attr.scoped);
> BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
> - BUILD_BUG_ON(sizeof(ruleset_attr) != 16);
> + BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
>
> path_beneath_size = sizeof(path_beneath_attr.allowed_access);
> path_beneath_size += sizeof(path_beneath_attr.parent_fd);
> @@ -149,7 +150,7 @@ static const struct file_operations ruleset_fops = {
> .write = fop_dummy_write,
> };
>
> -#define LANDLOCK_ABI_VERSION 5
> +#define LANDLOCK_ABI_VERSION 6
>
> /**
> * sys_landlock_create_ruleset - Create a new ruleset
> @@ -170,7 +171,7 @@ static const struct file_operations ruleset_fops = {
> * Possible returned errors are:
> *
> * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
> - * - %EINVAL: unknown @flags, or unknown access, or too small @size;
> + * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size;
I think you missed one of my previous review.
> * - %E2BIG or %EFAULT: @attr or @size inconsistencies;
> * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs.
> */
> @@ -213,9 +214,14 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> LANDLOCK_MASK_ACCESS_NET)
> return -EINVAL;
>
> + /* Checks IPC scoping content (and 32-bits cast). */
> + if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
> + return -EINVAL;
> +
> /* Checks arguments and transforms to kernel struct. */
> ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
> - ruleset_attr.handled_access_net);
> + ruleset_attr.handled_access_net,
> + ruleset_attr.scoped);
> if (IS_ERR(ruleset))
> return PTR_ERR(ruleset);
>
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 849f5123610b..7e8579ebae83 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>
Please sort the included files.
>
> #include "common.h"
> #include "cred.h"
> @@ -108,9 +110,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> return task_ptrace(parent, current);
> }
>
> +static bool walk_and_check(const struct landlock_ruleset *const child,
> + struct landlock_hierarchy **walker,
> + size_t base_layer, size_t deep_layer,
> + access_mask_t check_scoping)
> +{
> + if (!child || base_layer < 0 || !(*walker))
> + return false;
> +
> + for (deep_layer; base_layer < deep_layer; deep_layer--) {
> + if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> + return false;
> + *walker = (*walker)->parent;
> + if (WARN_ON_ONCE(!*walker))
> + /* there is an inconsistency between num_layers
> + * and landlock_hierarchy in the ruleset
> + */
> + return false;
> + }
> + return true;
> +}
> +
> +/**
> + * domain_IPC_scope - Checks if the client domain is scoped in the same
Please don't use uppercase (IPC) in function or variable's names, it is
reserved to "#define" statements only.
> + * domain as the server.
> + *
> + * @client: IPC sender domain.
> + * @server: IPC receiver domain.
> + *
> + * Check if the @client domain is scoped to access the @server; the @server
> + * must be scoped in the same domain.
> + */
> +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> + const struct landlock_ruleset *const server,
> + access_mask_t ipc_type)
> +{
> + size_t client_layer, server_layer = 0;
> + int base_layer;
> + struct landlock_hierarchy *client_walker, *server_walker;
> + bool is_scoped;
> +
> + /* Quick return if client has no domain */
> + if (!client)
> + return true;
> +
> + client_layer = client->num_layers - 1;
> + client_walker = client->hierarchy;
> + if (server) {
> + server_layer = server->num_layers - 1;
> + server_walker = server->hierarchy;
> + }
> + base_layer = (client_layer > server_layer) ? server_layer :
> + client_layer;
> +
> + /* For client domain, walk_and_check ensures the client domain is
> + * not scoped until gets to base_layer.
> + * For server_domain, it only ensures that the server domain exist.
> + */
> + if (client_layer != server_layer) {
> + if (client_layer > server_layer)
> + is_scoped = walk_and_check(client, &client_walker,
> + server_layer, client_layer,
> + ipc_type);
> + else
> + is_scoped = walk_and_check(server, &server_walker,
> + client_layer, server_layer,
> + ipc_type & 0);
> + if (!is_scoped)
> + return false;
> + }
> + /* client and server are at the same level in hierarchy. If client is
> + * scoped, the server must be scoped in the same domain
> + */
> + for (base_layer; base_layer >= 0; base_layer--) {
> + if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
> + /* This check must be here since access would be denied only if
> + * the client is scoped and the server has no domain, so
> + * if the client has a domain but is not scoped and the server
> + * has no domain, access is guaranteed.
> + */
> + if (!server)
> + return false;
> +
> + if (server_walker == client_walker)
> + return true;
> +
> + return false;
> + }
> + client_walker = client_walker->parent;
> + server_walker = server_walker->parent;
> + /* Warn if there is an incosistenncy between num_layers and
> + * landlock_hierarchy in each of rulesets
> + */
> + if (WARN_ON_ONCE(base_layer > 0 &&
> + (!server_walker || !client_walker)))
> + return false;
> + }
> + 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;
> + return domain_IPC_scope(dom, dom_other,
> + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET);
> +}
> +
> +static bool check_unix_address_format(struct sock *const sock)
> +{
> + struct unix_address *addr = unix_sk(sock)->addr;
> +
> + if (!addr)
> + return true;
> +
> + if (addr->len > sizeof(AF_UNIX)) {
AF_UNIX is the family, not a length.
I'm wondering if we cannot also check the "path" field or something
else.
> + /* handling unspec sockets */
> + if (!addr->name[0].sun_path)
> + return true;
> +
> + if (addr->name[0].sun_path[0] == '\0')
> + if (!sock_is_scoped(sock))
This function should only check the address format. sock_is_scoped()
can be called in each LSM hook.
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static int hook_unix_stream_connect(struct sock *const sock,
> + struct sock *const other,
> + struct sock *const newsk)
> +{
To not impact unsandboxed tasks, we need to first check if current is
sandboxed with Landlock, and then pass this dom to the following
functions.
> + if (check_unix_address_format(other))
> + return 0;
> +
> + return -EPERM;
> +}
> +
> +static int hook_unix_may_send(struct socket *const sock,
> + struct socket *const other)
> +{
> + if (check_unix_address_format(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
>
>
© 2016 - 2026 Red Hat, Inc.