Re: [PATCH v5] landlock: Add abstract unix socket connect restriction
From: Mickaël Salaün <mic@digikod.net>
Date: 2024-06-21 16:05:44
Also in:
linux-security-module, lkml
On Thu, Jun 20, 2024 at 03:05:34PM GMT, Tahera Fahimi wrote:
Abstract unix sockets are used for local inter-process communications without on a filesystem. Currently a sandboxed process can connect to a
"without a"
socket outside of the sandboxed environment, since landlock has no
s/landlock/Landlock/
restriction for connecting to a unix socket in the abstract namespace.
"namespace" usually refers to the namespaces(7) man page. What about using the same vocabulary is in unix(7): "for connecting to an abstract socket address."
Access to such sockets for a sandboxed process should be scoped the same way ptrace is limited. Because of compatibility reasons and since landlock should be flexible, we extend the user space interface by adding a new "scoped" field
...to the ruleset attribute structure.
. This field optionally contains a "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" to specify that the ruleset will deny any connection from within the sandbox to its parents(i.e. any parent sandbox or non-sandbox processes) Closes: https://github.com/landlock-lsm/linux/issues/7 Signed-off-by: Tahera Fahimi <redacted> -------
For the next version, please list all changes since last version. With this v5 I see some renaming, a new curr_ruleset field with optional domain scopping, and code formatting.
quoted hunk ↗ jump to hunk
V4: Added abstract unix socket scoping tests V3: Added "scoped" field to landlock_ruleset_attr V2: Remove wrapper functions ------- Signed-off-by: Tahera Fahimi <redacted> --- include/uapi/linux/landlock.h | 27 ++ security/landlock/limits.h | 3 + security/landlock/ruleset.c | 12 +- security/landlock/ruleset.h | 27 +- security/landlock/syscalls.c | 13 +- security/landlock/task.c | 95 +++++++ .../testing/selftests/landlock/ptrace_test.c | 261 ++++++++++++++++++ 7 files changed, 430 insertions(+), 8 deletions(-)diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h index 68625e728f43..1eb459afcb3b 100644 --- a/include/uapi/linux/landlock.h +++ b/include/uapi/linux/landlock.h@@ -37,6 +37,11 @@ struct landlock_ruleset_attr { * rule explicitly allow them. */ __u64 handled_access_net; + /** + * scoped: Bitmask of actions (cf. `Scope access flags`_)
Please take a look at the generated documentation and fix the build warnings related to this patch: check-linux.sh doc (or make htmldocs)
+ * which are confined to only affect the current Landlock domain.
What about this? "Bitmask of scopes () restricting a Landlock domain from accessing outside resources (e.g. IPCs)."
quoted hunk ↗ jump to hunk
+ */ + __u64 scoped; }; /*@@ -266,4 +271,26 @@ struct landlock_net_port_attr { #define LANDLOCK_ACCESS_NET_BIND_TCP (1ULL << 0) #define LANDLOCK_ACCESS_NET_CONNECT_TCP (1ULL << 1) /* clang-format on */ + +/** + * DOC: scope + * + * .scoped attribute handles a set of restrictions on kernel IPCs through + * the following flags.
Shouldn't this be after the section title?
+ * + * Scope access flags
You can remove "access"
+ * ~~~~~~~~~~~~~~~~~~~~ + * + * These flags enable to restrict a sandboxed process from a set of IPC
There are several spaces at the end of lines, they should be removed.
+ * actions. Setting a flag in a landlock domain will isolate the Landlock
A flag is not set "in a Landlock domain" but for a ruleset.
+ * domain to forbid connections to resources outside the domain.
Please remove unneeded spaces.
+ * + * IPCs with scoped actions: + * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandbox process to + * connect to a process outside of the sandbox domain through abstract + * unix sockets.
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 process which is not sandboxed).
quoted hunk ↗ jump to hunk
+ */ +/* 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..3b3844574326 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; }@@ -311,7 +314,7 @@ static void put_hierarchy(struct landlock_hierarchy *hierarchy) { while (hierarchy && refcount_dec_and_test(&hierarchy->usage)) { const struct landlock_hierarchy *const freeme = hierarchy; - + hierarchy = hierarchy->parent; kfree(freeme); }@@ -472,6 +475,7 @@ static int inherit_ruleset(struct landlock_ruleset *const parent, } get_hierarchy(parent->hierarchy); child->hierarchy->parent = parent->hierarchy; + child->hierarchy->curr_ruleset = child; out_unlock: mutex_unlock(&parent->lock);@@ -571,7 +575,7 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent, err = merge_ruleset(new_dom, ruleset); if (err) goto out_put_dom; - + new_dom->hierarchy->curr_ruleset = new_dom; return new_dom; out_put_dom:diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h index 0f1b5b4c8f6b..39cb313812dc 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;@@ -150,6 +153,10 @@ struct landlock_hierarchy { * domain. */ refcount_t usage; + /** + * @curr_ruleset: a pointer back to the current ruleset + */ + struct landlock_ruleset *curr_ruleset;
This curr_ruleset pointer can become a dangling pointer and then lead to a user after free bug because a domain (i.e. ruleset tie to a set of processes) is free when no processes use it. Instead, we could just use a bitmask (or a boolean for now) to identify if the related layer scopes abstract unix sockets. Because struct landlock_hierarchy identifies only one layer of a domain, another and simpler approach would be to only rely on the "client" and "server" domains' layers.