[PATCH v5] landlock: Add abstract unix socket connect restriction
Mickaël Salaün
mic at digikod.net
Fri Jun 21 16:00:01 UTC 2024
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 <fahimitahera at gmail.com>
>
> -------
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.
> V4: Added abstract unix socket scoping tests
> V3: Added "scoped" field to landlock_ruleset_attr
> V2: Remove wrapper functions
>
> -------
>
> Signed-off-by: Tahera Fahimi <fahimitahera at gmail.com>
> ---
> 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)."
> + */
> + __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).
> + */
> +/* 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.
More information about the Linux-security-module-archive
mailing list