* [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction
@ 2024-08-14 6:22 Tahera Fahimi
2024-08-14 6:22 ` [PATCH v9 1/5] " Tahera Fahimi
` (5 more replies)
0 siblings, 6 replies; 21+ messages in thread
From: Tahera Fahimi @ 2024-08-14 6:22 UTC (permalink / raw)
To: outreachy
Cc: mic, gnoack, paul, jmorris, serge, linux-security-module,
linux-kernel, bjorn3_gh, jannh, netdev, Tahera Fahimi
This patch series adds scoping mechanism for abstract unix sockets.
Closes: https://github.com/landlock-lsm/linux/issues/7
Problem
=======
Abstract unix sockets are used for local inter-process communications
independent of the filesystem. Currently, a sandboxed process can
connect to a socket outside of the sandboxed environment, since Landlock
has no restriction for connecting to an abstract socket address(see more
details in [1,2]). Access to such sockets for a sandboxed process should
be scoped the same way ptrace is limited.
[1] https://lore.kernel.org/all/20231023.ahphah4Wii4v@digikod.net/
[2] https://lore.kernel.org/all/20231102.MaeWaepav8nu@digikod.net/
Solution
========
To solve this issue, we extend the user space interface by adding a new
"scoped" field to Landlock ruleset attribute structure. This field can
contains different rights to restrict different functionalities. For
abstract unix sockets, we introduce
"LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" field to specify that a ruleset
will deny any connection from within the sandbox domain to its parent
(i.e. any parent sandbox or non-sandbox processes).
Example
=======
Starting a listening socket with socat(1):
socat abstract-listen:mysocket -
Starting a sandboxed shell from $HOME with samples/landlock/sandboxer:
LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash
If we try to connect to the listening socket, the connection would be
refused.
socat - abstract-connect:mysocket --> fails
Notes of Implementation
=======================
* Using the "scoped" field provides enough compatibility and flexibility
to extend the scoping mechanism for other IPCs(e.g. signals).
* To access the domain of a socket, we use its credentials of the file's FD
which point to the credentials of the process that created the socket.
(see more details in [3]). Cases where the process using the socket has
a different domain than the process created it are covered in the
unix_sock_special_cases test.
[3]https://lore.kernel.org/all/20240611.Pi8Iph7ootae@digikod.net/
Previous Versions
=================
v8: https://lore.kernel.org/all/cover.1722570749.git.fahimitahera@gmail.com/
v7: https://lore.kernel.org/all/cover.1721269836.git.fahimitahera@gmail.com/
v6: https://lore.kernel.org/all/Zn32CYZiu7pY+rdI@tahera-OptiPlex-5000/
and https://lore.kernel.org/all/Zn32KKIJrY7Zi51K@tahera-OptiPlex-5000/
v5: https://lore.kernel.org/all/ZnSZnhGBiprI6FRk@tahera-OptiPlex-5000/
v4: https://lore.kernel.org/all/ZnNcE3ph2SWi1qmd@tahera-OptiPlex-5000/
v3: https://lore.kernel.org/all/ZmJJ7lZdQuQop7e5@tahera-OptiPlex-5000/
v2: https://lore.kernel.org/all/ZgX5TRTrSDPrJFfF@tahera-OptiPlex-5000/
v1: https://lore.kernel.org/all/ZgXN5fi6A1YQKiAQ@tahera-OptiPlex-5000/
Tahera Fahimi (5):
Landlock: Add abstract unix socket connect restriction
selftests/Landlock: Abstract unix socket restriction tests
selftests/Landlock: Adding pathname Unix socket tests
sample/Landlock: Support abstract unix socket restriction
Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI
versioning
Documentation/userspace-api/landlock.rst | 33 +-
include/uapi/linux/landlock.h | 27 +
samples/landlock/sandboxer.c | 58 +-
security/landlock/limits.h | 3 +
security/landlock/ruleset.c | 7 +-
security/landlock/ruleset.h | 23 +-
security/landlock/syscalls.c | 17 +-
security/landlock/task.c | 129 ++
tools/testing/selftests/landlock/base_test.c | 2 +-
tools/testing/selftests/landlock/common.h | 38 +
tools/testing/selftests/landlock/net_test.c | 31 +-
.../landlock/scoped_abstract_unix_test.c | 1146 +++++++++++++++++
12 files changed, 1469 insertions(+), 45 deletions(-)
create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c
--
2.34.1
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH v9 1/5] Landlock: Add abstract unix socket connect restriction 2024-08-14 6:22 [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction Tahera Fahimi @ 2024-08-14 6:22 ` Tahera Fahimi 2024-08-16 21:19 ` Mickaël Salaün ` (2 more replies) 2024-08-14 6:22 ` [PATCH v9 2/5] selftests/Landlock: Abstract unix socket restriction tests Tahera Fahimi ` (4 subsequent siblings) 5 siblings, 3 replies; 21+ messages in thread From: Tahera Fahimi @ 2024-08-14 6:22 UTC (permalink / raw) To: outreachy Cc: mic, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev, Tahera Fahimi 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> --- v9: - Editting inline comments. - Major refactoring in domain_is_scoped() and is_abstract_socket 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 | 27 +++++++ security/landlock/limits.h | 3 + security/landlock/ruleset.c | 7 +- security/landlock/ruleset.h | 23 +++++- security/landlock/syscalls.c | 17 +++-- security/landlock/task.c | 129 ++++++++++++++++++++++++++++++++++ 6 files changed, 198 insertions(+), 8 deletions(-) diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h index 68625e728f43..057a4811ed06 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,25 @@ 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 + * + * 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..20d2a8b5aa42 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,8 +171,9 @@ 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; - * - %E2BIG or %EFAULT: @attr or @size inconsistencies; + * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size; + * - %E2BIG: @attr or @size inconsistencies; + * - %EFAULT: @attr or @size inconsistencies; * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs. */ SYSCALL_DEFINE3(landlock_create_ruleset, @@ -213,9 +215,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..a461923c0571 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/af_unix.h> +#include <net/sock.h> #include "common.h" #include "cred.h" @@ -108,9 +110,136 @@ static int hook_ptrace_traceme(struct task_struct *const parent) return task_ptrace(parent, current); } +/** + * domain_is_scoped - Checks if the client domain is scoped in the same + * domain as the server. + * + * @client: IPC sender domain. + * @server: IPC receiver domain. + * + * Return true if the @client domain is scoped to access the @server, + * unless the @server is also scoped in the same domain as @client. + */ +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; + + /* Quick return if client has no domain */ + 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)); + + 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. + */ + 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. + */ + return server_walker != client_walker; + } + client_walker = client_walker->parent; + server_walker = server_walker->parent; + } + return false; +} + +static bool sock_is_scoped(struct sock *const other, + const struct landlock_ruleset *const dom) +{ + const struct landlock_ruleset *dom_other; + + /* 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_is_scoped(dom, dom_other, + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET); +} + +static bool is_abstract_socket(struct sock *const sock) +{ + struct unix_address *addr = unix_sk(sock)->addr; + + if (!addr) + return false; + + if (addr->len >= offsetof(struct sockaddr_un, sun_path) + 1 && + addr->name[0].sun_path[0] == '\0') + return true; + + return false; +} + +static int hook_unix_stream_connect(struct sock *const sock, + struct sock *const other, + struct sock *const newsk) +{ + const struct landlock_ruleset *const dom = + landlock_get_current_domain(); + + /* quick return for non-sandboxed processes */ + if (!dom) + return 0; + + if (is_abstract_socket(other)) + if (sock_is_scoped(other, dom)) + return -EPERM; + + return 0; +} + +static int hook_unix_may_send(struct socket *const sock, + struct socket *const other) +{ + const struct landlock_ruleset *const dom = + landlock_get_current_domain(); + + if (!dom) + return 0; + + if (is_abstract_socket(other->sk)) + if (sock_is_scoped(other->sk, dom)) + return -EPERM; + + return 0; +} + 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v9 1/5] Landlock: Add abstract unix socket connect restriction 2024-08-14 6:22 ` [PATCH v9 1/5] " Tahera Fahimi @ 2024-08-16 21:19 ` Mickaël Salaün 2024-08-19 15:37 ` Mickaël Salaün 2024-08-19 19:35 ` Mickaël Salaün 2 siblings, 0 replies; 21+ messages in thread From: Mickaël Salaün @ 2024-08-16 21:19 UTC (permalink / raw) To: Tahera Fahimi Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Wed, Aug 14, 2024 at 12:22:19AM -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> > > --- > v9: > - Editting inline comments. > - Major refactoring in domain_is_scoped() and is_abstract_socket > 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/ > ---- Useless "----" > --- > include/uapi/linux/landlock.h | 27 +++++++ > security/landlock/limits.h | 3 + > security/landlock/ruleset.c | 7 +- > security/landlock/ruleset.h | 23 +++++- > security/landlock/syscalls.c | 17 +++-- > security/landlock/task.c | 129 ++++++++++++++++++++++++++++++++++ > 6 files changed, 198 insertions(+), 8 deletions(-) > > +static bool sock_is_scoped(struct sock *const other, > + const struct landlock_ruleset *const dom) Please rename "dom" to "domain". Function arguments with full names make the API more consistent and easier to understand. > +{ > + const struct landlock_ruleset *dom_other; > + > + /* 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_is_scoped(dom, dom_other, > + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET); > +} > + > +static bool is_abstract_socket(struct sock *const sock) > +{ > + struct unix_address *addr = unix_sk(sock)->addr; > + > + if (!addr) > + return false; > + > + if (addr->len >= offsetof(struct sockaddr_un, sun_path) + 1 && > + addr->name[0].sun_path[0] == '\0') > + return true; > + > + return false; Much better! > +} > + > +static int hook_unix_stream_connect(struct sock *const sock, > + struct sock *const other, > + struct sock *const newsk) > +{ > + const struct landlock_ruleset *const dom = > + landlock_get_current_domain(); > + > + /* quick return for non-sandboxed processes */ > + if (!dom) > + return 0; > + > + if (is_abstract_socket(other)) > + if (sock_is_scoped(other, dom)) if (is_abstract_socket(other) && sock_is_scoped(other, dom)) (We might want to extend this hook in the future but we'll revise this notation when needed) > + return -EPERM; > + > + return 0; > +} > + > +static int hook_unix_may_send(struct socket *const sock, > + struct socket *const other) > +{ > + const struct landlock_ruleset *const dom = > + landlock_get_current_domain(); > + > + if (!dom) > + return 0; > + > + if (is_abstract_socket(other->sk)) > + if (sock_is_scoped(other->sk, dom)) ditto > + return -EPERM; > + > + return 0; > +} > + > 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 > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 1/5] Landlock: Add abstract unix socket connect restriction 2024-08-14 6:22 ` [PATCH v9 1/5] " Tahera Fahimi 2024-08-16 21:19 ` Mickaël Salaün @ 2024-08-19 15:37 ` Mickaël Salaün 2024-08-19 22:20 ` Tahera Fahimi 2024-08-19 19:35 ` Mickaël Salaün 2 siblings, 1 reply; 21+ messages in thread From: Mickaël Salaün @ 2024-08-19 15:37 UTC (permalink / raw) To: Tahera Fahimi Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Wed, Aug 14, 2024 at 12:22:19AM -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 We should follow the man page style and use "UNIX" everywhere. > the same landlock domain. It implements two hooks, unix_stream_connect We should always write "Landlock" in doc/comment/commit messages, except for subject prefixes because of the file names (e.g. security/landlock). > 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> > > --- > v9: > - Editting inline comments. > - Major refactoring in domain_is_scoped() and is_abstract_socket > 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 | 27 +++++++ > security/landlock/limits.h | 3 + > security/landlock/ruleset.c | 7 +- > security/landlock/ruleset.h | 23 +++++- > security/landlock/syscalls.c | 17 +++-- > security/landlock/task.c | 129 ++++++++++++++++++++++++++++++++++ > 6 files changed, 198 insertions(+), 8 deletions(-) > > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h > index 68625e728f43..057a4811ed06 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). Missing space > + */ > + __u64 scoped; > }; > > /* > @@ -266,4 +272,25 @@ 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 > + * > + * 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*/ Please add a newline here. > #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*/ "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; > + Plesae add the same comment as for similar helpers explaining why this should never happen. > + 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..20d2a8b5aa42 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,8 +171,9 @@ 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; > - * - %E2BIG or %EFAULT: @attr or @size inconsistencies; > + * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size; > + * - %E2BIG: @attr or @size inconsistencies; > + * - %EFAULT: @attr or @size inconsistencies; > * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs. > */ > SYSCALL_DEFINE3(landlock_create_ruleset, > @@ -213,9 +215,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; A test should check that, similarly to layout0.ruleset_with_unknown_access, which should be updated for the signal patch series too. You can put this test in a dedicated scoped_test.c file because it would be common to all scoped restrictions > + > /* 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..a461923c0571 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/af_unix.h> > +#include <net/sock.h> > > #include "common.h" > #include "cred.h" > @@ -108,9 +110,136 @@ static int hook_ptrace_traceme(struct task_struct *const parent) > return task_ptrace(parent, current); > } > > +/** > + * domain_is_scoped - Checks if the client domain is scoped in the same > + * domain as the server. > + * > + * @client: IPC sender domain. > + * @server: IPC receiver domain. > + * > + * Return true if the @client domain is scoped to access the @server, > + * unless the @server is also scoped in the same domain as @client. > + */ > +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; > + > + /* Quick return if client has no domain */ > + 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)); > + > + 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. > + */ > + 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. > + */ > + return server_walker != client_walker; > + } > + client_walker = client_walker->parent; > + server_walker = server_walker->parent; > + } > + return false; > +} > + > +static bool sock_is_scoped(struct sock *const other, > + const struct landlock_ruleset *const dom) > +{ > + const struct landlock_ruleset *dom_other; > + > + /* 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_is_scoped(dom, dom_other, > + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET); > +} > + > +static bool is_abstract_socket(struct sock *const sock) > +{ > + struct unix_address *addr = unix_sk(sock)->addr; > + > + if (!addr) > + return false; > + > + if (addr->len >= offsetof(struct sockaddr_un, sun_path) + 1 && > + addr->name[0].sun_path[0] == '\0') > + return true; We don't check for invalid addr values but that's OK because unix_validate_addr() was called before the hooks, and we don't need to handle -EINVAL. However, we should have test that creates a socketpair in a parent process, and check that the scoped child process can still connect (with a stream one, and send data with a datagram one) to this socket because it is not tied to an abstract unix address. I think the kernel code should not need any change, but otherwise unix_may_send() should help. Anyway, I'd like a comment explaining why we don't need the same checks as unix_may_send(). I'm also worried that a connected socket on which we send data with sendto() (with NULL and 0) could be denied, which would not be correct. I think this is OK because security_unix_may_send() is only called by unix_dgram_sendmsg() and unix_dgram_connect(), and unix_stream_sendmsg() doesn't call any hook. Please add a test to prove this. > + > + return false; > +} > + > +static int hook_unix_stream_connect(struct sock *const sock, > + struct sock *const other, > + struct sock *const newsk) > +{ > + const struct landlock_ruleset *const dom = > + landlock_get_current_domain(); > + > + /* quick return for non-sandboxed processes */ > + if (!dom) > + return 0; > + > + if (is_abstract_socket(other)) > + if (sock_is_scoped(other, dom)) > + return -EPERM; > + > + return 0; > +} > + > +static int hook_unix_may_send(struct socket *const sock, > + struct socket *const other) > +{ > + const struct landlock_ruleset *const dom = > + landlock_get_current_domain(); > + > + if (!dom) > + return 0; > + > + if (is_abstract_socket(other->sk)) > + if (sock_is_scoped(other->sk, dom)) > + return -EPERM; > + > + return 0; > +} > + > 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 > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 1/5] Landlock: Add abstract unix socket connect restriction 2024-08-19 15:37 ` Mickaël Salaün @ 2024-08-19 22:20 ` Tahera Fahimi 2024-08-20 15:56 ` Mickaël Salaün 0 siblings, 1 reply; 21+ messages in thread From: Tahera Fahimi @ 2024-08-19 22:20 UTC (permalink / raw) To: Mickaël Salaün Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Mon, Aug 19, 2024 at 05:37:23PM +0200, Mickaël Salaün wrote: > On Wed, Aug 14, 2024 at 12:22:19AM -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 > > We should follow the man page style and use "UNIX" everywhere. > > > the same landlock domain. It implements two hooks, unix_stream_connect > > We should always write "Landlock" in doc/comment/commit messages, except > for subject prefixes because of the file names (e.g. security/landlock). > > > > 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> > > > > --- > > v9: > > - Editting inline comments. > > - Major refactoring in domain_is_scoped() and is_abstract_socket > > 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 | 27 +++++++ > > security/landlock/limits.h | 3 + > > security/landlock/ruleset.c | 7 +- > > security/landlock/ruleset.h | 23 +++++- > > security/landlock/syscalls.c | 17 +++-- > > security/landlock/task.c | 129 ++++++++++++++++++++++++++++++++++ > > 6 files changed, 198 insertions(+), 8 deletions(-) > > > > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h > > index 68625e728f43..057a4811ed06 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). > > Missing space > > > + */ > > + __u64 scoped; > > }; > > > > /* > > @@ -266,4 +272,25 @@ 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 > > + * > > + * 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*/ > > Please add a newline here. > > > #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*/ > > "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; > > + > > Plesae add the same comment as for similar helpers explaining why this > should never happen. > > > + 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..20d2a8b5aa42 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,8 +171,9 @@ 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; > > - * - %E2BIG or %EFAULT: @attr or @size inconsistencies; > > + * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size; > > + * - %E2BIG: @attr or @size inconsistencies; > > + * - %EFAULT: @attr or @size inconsistencies; > > * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs. > > */ > > SYSCALL_DEFINE3(landlock_create_ruleset, > > @@ -213,9 +215,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; > > A test should check that, similarly to > layout0.ruleset_with_unknown_access, which should be updated for the > signal patch series too. You can put this test in a dedicated > scoped_test.c file because it would be common to all scoped restrictions > > > + > > /* 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..a461923c0571 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/af_unix.h> > > +#include <net/sock.h> > > > > #include "common.h" > > #include "cred.h" > > @@ -108,9 +110,136 @@ static int hook_ptrace_traceme(struct task_struct *const parent) > > return task_ptrace(parent, current); > > } > > > > +/** > > + * domain_is_scoped - Checks if the client domain is scoped in the same > > + * domain as the server. > > + * > > + * @client: IPC sender domain. > > + * @server: IPC receiver domain. > > + * > > + * Return true if the @client domain is scoped to access the @server, > > + * unless the @server is also scoped in the same domain as @client. > > + */ > > +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; > > + > > + /* Quick return if client has no domain */ > > + 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)); > > + > > + 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. > > + */ > > + 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. > > + */ > > + return server_walker != client_walker; > > + } > > + client_walker = client_walker->parent; > > + server_walker = server_walker->parent; > > + } > > + return false; > > +} > > + > > +static bool sock_is_scoped(struct sock *const other, > > + const struct landlock_ruleset *const dom) > > +{ > > + const struct landlock_ruleset *dom_other; > > + > > + /* 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_is_scoped(dom, dom_other, > > + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET); > > +} > > + > > +static bool is_abstract_socket(struct sock *const sock) > > +{ > > + struct unix_address *addr = unix_sk(sock)->addr; > > + > > + if (!addr) > > + return false; > > + > > + if (addr->len >= offsetof(struct sockaddr_un, sun_path) + 1 && > > + addr->name[0].sun_path[0] == '\0') > > + return true; > > We don't check for invalid addr values but that's OK because > unix_validate_addr() was called before the hooks, and we don't need to > handle -EINVAL. > > However, we should have test that creates a socketpair in a parent > process, and check that the scoped child process can still connect (with > a stream one, and send data with a datagram one) to this socket because > it is not tied to an abstract unix address. I think the kernel code I think we have covered this case for stream sockets in pathname_address_format test (I will add the case for dgram as well). > should not need any change, but otherwise unix_may_send() should help. > Anyway, I'd like a comment explaining why we don't need the same checks > as unix_may_send(). I did not fully understand this part, can you please explain what do you mean by same checks? > I'm also worried that a connected socket on which we send data with > sendto() (with NULL and 0) could be denied, which would not be correct. > I think this is OK because security_unix_may_send() is only called by > unix_dgram_sendmsg() and unix_dgram_connect(), and unix_stream_sendmsg() > doesn't call any hook. Please add a test to prove this. Correct. The security_unix_may_send() is only used by the dgram sockets, and because it is not a connected connection, every time it tries to send a packet, it will check if it has permission to send a packet (which is not the case for connected sockets.) I can add a test where the process is scoped in the middle of the connection, so the stream connection should still be connected, but the dgram connection should fail to send a packet. Is this the type of test case you had in mind? > > + > > + return false; > > +} > > + > > +static int hook_unix_stream_connect(struct sock *const sock, > > + struct sock *const other, > > + struct sock *const newsk) > > +{ > > + const struct landlock_ruleset *const dom = > > + landlock_get_current_domain(); > > + > > + /* quick return for non-sandboxed processes */ > > + if (!dom) > > + return 0; > > + > > + if (is_abstract_socket(other)) > > + if (sock_is_scoped(other, dom)) > > + return -EPERM; > > + > > + return 0; > > +} > > + > > +static int hook_unix_may_send(struct socket *const sock, > > + struct socket *const other) > > +{ > > + const struct landlock_ruleset *const dom = > > + landlock_get_current_domain(); > > + > > + if (!dom) > > + return 0; > > + > > + if (is_abstract_socket(other->sk)) > > + if (sock_is_scoped(other->sk, dom)) > > + return -EPERM; > > + > > + return 0; > > +} > > + > > 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 > > > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 1/5] Landlock: Add abstract unix socket connect restriction 2024-08-19 22:20 ` Tahera Fahimi @ 2024-08-20 15:56 ` Mickaël Salaün 0 siblings, 0 replies; 21+ messages in thread From: Mickaël Salaün @ 2024-08-20 15:56 UTC (permalink / raw) To: Tahera Fahimi Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Mon, Aug 19, 2024 at 04:20:28PM -0600, Tahera Fahimi wrote: > On Mon, Aug 19, 2024 at 05:37:23PM +0200, Mickaël Salaün wrote: > > On Wed, Aug 14, 2024 at 12:22:19AM -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 > > > > We should follow the man page style and use "UNIX" everywhere. > > > > > the same landlock domain. It implements two hooks, unix_stream_connect > > > > We should always write "Landlock" in doc/comment/commit messages, except > > for subject prefixes because of the file names (e.g. security/landlock). > > > > > > > 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> > > > > > > --- > > > v9: > > > - Editting inline comments. > > > - Major refactoring in domain_is_scoped() and is_abstract_socket > > > 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 | 27 +++++++ > > > security/landlock/limits.h | 3 + > > > security/landlock/ruleset.c | 7 +- > > > security/landlock/ruleset.h | 23 +++++- > > > security/landlock/syscalls.c | 17 +++-- > > > security/landlock/task.c | 129 ++++++++++++++++++++++++++++++++++ > > > 6 files changed, 198 insertions(+), 8 deletions(-) > > > > > > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h > > > index 68625e728f43..057a4811ed06 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). > > > > Missing space > > > > > + */ > > > + __u64 scoped; > > > }; > > > > > > /* > > > @@ -266,4 +272,25 @@ 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 > > > + * > > > + * 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*/ > > > > Please add a newline here. > > > > > #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*/ > > > > "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; > > > + > > > > Plesae add the same comment as for similar helpers explaining why this > > should never happen. > > > > > + 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..20d2a8b5aa42 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,8 +171,9 @@ 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; > > > - * - %E2BIG or %EFAULT: @attr or @size inconsistencies; > > > + * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size; > > > + * - %E2BIG: @attr or @size inconsistencies; > > > + * - %EFAULT: @attr or @size inconsistencies; > > > * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs. > > > */ > > > SYSCALL_DEFINE3(landlock_create_ruleset, > > > @@ -213,9 +215,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; > > > > A test should check that, similarly to > > layout0.ruleset_with_unknown_access, which should be updated for the > > signal patch series too. You can put this test in a dedicated > > scoped_test.c file because it would be common to all scoped restrictions > > > > > + > > > /* 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..a461923c0571 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/af_unix.h> > > > +#include <net/sock.h> > > > > > > #include "common.h" > > > #include "cred.h" > > > @@ -108,9 +110,136 @@ static int hook_ptrace_traceme(struct task_struct *const parent) > > > return task_ptrace(parent, current); > > > } > > > > > > +/** > > > + * domain_is_scoped - Checks if the client domain is scoped in the same > > > + * domain as the server. > > > + * > > > + * @client: IPC sender domain. > > > + * @server: IPC receiver domain. > > > + * > > > + * Return true if the @client domain is scoped to access the @server, > > > + * unless the @server is also scoped in the same domain as @client. > > > + */ > > > +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; > > > + > > > + /* Quick return if client has no domain */ > > > + 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)); > > > + > > > + 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. > > > + */ > > > + 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. > > > + */ > > > + return server_walker != client_walker; > > > + } > > > + client_walker = client_walker->parent; > > > + server_walker = server_walker->parent; > > > + } > > > + return false; > > > +} > > > + > > > +static bool sock_is_scoped(struct sock *const other, > > > + const struct landlock_ruleset *const dom) > > > +{ > > > + const struct landlock_ruleset *dom_other; > > > + > > > + /* 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_is_scoped(dom, dom_other, > > > + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET); > > > +} > > > + > > > +static bool is_abstract_socket(struct sock *const sock) > > > +{ > > > + struct unix_address *addr = unix_sk(sock)->addr; > > > + > > > + if (!addr) > > > + return false; > > > + > > > + if (addr->len >= offsetof(struct sockaddr_un, sun_path) + 1 && > > > + addr->name[0].sun_path[0] == '\0') > > > + return true; > > > > We don't check for invalid addr values but that's OK because > > unix_validate_addr() was called before the hooks, and we don't need to > > handle -EINVAL. > > > > However, we should have test that creates a socketpair in a parent > > process, and check that the scoped child process can still connect (with > > a stream one, and send data with a datagram one) to this socket because > > it is not tied to an abstract unix address. I think the kernel code > I think we have covered this case for stream sockets in > pathname_address_format test (I will add the case for dgram as well). No, there is no test checking sockets created with socketpair(2). Such sockets are only used to pass FDs. > > should not need any change, but otherwise unix_may_send() should help. > > Anyway, I'd like a comment explaining why we don't need the same checks > > as unix_may_send(). > I did not fully understand this part, can you please explain what do you > mean by same checks? unix_may_send() checks the socket's peer (NULL or same). Do we need that as well? > > > I'm also worried that a connected socket on which we send data with > > sendto() (with NULL and 0) could be denied, which would not be correct. > > I think this is OK because security_unix_may_send() is only called by > > unix_dgram_sendmsg() and unix_dgram_connect(), and unix_stream_sendmsg() > > doesn't call any hook. Please add a test to prove this. > Correct. The security_unix_may_send() is only used by the dgram sockets, > and because it is not a connected connection, every time it tries to > send a packet, it will check if it has permission to send a packet > (which is not the case for connected sockets.) I can add a test where > the process is scoped in the middle of the connection, so the stream > connection should still be connected, but the dgram connection should > fail to send a packet. Is this the type of test case you had in mind? Yes please add such test where we can send(2) and sendto(2) with the connected stream socket, whereas we should not be able to send(2) nor sendto(2) with a (not-connected) datagram socket. Thinking more about the connected datagram socket, it makes more sense to follow the same semantic as for a connected stream socket. If a datagram socket is connected and if the "other" socket is the connected peer, then the unix_may_send() should allow the related send(2) or sendto(2). This should work as follow: > > > + > > > + return false; > > > +} > > > + > > > +static int hook_unix_stream_connect(struct sock *const sock, > > > + struct sock *const other, > > > + struct sock *const newsk) > > > +{ > > > + const struct landlock_ruleset *const dom = > > > + landlock_get_current_domain(); > > > + > > > + /* quick return for non-sandboxed processes */ > > > + if (!dom) > > > + return 0; > > > + > > > + if (is_abstract_socket(other)) > > > + if (sock_is_scoped(other, dom)) > > > + return -EPERM; > > > + > > > + return 0; > > > +} > > > + > > > +static int hook_unix_may_send(struct socket *const sock, > > > + struct socket *const other) > > > +{ > > > + const struct landlock_ruleset *const dom = > > > + landlock_get_current_domain(); > > > + > > > + if (!dom) > > > + return 0; > > > + > > > + if (is_abstract_socket(other->sk)) > > > + if (sock_is_scoped(other->sk, dom)) > > > + return -EPERM; if (is_abstract_socket(other->sk)) { /* * Checks if this datagram socket was already allowed to be * connected to other. */ if (unix_peer(sock->sk) == other->sk) return 0 if (sock_is_scoped(other->sk, dom)) return -EPERM; } Please add another test to check this specific case where the datagram socket is connected and the send/sendto works, whereas it is denied if the datagram socket is not connected. The documentation needs to be updated accordingly to explain the semantic and the differences (it should now be the same) between datagram and stream sockets. We could also allow sockets to send data or connect to themselves (sock == other), but I think it is not a good idea because the socket might be shared and the sender might not be the only one receiving the data, hence breaking the sopping semantic. Please add another test for this case (where the socket was created by a parent and the scoped child is denied to connect or send data to its inherited FD). The documentation should also mention this case and explain the rationale. > > > + > > > + return 0; > > > +} > > > + > > > 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 > > > > > > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 1/5] Landlock: Add abstract unix socket connect restriction 2024-08-14 6:22 ` [PATCH v9 1/5] " Tahera Fahimi 2024-08-16 21:19 ` Mickaël Salaün 2024-08-19 15:37 ` Mickaël Salaün @ 2024-08-19 19:35 ` Mickaël Salaün 2 siblings, 0 replies; 21+ messages in thread From: Mickaël Salaün @ 2024-08-19 19:35 UTC (permalink / raw) To: Tahera Fahimi Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Wed, Aug 14, 2024 at 12:22:19AM -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> > > --- > diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c > index 03b470f5a85a..20d2a8b5aa42 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 Each test need to pass with each commit (not only this one BTW), so we need to update the abi_version test with this commit. To be sure that everything is OK, you can run `check-linux.sh all` on each commit. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 2/5] selftests/Landlock: Abstract unix socket restriction tests 2024-08-14 6:22 [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction Tahera Fahimi 2024-08-14 6:22 ` [PATCH v9 1/5] " Tahera Fahimi @ 2024-08-14 6:22 ` Tahera Fahimi 2024-08-16 21:23 ` Mickaël Salaün 2024-08-19 15:42 ` Mickaël Salaün 2024-08-14 6:22 ` [PATCH v9 3/5] selftests/Landlock: Adding pathname Unix socket tests Tahera Fahimi ` (3 subsequent siblings) 5 siblings, 2 replies; 21+ messages in thread From: Tahera Fahimi @ 2024-08-14 6:22 UTC (permalink / raw) To: outreachy Cc: mic, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev, Tahera Fahimi The patch introduces Landlock ABI version 6 and has three types of tests that examines different scenarios for abstract unix socket connection: 1) unix_socket: base tests of the abstract socket scoping mechanism for a landlocked process, same as the ptrace test. 2) optional_scoping: generates three processes with different domains and tests if a process with a non-scoped domain can connect to other processes. 3) unix_sock_special_cases: since the socket's creator credentials are used for scoping sockets, this test examines the cases where the socket's credentials are different from the process using it. Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> --- Changes in versions: v9: - Move pathname_address_sockets to a different patch. - Extend optional_scoping test scenarios. - Removing hardcoded numbers and using "backlog" instead. V8: - Move tests to scoped_abstract_unix_test.c file. - To avoid potential conflicts among Unix socket names in different tests, set_unix_address is added to common.h to set different sun_path for Unix sockets. - protocol_variant and service_fixture structures are also moved to common.h - Adding pathname_address_sockets to cover all types of address formats for unix sockets, and moving remove_path() to common.h to reuse in this test. V7: - Introducing landlock ABI version 6. - Adding some edge test cases to optional_scoping test. - Using `enum` for different domains in optional_scoping tests. - Extend unix_sock_special_cases test cases for connected(SOCK_STREAM) sockets. - Modifying inline comments. V6: - Introducing optional_scoping test which ensures a sandboxed process with a non-scoped domain can still connect to another abstract unix socket(either sandboxed or non-sandboxed). - Introducing unix_sock_special_cases test which tests examines scenarios where the connecting sockets have different domain than the process using them. V4: - Introducing unix_socket to evaluate the basic scoping mechanism for abstract unix sockets. --- tools/testing/selftests/landlock/base_test.c | 2 +- tools/testing/selftests/landlock/common.h | 38 + tools/testing/selftests/landlock/net_test.c | 31 +- .../landlock/scoped_abstract_unix_test.c | 942 ++++++++++++++++++ 4 files changed, 982 insertions(+), 31 deletions(-) create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c index 3c1e9f35b531..52b00472a487 100644 --- a/tools/testing/selftests/landlock/base_test.c +++ b/tools/testing/selftests/landlock/base_test.c @@ -75,7 +75,7 @@ TEST(abi_version) const struct landlock_ruleset_attr ruleset_attr = { .handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE, }; - ASSERT_EQ(5, landlock_create_ruleset(NULL, 0, + ASSERT_EQ(6, landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION)); ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0, diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h index 7e2b431b9f90..cca387df86c2 100644 --- a/tools/testing/selftests/landlock/common.h +++ b/tools/testing/selftests/landlock/common.h @@ -7,6 +7,7 @@ * Copyright © 2021 Microsoft Corporation */ +#include <arpa/inet.h> #include <errno.h> #include <linux/landlock.h> #include <linux/securebits.h> @@ -14,10 +15,12 @@ #include <sys/socket.h> #include <sys/syscall.h> #include <sys/types.h> +#include <sys/un.h> #include <sys/wait.h> #include <unistd.h> #include "../kselftest_harness.h" +#define TMP_DIR "tmp" #ifndef __maybe_unused #define __maybe_unused __attribute__((__unused__)) @@ -226,3 +229,38 @@ enforce_ruleset(struct __test_metadata *const _metadata, const int ruleset_fd) TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); } } + +struct protocol_variant { + int domain; + int type; +}; + +struct service_fixture { + struct protocol_variant protocol; + /* port is also stored in ipv4_addr.sin_port or ipv6_addr.sin6_port */ + unsigned short port; + union { + struct sockaddr_in ipv4_addr; + struct sockaddr_in6 ipv6_addr; + struct { + struct sockaddr_un unix_addr; + socklen_t unix_addr_len; + }; + }; +}; + +static pid_t __maybe_unused sys_gettid(void) +{ + return syscall(__NR_gettid); +} + +static void __maybe_unused set_unix_address(struct service_fixture *const srv, + const unsigned short index) +{ + srv->unix_addr.sun_family = AF_UNIX; + sprintf(srv->unix_addr.sun_path, + "_selftests-landlock-abstract-unix-tid%d-index%d", sys_gettid(), + index); + srv->unix_addr_len = SUN_LEN(&srv->unix_addr); + srv->unix_addr.sun_path[0] = '\0'; +} diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c index f21cfbbc3638..4e0aeb53b225 100644 --- a/tools/testing/selftests/landlock/net_test.c +++ b/tools/testing/selftests/landlock/net_test.c @@ -36,30 +36,6 @@ enum sandbox_type { TCP_SANDBOX, }; -struct protocol_variant { - int domain; - int type; -}; - -struct service_fixture { - struct protocol_variant protocol; - /* port is also stored in ipv4_addr.sin_port or ipv6_addr.sin6_port */ - unsigned short port; - union { - struct sockaddr_in ipv4_addr; - struct sockaddr_in6 ipv6_addr; - struct { - struct sockaddr_un unix_addr; - socklen_t unix_addr_len; - }; - }; -}; - -static pid_t sys_gettid(void) -{ - return syscall(__NR_gettid); -} - static int set_service(struct service_fixture *const srv, const struct protocol_variant prot, const unsigned short index) @@ -92,12 +68,7 @@ static int set_service(struct service_fixture *const srv, return 0; case AF_UNIX: - srv->unix_addr.sun_family = prot.domain; - sprintf(srv->unix_addr.sun_path, - "_selftests-landlock-net-tid%d-index%d", sys_gettid(), - index); - srv->unix_addr_len = SUN_LEN(&srv->unix_addr); - srv->unix_addr.sun_path[0] = '\0'; + set_unix_address(srv, index); return 0; } return 1; diff --git a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c new file mode 100644 index 000000000000..232c3b767b8a --- /dev/null +++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c @@ -0,0 +1,942 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Landlock tests - Abstract Unix Socket + * + * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net> + * Copyright © 2019-2020 ANSSI + */ + +#define _GNU_SOURCE +#include <errno.h> +#include <fcntl.h> +#include <linux/landlock.h> +#include <sched.h> +#include <signal.h> +#include <stddef.h> +#include <sys/prctl.h> +#include <sys/socket.h> +#include <sys/stat.h> +#include <sys/types.h> +#include <sys/un.h> +#include <sys/wait.h> +#include <unistd.h> + +#include "common.h" + +/* Number pending connections queue to be hold. */ +const short backlog = 10; + +static void create_fs_domain(struct __test_metadata *const _metadata) +{ + int ruleset_fd; + struct landlock_ruleset_attr ruleset_attr = { + .handled_access_fs = LANDLOCK_ACCESS_FS_READ_DIR, + }; + + ruleset_fd = + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); + EXPECT_LE(0, ruleset_fd) + { + TH_LOG("Failed to create a ruleset: %s", strerror(errno)); + } + EXPECT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); + EXPECT_EQ(0, landlock_restrict_self(ruleset_fd, 0)); + EXPECT_EQ(0, close(ruleset_fd)); +} + +static void create_unix_domain(struct __test_metadata *const _metadata) +{ + int ruleset_fd; + const struct landlock_ruleset_attr ruleset_attr = { + .scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET, + }; + + ruleset_fd = + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); + EXPECT_LE(0, ruleset_fd) + { + TH_LOG("Failed to create a ruleset: %s", strerror(errno)); + } + enforce_ruleset(_metadata, ruleset_fd); + EXPECT_EQ(0, close(ruleset_fd)); +} + +/* clang-format off */ +FIXTURE(unix_socket) +{ + struct service_fixture stream_address, dgram_address; + int server, client, dgram_server, dgram_client; +}; + +/* clang-format on */ +FIXTURE_VARIANT(unix_socket) +{ + bool domain_both; + bool domain_parent; + bool domain_child; + bool connect_to_parent; +}; + +FIXTURE_SETUP(unix_socket) +{ + memset(&self->stream_address, 0, sizeof(self->stream_address)); + memset(&self->dgram_address, 0, sizeof(self->dgram_address)); + + set_unix_address(&self->stream_address, 0); + set_unix_address(&self->dgram_address, 1); +} + +FIXTURE_TEARDOWN(unix_socket) +{ + close(self->server); + close(self->client); + close(self->dgram_server); + close(self->dgram_client); +} + +/* + * No domain + * + * P1-. P1 -> P2 : allow + * \ P2 -> P1 : allow + * 'P2 + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, allow_without_domain_connect_to_parent) { + /* clang-format on */ + .domain_both = false, + .domain_parent = false, + .domain_child = false, + .connect_to_parent = true, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, allow_without_domain_connect_to_child) { + /* clang-format on */ + .domain_both = false, + .domain_parent = false, + .domain_child = false, + .connect_to_parent = false, +}; + +/* + * Child domain + * + * P1--. P1 -> P2 : allow + * \ P2 -> P1 : deny + * .'-----. + * | P2 | + * '------' + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, deny_with_one_domain_connect_to_parent) { + /* clang-format on */ + .domain_both = false, + .domain_parent = false, + .domain_child = true, + .connect_to_parent = true, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, allow_with_one_domain_connect_to_child) { + /* clang-format on */ + .domain_both = false, + .domain_parent = false, + .domain_child = true, + .connect_to_parent = false, +}; + +/* + * Parent domain + * .------. + * | P1 --. P1 -> P2 : deny + * '------' \ P2 -> P1 : allow + * ' + * P2 + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, allow_with_parent_domain_connect_to_parent) { + /* clang-format on */ + .domain_both = false, + .domain_parent = true, + .domain_child = false, + .connect_to_parent = true, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, deny_with_parent_domain_connect_to_child) { + /* clang-format on */ + .domain_both = false, + .domain_parent = true, + .domain_child = false, + .connect_to_parent = false, +}; + +/* + * Parent + child domain (siblings) + * .------. + * | P1 ---. P1 -> P2 : deny + * '------' \ P2 -> P1 : deny + * .---'--. + * | P2 | + * '------' + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, deny_with_sibling_domain_connect_to_parent) { + /* clang-format on */ + .domain_both = false, + .domain_parent = true, + .domain_child = true, + .connect_to_parent = true, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, deny_with_sibling_domain_connect_to_child) { + /* clang-format on */ + .domain_both = false, + .domain_parent = true, + .domain_child = true, + .connect_to_parent = false, +}; + +/* + * Same domain (inherited) + * .-------------. + * | P1----. | P1 -> P2 : allow + * | \ | P2 -> P1 : allow + * | ' | + * | P2 | + * '-------------' + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, allow_inherited_domain_connect_to_parent) { + /* clang-format on */ + .domain_both = true, + .domain_parent = false, + .domain_child = false, + .connect_to_parent = true, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, allow_inherited_domain_connect_to_child) { + /* clang-format on */ + .domain_both = true, + .domain_parent = false, + .domain_child = false, + .connect_to_parent = false, +}; + +/* + * Inherited + child domain + * .-----------------. + * | P1----. | P1 -> P2 : allow + * | \ | P2 -> P1 : deny + * | .-'----. | + * | | P2 | | + * | '------' | + * '-----------------' + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, deny_nested_domain_connect_to_parent) { + /* clang-format on */ + .domain_both = true, + .domain_parent = false, + .domain_child = true, + .connect_to_parent = true, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, allow_nested_domain_connect_to_child) { + /* clang-format on */ + .domain_both = true, + .domain_parent = false, + .domain_child = true, + .connect_to_parent = false, +}; + +/* + * Inherited + parent domain + * .-----------------. + * |.------. | P1 -> P2 : deny + * || P1 ----. | P2 -> P1 : allow + * |'------' \ | + * | ' | + * | P2 | + * '-----------------' + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, allow_with_nested_and_parent_domain_connect_to_parent) { + /* clang-format on */ + .domain_both = true, + .domain_parent = true, + .domain_child = false, + .connect_to_parent = true, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, deny_with_nested_and_parent_domain_connect_to_child) { + /* clang-format on */ + .domain_both = true, + .domain_parent = true, + .domain_child = false, + .connect_to_parent = false, +}; + +/* + * Inherited + parent and child domain (siblings) + * .-----------------. + * | .------. | P1 -> P2 : deny + * | | P1 . | P2 -> P1 : deny + * | '------'\ | + * | \ | + * | .--'---. | + * | | P2 | | + * | '------' | + * '-----------------' + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, deny_with_forked_domain_connect_to_parent) { + /* clang-format on */ + .domain_both = true, + .domain_parent = true, + .domain_child = true, + .connect_to_parent = true, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_socket, deny_with_forked_domain_connect_to_child) { + /* clang-format on */ + .domain_both = true, + .domain_parent = true, + .domain_child = true, + .connect_to_parent = false, +}; + +/* + * Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent and child, + * when they have scoped domain or no domain. + */ +TEST_F(unix_socket, abstract_unix_socket) +{ + int status; + pid_t child; + bool can_connect_to_parent, can_connect_to_child; + int err, err_dgram; + int pipe_child[2], pipe_parent[2]; + char buf_parent; + + /* + * can_connect_to_child is true if a parent process can connect to its + * child process. The parent process is not isolated from the child + * with a dedicated Landlock domain. + */ + can_connect_to_child = !variant->domain_parent; + /* + * can_connect_to_parent is true if a child process can connect to its + * parent process. This depends on the child process is not isolated from + * the parent with a dedicated Landlock domain. + */ + can_connect_to_parent = !variant->domain_child; + + ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC)); + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC)); + if (variant->domain_both) { + create_unix_domain(_metadata); + if (!__test_passed(_metadata)) + return; + } + + child = fork(); + ASSERT_LE(0, child); + if (child == 0) { + char buf_child; + + ASSERT_EQ(0, close(pipe_parent[1])); + ASSERT_EQ(0, close(pipe_child[0])); + if (variant->domain_child) + create_unix_domain(_metadata); + + /* Waits for the parent to be in a domain, if any. */ + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); + + if (variant->connect_to_parent) { + self->client = socket(AF_UNIX, SOCK_STREAM, 0); + self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0); + + ASSERT_NE(-1, self->client); + ASSERT_NE(-1, self->dgram_client); + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); + + err = connect(self->client, + &self->stream_address.unix_addr, + (self->stream_address).unix_addr_len); + err_dgram = + connect(self->dgram_client, + &self->dgram_address.unix_addr, + (self->dgram_address).unix_addr_len); + + if (can_connect_to_parent) { + EXPECT_EQ(0, err); + EXPECT_EQ(0, err_dgram); + } else { + EXPECT_EQ(-1, err); + EXPECT_EQ(-1, err_dgram); + EXPECT_EQ(EPERM, errno); + } + } else { + self->server = socket(AF_UNIX, SOCK_STREAM, 0); + self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0); + ASSERT_NE(-1, self->server); + ASSERT_NE(-1, self->dgram_server); + + ASSERT_EQ(0, + bind(self->server, + &self->stream_address.unix_addr, + (self->stream_address).unix_addr_len)); + ASSERT_EQ(0, bind(self->dgram_server, + &self->dgram_address.unix_addr, + (self->dgram_address).unix_addr_len)); + ASSERT_EQ(0, listen(self->server, backlog)); + + /* signal to parent that child is listening */ + ASSERT_EQ(1, write(pipe_child[1], ".", 1)); + /* wait to connect */ + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); + } + _exit(_metadata->exit_code); + return; + } + + ASSERT_EQ(0, close(pipe_child[1])); + ASSERT_EQ(0, close(pipe_parent[0])); + + if (variant->domain_parent) + create_unix_domain(_metadata); + + /* Signals that the parent is in a domain, if any. */ + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); + + if (!variant->connect_to_parent) { + self->client = socket(AF_UNIX, SOCK_STREAM, 0); + self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0); + + ASSERT_NE(-1, self->client); + ASSERT_NE(-1, self->dgram_client); + + /* Waits for the child to listen */ + ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1)); + err = connect(self->client, &self->stream_address.unix_addr, + (self->stream_address).unix_addr_len); + err_dgram = connect(self->dgram_client, + &self->dgram_address.unix_addr, + (self->dgram_address).unix_addr_len); + + if (can_connect_to_child) { + EXPECT_EQ(0, err); + EXPECT_EQ(0, err_dgram); + } else { + EXPECT_EQ(-1, err); + EXPECT_EQ(-1, err_dgram); + EXPECT_EQ(EPERM, errno); + } + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); + } else { + self->server = socket(AF_UNIX, SOCK_STREAM, 0); + self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0); + ASSERT_NE(-1, self->server); + ASSERT_NE(-1, self->dgram_server); + ASSERT_EQ(0, bind(self->server, &self->stream_address.unix_addr, + (self->stream_address).unix_addr_len)); + ASSERT_EQ(0, bind(self->dgram_server, + &self->dgram_address.unix_addr, + (self->dgram_address).unix_addr_len)); + ASSERT_EQ(0, listen(self->server, backlog)); + + /* signal to child that parent is listening */ + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); + } + + ASSERT_EQ(child, waitpid(child, &status, 0)); + + if (WIFSIGNALED(status) || !WIFEXITED(status) || + WEXITSTATUS(status) != EXIT_SUCCESS) + _metadata->exit_code = KSFT_FAIL; +} + +enum sandbox_type { + NO_SANDBOX, + SCOPE_SANDBOX, + /* Any other type of sandboxing domain */ + OTHER_SANDBOX, +}; + +/* clang-format off */ +FIXTURE(optional_scoping) +{ + struct service_fixture parent_address, child_address; + int parent_server, child_server, client; +}; + +/* clang-format on */ +FIXTURE_VARIANT(optional_scoping) +{ + const int domain_all; + const int domain_parent; + const int domain_children; + const int domain_child; + const int domain_grand_child; + const int type; +}; + +FIXTURE_SETUP(optional_scoping) +{ + memset(&self->parent_address, 0, sizeof(self->parent_address)); + memset(&self->child_address, 0, sizeof(self->child_address)); + + set_unix_address(&self->parent_address, 0); + set_unix_address(&self->child_address, 1); +} + +FIXTURE_TEARDOWN(optional_scoping) +{ + close(self->parent_server); + close(self->child_server); + close(self->client); +} + +/* + * .-----------------. + * | ####### | P3 -> P2 : allow + * | P1----# P2 # | P3 -> P1 : deny + * | # | # | + * | # P3 # | + * | ####### | + * '-----------------' + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(optional_scoping, deny_scoped) { + .domain_all = OTHER_SANDBOX, + .domain_parent = NO_SANDBOX, + .domain_children = SCOPE_SANDBOX, + .domain_child = NO_SANDBOX, + .domain_grand_child = NO_SANDBOX, + .type = SOCK_DGRAM, + /* clang-format on */ +}; + +/* + * ################### + * # ####### # P3 -> P2 : allow + * # P1----# P2 # # P3 -> P1 : deny + * # # | # # + * # # P3 # # + * # ####### # + * ################### + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(optional_scoping, all_scoped) { + .domain_all = SCOPE_SANDBOX, + .domain_parent = NO_SANDBOX, + .domain_children = SCOPE_SANDBOX, + .domain_child = NO_SANDBOX, + .domain_grand_child = NO_SANDBOX, + .type = SOCK_DGRAM, + /* clang-format on */ +}; + +/* + * .-----------------. + * | .-----. | P3 -> P2 : allow + * | P1----| P2 | | P3 -> P1 : allow + * | | | | + * | | P3 | | + * | '-----' | + * '-----------------' + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(optional_scoping, allow_with_other_domain) { + .domain_all = OTHER_SANDBOX, + .domain_parent = NO_SANDBOX, + .domain_children = OTHER_SANDBOX, + .domain_child = NO_SANDBOX, + .domain_grand_child = NO_SANDBOX, + .type = SOCK_DGRAM, + /* clang-format on */ +}; + +/* + * .----. ###### P3 -> P2 : allow + * | P1 |----# P2 # P3 -> P1 : allow + * '----' ###### + * | + * P3 + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(optional_scoping, allow_with_one_domain) { + .domain_all = NO_SANDBOX, + .domain_parent = OTHER_SANDBOX, + .domain_children = NO_SANDBOX, + .domain_child = SCOPE_SANDBOX, + .domain_grand_child = NO_SANDBOX, + .type = SOCK_DGRAM, + /* clang-format on */ +}; + +/* + * ###### .-----. P3 -> P2 : allow + * # P1 #----| P2 | P3 -> P1 : allow + * ###### '-----' + * | + * P3 + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(optional_scoping, allow_with_grand_parent_scoped) { + .domain_all = NO_SANDBOX, + .domain_parent = SCOPE_SANDBOX, + .domain_children = NO_SANDBOX, + .domain_child = OTHER_SANDBOX, + .domain_grand_child = NO_SANDBOX, + .type = SOCK_STREAM, + /* clang-format on */ +}; + +/* + * ###### ###### P3 -> P2 : allow + * # P1 #----# P2 # P3 -> P1 : allow + * ###### ###### + * | + * .----. + * | P3 | + * '----' + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(optional_scoping, allow_with_parents_domain) { + .domain_all = NO_SANDBOX, + .domain_parent = SCOPE_SANDBOX, + .domain_children = NO_SANDBOX, + .domain_child = SCOPE_SANDBOX, + .domain_grand_child = NO_SANDBOX, + .type = SOCK_STREAM, + /* clang-format on */ +}; + +/* + * ###### P3 -> P2 : deny + * # P1 #----P2 P3 -> P1 : deny + * ###### | + * | + * ###### + * # P3 # + * ###### + */ +/* clang-format off */ +FIXTURE_VARIANT_ADD(optional_scoping, deny_with_self_and_parents_domain) { + .domain_all = NO_SANDBOX, + .domain_parent = SCOPE_SANDBOX, + .domain_children = NO_SANDBOX, + .domain_child = NO_SANDBOX, + .domain_grand_child = SCOPE_SANDBOX, + .type = SOCK_STREAM, + /* clang-format on */ +}; + +/* + * Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent, child + * and grand child processes when they can have scoped or non-scoped + * domains. + **/ +TEST_F(optional_scoping, unix_scoping) +{ + pid_t child; + int status; + bool can_connect_to_parent, can_connect_to_child; + int pipe_parent[2]; + + can_connect_to_child = + (variant->domain_grand_child == SCOPE_SANDBOX) ? false : true; + + can_connect_to_parent = (!can_connect_to_child || + variant->domain_children == SCOPE_SANDBOX) ? + false : + true; + + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC)); + + if (variant->domain_all == OTHER_SANDBOX) + create_fs_domain(_metadata); + else if (variant->domain_all == SCOPE_SANDBOX) + create_unix_domain(_metadata); + + child = fork(); + ASSERT_LE(0, child); + if (child == 0) { + int pipe_child[2]; + + ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC)); + pid_t grand_child; + + if (variant->domain_children == OTHER_SANDBOX) + create_fs_domain(_metadata); + else if (variant->domain_children == SCOPE_SANDBOX) + create_unix_domain(_metadata); + + grand_child = fork(); + ASSERT_LE(0, grand_child); + if (grand_child == 0) { + ASSERT_EQ(0, close(pipe_parent[1])); + ASSERT_EQ(0, close(pipe_child[1])); + + char buf1, buf2; + int err; + + if (variant->domain_grand_child == OTHER_SANDBOX) + create_fs_domain(_metadata); + else if (variant->domain_grand_child == SCOPE_SANDBOX) + create_unix_domain(_metadata); + + self->client = socket(AF_UNIX, variant->type, 0); + ASSERT_NE(-1, self->client); + + ASSERT_EQ(1, read(pipe_child[0], &buf2, 1)); + err = connect(self->client, + &self->child_address.unix_addr, + (self->child_address).unix_addr_len); + if (can_connect_to_child) { + EXPECT_EQ(0, err); + } else { + EXPECT_EQ(-1, err); + EXPECT_EQ(EPERM, errno); + } + + if (variant->type == SOCK_STREAM) { + EXPECT_EQ(0, close(self->client)); + self->client = + socket(AF_UNIX, variant->type, 0); + ASSERT_NE(-1, self->client); + } + + ASSERT_EQ(1, read(pipe_parent[0], &buf1, 1)); + err = connect(self->client, + &self->parent_address.unix_addr, + (self->parent_address).unix_addr_len); + if (can_connect_to_parent) { + EXPECT_EQ(0, err); + } else { + EXPECT_EQ(-1, err); + EXPECT_EQ(EPERM, errno); + } + EXPECT_EQ(0, close(self->client)); + + _exit(_metadata->exit_code); + return; + } + + ASSERT_EQ(0, close(pipe_child[0])); + if (variant->domain_child == OTHER_SANDBOX) + create_fs_domain(_metadata); + else if (variant->domain_child == SCOPE_SANDBOX) + create_unix_domain(_metadata); + + self->child_server = socket(AF_UNIX, variant->type, 0); + ASSERT_NE(-1, self->child_server); + ASSERT_EQ(0, bind(self->child_server, + &self->child_address.unix_addr, + (self->child_address).unix_addr_len)); + if (variant->type == SOCK_STREAM) + ASSERT_EQ(0, listen(self->child_server, backlog)); + + ASSERT_EQ(1, write(pipe_child[1], ".", 1)); + ASSERT_EQ(grand_child, waitpid(grand_child, &status, 0)); + return; + } + ASSERT_EQ(0, close(pipe_parent[0])); + + if (variant->domain_parent == OTHER_SANDBOX) + create_fs_domain(_metadata); + else if (variant->domain_parent == SCOPE_SANDBOX) + create_unix_domain(_metadata); + + self->parent_server = socket(AF_UNIX, variant->type, 0); + ASSERT_NE(-1, self->parent_server); + ASSERT_EQ(0, bind(self->parent_server, &self->parent_address.unix_addr, + (self->parent_address).unix_addr_len)); + + if (variant->type == SOCK_STREAM) + ASSERT_EQ(0, listen(self->parent_server, backlog)); + + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); + ASSERT_EQ(child, waitpid(child, &status, 0)); + if (WIFSIGNALED(status) || !WIFEXITED(status) || + WEXITSTATUS(status) != EXIT_SUCCESS) + _metadata->exit_code = KSFT_FAIL; +} + +/* + * Since the special case of scoping only happens when the connecting socket + * is scoped, the client's domain is true for all the following test cases. + */ +/* clang-format off */ +FIXTURE(unix_sock_special_cases) { + int server_socket, client; + int stream_server, stream_client; + struct service_fixture address, transit_address; +}; + +/* clang-format on */ +FIXTURE_VARIANT(unix_sock_special_cases) +{ + const bool domain_server; + const bool domain_server_socket; + const int type; +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_sock_special_cases, allow_dgram_server_sock_domain) { + /* clang-format on */ + .domain_server = false, + .domain_server_socket = true, + .type = SOCK_DGRAM, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_sock_special_cases, deny_dgram_server_domain) { + /* clang-format on */ + .domain_server = true, + .domain_server_socket = false, + .type = SOCK_DGRAM, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_sock_special_cases, allow_stream_server_sock_domain) { + /* clang-format on */ + .domain_server = false, + .domain_server_socket = true, + .type = SOCK_STREAM, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(unix_sock_special_cases, deny_stream_server_domain) { + /* clang-format on */ + .domain_server = true, + .domain_server_socket = false, + .type = SOCK_STREAM, +}; + +FIXTURE_SETUP(unix_sock_special_cases) +{ + memset(&self->transit_address, 0, sizeof(self->transit_address)); + memset(&self->address, 0, sizeof(self->address)); + set_unix_address(&self->transit_address, 0); + set_unix_address(&self->address, 1); +} + +FIXTURE_TEARDOWN(unix_sock_special_cases) +{ + close(self->client); + close(self->server_socket); + close(self->stream_server); + close(self->stream_client); +} + +/* Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent and + * child processes when connecting socket has different domain + * than the process using it. + **/ +TEST_F(unix_sock_special_cases, socket_with_different_domain) +{ + pid_t child; + int err, status; + int pipe_child[2], pipe_parent[2]; + char buf_parent; + + ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC)); + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC)); + + child = fork(); + ASSERT_LE(0, child); + if (child == 0) { + char buf_child; + + ASSERT_EQ(0, close(pipe_parent[1])); + ASSERT_EQ(0, close(pipe_child[0])); + + /* client always has domain */ + create_unix_domain(_metadata); + + if (variant->domain_server_socket) { + int data_socket; + int fd_sock = socket(AF_UNIX, variant->type, 0); + + ASSERT_NE(-1, fd_sock); + + self->stream_server = socket(AF_UNIX, SOCK_STREAM, 0); + + ASSERT_NE(-1, self->stream_server); + ASSERT_EQ(0, + bind(self->stream_server, + &self->transit_address.unix_addr, + (self->transit_address).unix_addr_len)); + ASSERT_EQ(0, listen(self->stream_server, backlog)); + + ASSERT_EQ(1, write(pipe_child[1], ".", 1)); + + data_socket = accept(self->stream_server, NULL, NULL); + + ASSERT_EQ(0, send_fd(data_socket, fd_sock)); + ASSERT_EQ(0, close(fd_sock)); + } + + self->client = socket(AF_UNIX, variant->type, 0); + ASSERT_NE(-1, self->client); + /* wait for parent signal for connection */ + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); + + err = connect(self->client, &self->address.unix_addr, + (self->address).unix_addr_len); + if (!variant->domain_server_socket) { + EXPECT_EQ(-1, err); + EXPECT_EQ(EPERM, errno); + } else { + EXPECT_EQ(0, err); + } + _exit(_metadata->exit_code); + return; + } + + ASSERT_EQ(0, close(pipe_child[1])); + ASSERT_EQ(0, close(pipe_parent[0])); + + if (!variant->domain_server_socket) { + self->server_socket = socket(AF_UNIX, variant->type, 0); + } else { + int cli = socket(AF_UNIX, SOCK_STREAM, 0); + + ASSERT_NE(-1, cli); + ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1)); + ASSERT_EQ(0, connect(cli, &self->transit_address.unix_addr, + (self->transit_address).unix_addr_len)); + + self->server_socket = recv_fd(cli); + ASSERT_LE(0, self->server_socket); + ASSERT_EQ(0, close(cli)); + } + + ASSERT_NE(-1, self->server_socket); + + if (variant->domain_server) + create_unix_domain(_metadata); + + ASSERT_EQ(0, bind(self->server_socket, &self->address.unix_addr, + (self->address).unix_addr_len)); + if (variant->type == SOCK_STREAM) + ASSERT_EQ(0, listen(self->server_socket, backlog)); + /* signal to child that parent is listening */ + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); + + ASSERT_EQ(child, waitpid(child, &status, 0)); + + if (WIFSIGNALED(status) || !WIFEXITED(status) || + WEXITSTATUS(status) != EXIT_SUCCESS) + _metadata->exit_code = KSFT_FAIL; +} +TEST_HARNESS_MAIN -- 2.34.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v9 2/5] selftests/Landlock: Abstract unix socket restriction tests 2024-08-14 6:22 ` [PATCH v9 2/5] selftests/Landlock: Abstract unix socket restriction tests Tahera Fahimi @ 2024-08-16 21:23 ` Mickaël Salaün 2024-08-16 23:08 ` Tahera Fahimi 2024-08-19 15:42 ` Mickaël Salaün 1 sibling, 1 reply; 21+ messages in thread From: Mickaël Salaün @ 2024-08-16 21:23 UTC (permalink / raw) To: Tahera Fahimi Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev Please make sure all subject's prefixes have "landlock", not "Landlock" for consistency with current commits. On Wed, Aug 14, 2024 at 12:22:20AM -0600, Tahera Fahimi wrote: > The patch introduces Landlock ABI version 6 and has three types of tests > that examines different scenarios for abstract unix socket connection: > 1) unix_socket: base tests of the abstract socket scoping mechanism for a > landlocked process, same as the ptrace test. > 2) optional_scoping: generates three processes with different domains and > tests if a process with a non-scoped domain can connect to other > processes. > 3) unix_sock_special_cases: since the socket's creator credentials are used > for scoping sockets, this test examines the cases where the socket's > credentials are different from the process using it. > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > --- > Changes in versions: > v9: > - Move pathname_address_sockets to a different patch. > - Extend optional_scoping test scenarios. > - Removing hardcoded numbers and using "backlog" instead. You may have missed some parts of my previous review (e.g. local variables). > V8: > - Move tests to scoped_abstract_unix_test.c file. > - To avoid potential conflicts among Unix socket names in different tests, > set_unix_address is added to common.h to set different sun_path for Unix sockets. > - protocol_variant and service_fixture structures are also moved to common.h > - Adding pathname_address_sockets to cover all types of address formats > for unix sockets, and moving remove_path() to common.h to reuse in this test. > V7: > - Introducing landlock ABI version 6. > - Adding some edge test cases to optional_scoping test. > - Using `enum` for different domains in optional_scoping tests. > - Extend unix_sock_special_cases test cases for connected(SOCK_STREAM) sockets. > - Modifying inline comments. > V6: > - Introducing optional_scoping test which ensures a sandboxed process with a > non-scoped domain can still connect to another abstract unix socket(either > sandboxed or non-sandboxed). > - Introducing unix_sock_special_cases test which tests examines scenarios where > the connecting sockets have different domain than the process using them. > V4: > - Introducing unix_socket to evaluate the basic scoping mechanism for abstract > unix sockets. > --- > tools/testing/selftests/landlock/base_test.c | 2 +- > tools/testing/selftests/landlock/common.h | 38 + > tools/testing/selftests/landlock/net_test.c | 31 +- > .../landlock/scoped_abstract_unix_test.c | 942 ++++++++++++++++++ > 4 files changed, 982 insertions(+), 31 deletions(-) > create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c > > +static void create_unix_domain(struct __test_metadata *const _metadata) > +{ > + int ruleset_fd; > + const struct landlock_ruleset_attr ruleset_attr = { > + .scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET, > + }; > + > + ruleset_fd = > + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); > + EXPECT_LE(0, ruleset_fd) > + { > + TH_LOG("Failed to create a ruleset: %s", strerror(errno)); > + } > + enforce_ruleset(_metadata, ruleset_fd); > + EXPECT_EQ(0, close(ruleset_fd)); > +} > + > +/* clang-format off */ It should not be required to add this clang-format comment here nor in most places, except variant declarations (that might change if we remove variables though). > +FIXTURE(unix_socket) > +{ > + struct service_fixture stream_address, dgram_address; > + int server, client, dgram_server, dgram_client; These variables don't need to be in the fixture but they should be local instead (and scoped to the if/else condition where they are used). > +}; > + > +/* clang-format on */ > +FIXTURE_VARIANT(unix_socket) > +{ > + bool domain_both; > + bool domain_parent; > + bool domain_child; > + bool connect_to_parent; > +}; > +/* > + * Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent and child, > + * when they have scoped domain or no domain. > + */ > +TEST_F(unix_socket, abstract_unix_socket) > +{ > + int status; > + pid_t child; > + bool can_connect_to_parent, can_connect_to_child; > + int err, err_dgram; > + int pipe_child[2], pipe_parent[2]; > + char buf_parent; > + > + /* > + * can_connect_to_child is true if a parent process can connect to its > + * child process. The parent process is not isolated from the child > + * with a dedicated Landlock domain. > + */ > + can_connect_to_child = !variant->domain_parent; > + /* > + * can_connect_to_parent is true if a child process can connect to its > + * parent process. This depends on the child process is not isolated from > + * the parent with a dedicated Landlock domain. > + */ > + can_connect_to_parent = !variant->domain_child; > + > + ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC)); > + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC)); > + if (variant->domain_both) { > + create_unix_domain(_metadata); > + if (!__test_passed(_metadata)) > + return; > + } > + > + child = fork(); > + ASSERT_LE(0, child); > + if (child == 0) { > + char buf_child; > + > + ASSERT_EQ(0, close(pipe_parent[1])); > + ASSERT_EQ(0, close(pipe_child[0])); > + if (variant->domain_child) > + create_unix_domain(_metadata); > + > + /* Waits for the parent to be in a domain, if any. */ > + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); > + > + if (variant->connect_to_parent) { > + self->client = socket(AF_UNIX, SOCK_STREAM, 0); int stream_client; stream_client = socket(AF_UNIX, SOCK_STREAM, 0); ditto for dgram_client > + self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0); > + > + ASSERT_NE(-1, self->client); > + ASSERT_NE(-1, self->dgram_client); > + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); > + > + err = connect(self->client, > + &self->stream_address.unix_addr, > + (self->stream_address).unix_addr_len); > + err_dgram = > + connect(self->dgram_client, > + &self->dgram_address.unix_addr, > + (self->dgram_address).unix_addr_len); > + > + if (can_connect_to_parent) { > + EXPECT_EQ(0, err); > + EXPECT_EQ(0, err_dgram); > + } else { > + EXPECT_EQ(-1, err); > + EXPECT_EQ(-1, err_dgram); > + EXPECT_EQ(EPERM, errno); > + } EXPECT_EQ(0, close(stream_client)); EXPECT_EQ(0, close(dgram_client)); > + } else { > + self->server = socket(AF_UNIX, SOCK_STREAM, 0); int stream_server; server = socket(AF_UNIX, SOCK_STREAM, 0); ditto for dgram_server > + self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0); > + ASSERT_NE(-1, self->server); > + ASSERT_NE(-1, self->dgram_server); > + > + ASSERT_EQ(0, > + bind(self->server, > + &self->stream_address.unix_addr, > + (self->stream_address).unix_addr_len)); > + ASSERT_EQ(0, bind(self->dgram_server, > + &self->dgram_address.unix_addr, > + (self->dgram_address).unix_addr_len)); > + ASSERT_EQ(0, listen(self->server, backlog)); > + > + /* signal to parent that child is listening */ > + ASSERT_EQ(1, write(pipe_child[1], ".", 1)); > + /* wait to connect */ > + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); ditto > + } > + _exit(_metadata->exit_code); > + return; > + } > + > + ASSERT_EQ(0, close(pipe_child[1])); > + ASSERT_EQ(0, close(pipe_parent[0])); > + > + if (variant->domain_parent) > + create_unix_domain(_metadata); > + > + /* Signals that the parent is in a domain, if any. */ > + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); > + > + if (!variant->connect_to_parent) { > + self->client = socket(AF_UNIX, SOCK_STREAM, 0); > + self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0); ditto > + > + ASSERT_NE(-1, self->client); > + ASSERT_NE(-1, self->dgram_client); > + > + /* Waits for the child to listen */ > + ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1)); > + err = connect(self->client, &self->stream_address.unix_addr, > + (self->stream_address).unix_addr_len); > + err_dgram = connect(self->dgram_client, > + &self->dgram_address.unix_addr, > + (self->dgram_address).unix_addr_len); > + > + if (can_connect_to_child) { > + EXPECT_EQ(0, err); > + EXPECT_EQ(0, err_dgram); > + } else { > + EXPECT_EQ(-1, err); > + EXPECT_EQ(-1, err_dgram); > + EXPECT_EQ(EPERM, errno); > + } > + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); ditto > + } else { > + self->server = socket(AF_UNIX, SOCK_STREAM, 0); > + self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0); ditto > + ASSERT_NE(-1, self->server); > + ASSERT_NE(-1, self->dgram_server); > + ASSERT_EQ(0, bind(self->server, &self->stream_address.unix_addr, > + (self->stream_address).unix_addr_len)); > + ASSERT_EQ(0, bind(self->dgram_server, > + &self->dgram_address.unix_addr, > + (self->dgram_address).unix_addr_len)); > + ASSERT_EQ(0, listen(self->server, backlog)); > + > + /* signal to child that parent is listening */ > + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); ditto > + } > + > + ASSERT_EQ(child, waitpid(child, &status, 0)); > + > + if (WIFSIGNALED(status) || !WIFEXITED(status) || > + WEXITSTATUS(status) != EXIT_SUCCESS) > + _metadata->exit_code = KSFT_FAIL; > +} > + > +/* > + * ################### > + * # ####### # P3 -> P2 : allow > + * # P1----# P2 # # P3 -> P1 : deny > + * # # | # # > + * # # P3 # # > + * # ####### # > + * ################### > + */ > +/* clang-format off */ > +FIXTURE_VARIANT_ADD(optional_scoping, all_scoped) { > + .domain_all = SCOPE_SANDBOX, > + .domain_parent = NO_SANDBOX, > + .domain_children = SCOPE_SANDBOX, > + .domain_child = NO_SANDBOX, > + .domain_grand_child = NO_SANDBOX, > + .type = SOCK_DGRAM, There are spaces instead of tabs here. > + /* clang-format on */ > +}; > +/* > + * ###### P3 -> P2 : deny > + * # P1 #----P2 P3 -> P1 : deny > + * ###### | > + * | > + * ###### > + * # P3 # > + * ###### > + */ > +/* clang-format off */ > +FIXTURE_VARIANT_ADD(optional_scoping, deny_with_self_and_parents_domain) { > + .domain_all = NO_SANDBOX, > + .domain_parent = SCOPE_SANDBOX, > + .domain_children = NO_SANDBOX, > + .domain_child = NO_SANDBOX, > + .domain_grand_child = SCOPE_SANDBOX, > + .type = SOCK_STREAM, ditto > + /* clang-format on */ > +}; > + > +/* Extra space > + * Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent, child > + * and grand child processes when they can have scoped or non-scoped > + * domains. > + **/ Extra '*' > +TEST_F(optional_scoping, unix_scoping) > +{ > + pid_t child; > + int status; > + bool can_connect_to_parent, can_connect_to_child; > + int pipe_parent[2]; > + > + can_connect_to_child = > + (variant->domain_grand_child == SCOPE_SANDBOX) ? false : true; No need for `? false : true`, just use comparison result: can_connect_to_child = (variant->domain_grand_child != SCOPE_SANDBOX); > + > + can_connect_to_parent = (!can_connect_to_child || > + variant->domain_children == SCOPE_SANDBOX) ? > + false : > + true; ditto > + > + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC)); > + > + if (variant->domain_all == OTHER_SANDBOX) > + create_fs_domain(_metadata); > + else if (variant->domain_all == SCOPE_SANDBOX) > + create_unix_domain(_metadata); > + > + child = fork(); > + ASSERT_LE(0, child); > + if (child == 0) { > + int pipe_child[2]; > + > + ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC)); > + pid_t grand_child; > + > + if (variant->domain_children == OTHER_SANDBOX) > + create_fs_domain(_metadata); > + else if (variant->domain_children == SCOPE_SANDBOX) > + create_unix_domain(_metadata); > + > + grand_child = fork(); > + ASSERT_LE(0, grand_child); > + if (grand_child == 0) { > + ASSERT_EQ(0, close(pipe_parent[1])); > + ASSERT_EQ(0, close(pipe_child[1])); > + > + char buf1, buf2; > + int err; > + > + if (variant->domain_grand_child == OTHER_SANDBOX) > + create_fs_domain(_metadata); > + else if (variant->domain_grand_child == SCOPE_SANDBOX) > + create_unix_domain(_metadata); > + > + self->client = socket(AF_UNIX, variant->type, 0); ditto > + ASSERT_NE(-1, self->client); > + > + ASSERT_EQ(1, read(pipe_child[0], &buf2, 1)); > + err = connect(self->client, > + &self->child_address.unix_addr, > + (self->child_address).unix_addr_len); > + if (can_connect_to_child) { > + EXPECT_EQ(0, err); > + } else { > + EXPECT_EQ(-1, err); > + EXPECT_EQ(EPERM, errno); > + } > + > + if (variant->type == SOCK_STREAM) { > + EXPECT_EQ(0, close(self->client)); > + self->client = > + socket(AF_UNIX, variant->type, 0); > + ASSERT_NE(-1, self->client); > + } > + > + ASSERT_EQ(1, read(pipe_parent[0], &buf1, 1)); > + err = connect(self->client, > + &self->parent_address.unix_addr, > + (self->parent_address).unix_addr_len); > + if (can_connect_to_parent) { > + EXPECT_EQ(0, err); > + } else { > + EXPECT_EQ(-1, err); > + EXPECT_EQ(EPERM, errno); > + } > + EXPECT_EQ(0, close(self->client)); > + > + _exit(_metadata->exit_code); > + return; > + } > + > + ASSERT_EQ(0, close(pipe_child[0])); > + if (variant->domain_child == OTHER_SANDBOX) > + create_fs_domain(_metadata); > + else if (variant->domain_child == SCOPE_SANDBOX) > + create_unix_domain(_metadata); > + > + self->child_server = socket(AF_UNIX, variant->type, 0); > + ASSERT_NE(-1, self->child_server); > + ASSERT_EQ(0, bind(self->child_server, > + &self->child_address.unix_addr, > + (self->child_address).unix_addr_len)); > + if (variant->type == SOCK_STREAM) > + ASSERT_EQ(0, listen(self->child_server, backlog)); > + > + ASSERT_EQ(1, write(pipe_child[1], ".", 1)); > + ASSERT_EQ(grand_child, waitpid(grand_child, &status, 0)); > + return; > + } > + ASSERT_EQ(0, close(pipe_parent[0])); > + > + if (variant->domain_parent == OTHER_SANDBOX) > + create_fs_domain(_metadata); > + else if (variant->domain_parent == SCOPE_SANDBOX) > + create_unix_domain(_metadata); > + > + self->parent_server = socket(AF_UNIX, variant->type, 0); > + ASSERT_NE(-1, self->parent_server); > + ASSERT_EQ(0, bind(self->parent_server, &self->parent_address.unix_addr, > + (self->parent_address).unix_addr_len)); > + > + if (variant->type == SOCK_STREAM) > + ASSERT_EQ(0, listen(self->parent_server, backlog)); > + > + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); > + ASSERT_EQ(child, waitpid(child, &status, 0)); > + if (WIFSIGNALED(status) || !WIFEXITED(status) || > + WEXITSTATUS(status) != EXIT_SUCCESS) > + _metadata->exit_code = KSFT_FAIL; > +} > + > +/* > + * Since the special case of scoping only happens when the connecting socket > + * is scoped, the client's domain is true for all the following test cases. > + */ > +/* clang-format off */ > +FIXTURE(unix_sock_special_cases) { > + int server_socket, client; > + int stream_server, stream_client; Same here, these variables should be local. > + struct service_fixture address, transit_address; > +}; > + > +/* clang-format on */ > +FIXTURE_VARIANT(unix_sock_special_cases) > +{ > + const bool domain_server; > + const bool domain_server_socket; > + const int type; > +}; > + > +/* clang-format off */ > +FIXTURE_VARIANT_ADD(unix_sock_special_cases, allow_dgram_server_sock_domain) { > + /* clang-format on */ > + .domain_server = false, > + .domain_server_socket = true, > + .type = SOCK_DGRAM, > +}; > + > +/* clang-format off */ > +FIXTURE_VARIANT_ADD(unix_sock_special_cases, deny_dgram_server_domain) { > + /* clang-format on */ > + .domain_server = true, > + .domain_server_socket = false, > + .type = SOCK_DGRAM, > +}; > + > +/* clang-format off */ > +FIXTURE_VARIANT_ADD(unix_sock_special_cases, allow_stream_server_sock_domain) { > + /* clang-format on */ > + .domain_server = false, > + .domain_server_socket = true, > + .type = SOCK_STREAM, > +}; > + > +/* clang-format off */ > +FIXTURE_VARIANT_ADD(unix_sock_special_cases, deny_stream_server_domain) { > + /* clang-format on */ > + .domain_server = true, > + .domain_server_socket = false, > + .type = SOCK_STREAM, > +}; > + > +FIXTURE_SETUP(unix_sock_special_cases) > +{ > + memset(&self->transit_address, 0, sizeof(self->transit_address)); > + memset(&self->address, 0, sizeof(self->address)); > + set_unix_address(&self->transit_address, 0); > + set_unix_address(&self->address, 1); > +} > + > +FIXTURE_TEARDOWN(unix_sock_special_cases) > +{ > + close(self->client); > + close(self->server_socket); > + close(self->stream_server); > + close(self->stream_client); These EXPECT_EQ(0, close()) calls should be local too. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 2/5] selftests/Landlock: Abstract unix socket restriction tests 2024-08-16 21:23 ` Mickaël Salaün @ 2024-08-16 23:08 ` Tahera Fahimi 2024-08-19 15:38 ` Mickaël Salaün 0 siblings, 1 reply; 21+ messages in thread From: Tahera Fahimi @ 2024-08-16 23:08 UTC (permalink / raw) To: Mickaël Salaün Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Fri, Aug 16, 2024 at 11:23:05PM +0200, Mickaël Salaün wrote: > Please make sure all subject's prefixes have "landlock", not "Landlock" > for consistency with current commits. > > On Wed, Aug 14, 2024 at 12:22:20AM -0600, Tahera Fahimi wrote: > > The patch introduces Landlock ABI version 6 and has three types of tests > > that examines different scenarios for abstract unix socket connection: > > 1) unix_socket: base tests of the abstract socket scoping mechanism for a > > landlocked process, same as the ptrace test. > > 2) optional_scoping: generates three processes with different domains and > > tests if a process with a non-scoped domain can connect to other > > processes. > > 3) unix_sock_special_cases: since the socket's creator credentials are used > > for scoping sockets, this test examines the cases where the socket's > > credentials are different from the process using it. > > > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > > --- > > Changes in versions: > > v9: > > - Move pathname_address_sockets to a different patch. > > - Extend optional_scoping test scenarios. > > - Removing hardcoded numbers and using "backlog" instead. > > You may have missed some parts of my previous review (e.g. local > variables). Hi Mickaël, Thanks for the feedback. I actually did not apply the local variable for the reason below. > > V8: > > - Move tests to scoped_abstract_unix_test.c file. > > - To avoid potential conflicts among Unix socket names in different tests, > > set_unix_address is added to common.h to set different sun_path for Unix sockets. > > - protocol_variant and service_fixture structures are also moved to common.h > > - Adding pathname_address_sockets to cover all types of address formats > > for unix sockets, and moving remove_path() to common.h to reuse in this test. > > V7: > > - Introducing landlock ABI version 6. > > - Adding some edge test cases to optional_scoping test. > > - Using `enum` for different domains in optional_scoping tests. > > - Extend unix_sock_special_cases test cases for connected(SOCK_STREAM) sockets. > > - Modifying inline comments. > > V6: > > - Introducing optional_scoping test which ensures a sandboxed process with a > > non-scoped domain can still connect to another abstract unix socket(either > > sandboxed or non-sandboxed). > > - Introducing unix_sock_special_cases test which tests examines scenarios where > > the connecting sockets have different domain than the process using them. > > V4: > > - Introducing unix_socket to evaluate the basic scoping mechanism for abstract > > unix sockets. > > --- > > tools/testing/selftests/landlock/base_test.c | 2 +- > > tools/testing/selftests/landlock/common.h | 38 + > > tools/testing/selftests/landlock/net_test.c | 31 +- > > .../landlock/scoped_abstract_unix_test.c | 942 ++++++++++++++++++ > > 4 files changed, 982 insertions(+), 31 deletions(-) > > create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c > > > > > +static void create_unix_domain(struct __test_metadata *const _metadata) > > +{ > > + int ruleset_fd; > > + const struct landlock_ruleset_attr ruleset_attr = { > > + .scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET, > > + }; > > + > > + ruleset_fd = > > + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); > > + EXPECT_LE(0, ruleset_fd) > > + { > > + TH_LOG("Failed to create a ruleset: %s", strerror(errno)); > > + } > > + enforce_ruleset(_metadata, ruleset_fd); > > + EXPECT_EQ(0, close(ruleset_fd)); > > +} > > + > > +/* clang-format off */ > > It should not be required to add this clang-format comment here nor in > most places, except variant declarations (that might change if we remove > variables though). > > > +FIXTURE(unix_socket) > > +{ > > + struct service_fixture stream_address, dgram_address; > > + int server, client, dgram_server, dgram_client; > > These variables don't need to be in the fixture but they should be local > instead (and scoped to the if/else condition where they are used). > > > +}; > > + > > +/* clang-format on */ > > +FIXTURE_VARIANT(unix_socket) > > +{ > > + bool domain_both; > > + bool domain_parent; > > + bool domain_child; > > + bool connect_to_parent; > > +}; > > > +/* > > + * Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent and child, > > + * when they have scoped domain or no domain. > > + */ > > +TEST_F(unix_socket, abstract_unix_socket) > > +{ > > + int status; > > + pid_t child; > > + bool can_connect_to_parent, can_connect_to_child; > > + int err, err_dgram; > > + int pipe_child[2], pipe_parent[2]; > > + char buf_parent; > > + > > + /* > > + * can_connect_to_child is true if a parent process can connect to its > > + * child process. The parent process is not isolated from the child > > + * with a dedicated Landlock domain. > > + */ > > + can_connect_to_child = !variant->domain_parent; > > + /* > > + * can_connect_to_parent is true if a child process can connect to its > > + * parent process. This depends on the child process is not isolated from > > + * the parent with a dedicated Landlock domain. > > + */ > > + can_connect_to_parent = !variant->domain_child; > > + > > + ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC)); > > + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC)); > > + if (variant->domain_both) { > > + create_unix_domain(_metadata); > > + if (!__test_passed(_metadata)) > > + return; > > + } > > + > > + child = fork(); > > + ASSERT_LE(0, child); > > + if (child == 0) { > > + char buf_child; > > + > > + ASSERT_EQ(0, close(pipe_parent[1])); > > + ASSERT_EQ(0, close(pipe_child[0])); > > + if (variant->domain_child) > > + create_unix_domain(_metadata); > > + > > + /* Waits for the parent to be in a domain, if any. */ > > + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); > > + > > + if (variant->connect_to_parent) { > > + self->client = socket(AF_UNIX, SOCK_STREAM, 0); > > int stream_client; > > stream_client = socket(AF_UNIX, SOCK_STREAM, 0); > > ditto for dgram_client > > > + self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0); > > + > > + ASSERT_NE(-1, self->client); > > + ASSERT_NE(-1, self->dgram_client); > > + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); > > + > > + err = connect(self->client, > > + &self->stream_address.unix_addr, > > + (self->stream_address).unix_addr_len); > > + err_dgram = > > + connect(self->dgram_client, > > + &self->dgram_address.unix_addr, > > + (self->dgram_address).unix_addr_len); > > + > > + if (can_connect_to_parent) { > > + EXPECT_EQ(0, err); > > + EXPECT_EQ(0, err_dgram); > > + } else { > > + EXPECT_EQ(-1, err); > > + EXPECT_EQ(-1, err_dgram); > > + EXPECT_EQ(EPERM, errno); > > + } > > EXPECT_EQ(0, close(stream_client)); > EXPECT_EQ(0, close(dgram_client)); > > > + } else { > > + self->server = socket(AF_UNIX, SOCK_STREAM, 0); > > int stream_server; > > server = socket(AF_UNIX, SOCK_STREAM, 0); > > ditto for dgram_server > > > + self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0); > > + ASSERT_NE(-1, self->server); > > + ASSERT_NE(-1, self->dgram_server); > > + > > + ASSERT_EQ(0, > > + bind(self->server, > > + &self->stream_address.unix_addr, > > + (self->stream_address).unix_addr_len)); > > + ASSERT_EQ(0, bind(self->dgram_server, > > + &self->dgram_address.unix_addr, > > + (self->dgram_address).unix_addr_len)); > > + ASSERT_EQ(0, listen(self->server, backlog)); > > + > > + /* signal to parent that child is listening */ > > + ASSERT_EQ(1, write(pipe_child[1], ".", 1)); > > + /* wait to connect */ > > + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); > > ditto > > > + } > > + _exit(_metadata->exit_code); > > + return; > > + } > > + > > + ASSERT_EQ(0, close(pipe_child[1])); > > + ASSERT_EQ(0, close(pipe_parent[0])); > > + > > + if (variant->domain_parent) > > + create_unix_domain(_metadata); > > + > > + /* Signals that the parent is in a domain, if any. */ > > + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); > > + > > + if (!variant->connect_to_parent) { > > + self->client = socket(AF_UNIX, SOCK_STREAM, 0); > > + self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0); > > ditto > > > + > > + ASSERT_NE(-1, self->client); > > + ASSERT_NE(-1, self->dgram_client); > > + > > + /* Waits for the child to listen */ > > + ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1)); > > + err = connect(self->client, &self->stream_address.unix_addr, > > + (self->stream_address).unix_addr_len); > > + err_dgram = connect(self->dgram_client, > > + &self->dgram_address.unix_addr, > > + (self->dgram_address).unix_addr_len); > > + > > + if (can_connect_to_child) { > > + EXPECT_EQ(0, err); > > + EXPECT_EQ(0, err_dgram); > > + } else { > > + EXPECT_EQ(-1, err); > > + EXPECT_EQ(-1, err_dgram); > > + EXPECT_EQ(EPERM, errno); > > + } > > + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); > > ditto > > > + } else { > > + self->server = socket(AF_UNIX, SOCK_STREAM, 0); > > + self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0); > > ditto > > > + ASSERT_NE(-1, self->server); > > + ASSERT_NE(-1, self->dgram_server); > > + ASSERT_EQ(0, bind(self->server, &self->stream_address.unix_addr, > > + (self->stream_address).unix_addr_len)); > > + ASSERT_EQ(0, bind(self->dgram_server, > > + &self->dgram_address.unix_addr, > > + (self->dgram_address).unix_addr_len)); > > + ASSERT_EQ(0, listen(self->server, backlog)); > > + > > + /* signal to child that parent is listening */ > > + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); > > ditto I think if I define "server" and "dgram_server" as local variables, then in here, we should ensure that the clients connections are finished and then close the server sockets. The client can write on the pipe after the connection test is finished and then servers can close the sockets, but the current version is much easier. Simply when the test is finished, the FIXTURE_TEARDOWN closes all the sockets. What do you think about this? > > + } > > + > > + ASSERT_EQ(child, waitpid(child, &status, 0)); > > + > > + if (WIFSIGNALED(status) || !WIFEXITED(status) || > > + WEXITSTATUS(status) != EXIT_SUCCESS) > > + _metadata->exit_code = KSFT_FAIL; > > +} > > + > > > +/* > > + * ################### > > + * # ####### # P3 -> P2 : allow > > + * # P1----# P2 # # P3 -> P1 : deny > > + * # # | # # > > + * # # P3 # # > > + * # ####### # > > + * ################### > > + */ > > +/* clang-format off */ > > +FIXTURE_VARIANT_ADD(optional_scoping, all_scoped) { > > + .domain_all = SCOPE_SANDBOX, > > + .domain_parent = NO_SANDBOX, > > + .domain_children = SCOPE_SANDBOX, > > + .domain_child = NO_SANDBOX, > > + .domain_grand_child = NO_SANDBOX, > > + .type = SOCK_DGRAM, > > There are spaces instead of tabs here. > > > + /* clang-format on */ > > +}; > > > +/* > > + * ###### P3 -> P2 : deny > > + * # P1 #----P2 P3 -> P1 : deny > > + * ###### | > > + * | > > + * ###### > > + * # P3 # > > + * ###### > > + */ > > +/* clang-format off */ > > +FIXTURE_VARIANT_ADD(optional_scoping, deny_with_self_and_parents_domain) { > > + .domain_all = NO_SANDBOX, > > + .domain_parent = SCOPE_SANDBOX, > > + .domain_children = NO_SANDBOX, > > + .domain_child = NO_SANDBOX, > > + .domain_grand_child = SCOPE_SANDBOX, > > + .type = SOCK_STREAM, > > ditto > > > + /* clang-format on */ > > +}; > > + > > +/* > > Extra space > > > + * Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent, child > > + * and grand child processes when they can have scoped or non-scoped > > + * domains. > > + **/ > > Extra '*' > > > +TEST_F(optional_scoping, unix_scoping) > > +{ > > + pid_t child; > > + int status; > > + bool can_connect_to_parent, can_connect_to_child; > > + int pipe_parent[2]; > > + > > + can_connect_to_child = > > + (variant->domain_grand_child == SCOPE_SANDBOX) ? false : true; > > No need for `? false : true`, just use comparison result: > can_connect_to_child = (variant->domain_grand_child != SCOPE_SANDBOX); sorry,my bad. > > > + > > + can_connect_to_parent = (!can_connect_to_child || > > + variant->domain_children == SCOPE_SANDBOX) ? > > + false : > > + true; > > ditto > > > + > > + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC)); > > + > > + if (variant->domain_all == OTHER_SANDBOX) > > + create_fs_domain(_metadata); > > + else if (variant->domain_all == SCOPE_SANDBOX) > > + create_unix_domain(_metadata); > > + > > + child = fork(); > > + ASSERT_LE(0, child); > > + if (child == 0) { > > + int pipe_child[2]; > > + > > + ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC)); > > + pid_t grand_child; > > + > > + if (variant->domain_children == OTHER_SANDBOX) > > + create_fs_domain(_metadata); > > + else if (variant->domain_children == SCOPE_SANDBOX) > > + create_unix_domain(_metadata); > > + > > + grand_child = fork(); > > + ASSERT_LE(0, grand_child); > > + if (grand_child == 0) { > > + ASSERT_EQ(0, close(pipe_parent[1])); > > + ASSERT_EQ(0, close(pipe_child[1])); > > + > > + char buf1, buf2; > > + int err; > > + > > + if (variant->domain_grand_child == OTHER_SANDBOX) > > + create_fs_domain(_metadata); > > + else if (variant->domain_grand_child == SCOPE_SANDBOX) > > + create_unix_domain(_metadata); > > + > > + self->client = socket(AF_UNIX, variant->type, 0); > > ditto > > > + ASSERT_NE(-1, self->client); > > + > > + ASSERT_EQ(1, read(pipe_child[0], &buf2, 1)); > > + err = connect(self->client, > > + &self->child_address.unix_addr, > > + (self->child_address).unix_addr_len); > > + if (can_connect_to_child) { > > + EXPECT_EQ(0, err); > > + } else { > > + EXPECT_EQ(-1, err); > > + EXPECT_EQ(EPERM, errno); > > + } > > + > > + if (variant->type == SOCK_STREAM) { > > + EXPECT_EQ(0, close(self->client)); > > + self->client = > > + socket(AF_UNIX, variant->type, 0); > > + ASSERT_NE(-1, self->client); > > + } > > + > > + ASSERT_EQ(1, read(pipe_parent[0], &buf1, 1)); > > + err = connect(self->client, > > + &self->parent_address.unix_addr, > > + (self->parent_address).unix_addr_len); > > + if (can_connect_to_parent) { > > + EXPECT_EQ(0, err); > > + } else { > > + EXPECT_EQ(-1, err); > > + EXPECT_EQ(EPERM, errno); > > + } > > + EXPECT_EQ(0, close(self->client)); > > + > > + _exit(_metadata->exit_code); > > + return; > > + } > > + > > + ASSERT_EQ(0, close(pipe_child[0])); > > + if (variant->domain_child == OTHER_SANDBOX) > > + create_fs_domain(_metadata); > > + else if (variant->domain_child == SCOPE_SANDBOX) > > + create_unix_domain(_metadata); > > + > > + self->child_server = socket(AF_UNIX, variant->type, 0); > > + ASSERT_NE(-1, self->child_server); > > + ASSERT_EQ(0, bind(self->child_server, > > + &self->child_address.unix_addr, > > + (self->child_address).unix_addr_len)); > > + if (variant->type == SOCK_STREAM) > > + ASSERT_EQ(0, listen(self->child_server, backlog)); > > + > > + ASSERT_EQ(1, write(pipe_child[1], ".", 1)); > > + ASSERT_EQ(grand_child, waitpid(grand_child, &status, 0)); > > + return; > > + } > > + ASSERT_EQ(0, close(pipe_parent[0])); > > + > > + if (variant->domain_parent == OTHER_SANDBOX) > > + create_fs_domain(_metadata); > > + else if (variant->domain_parent == SCOPE_SANDBOX) > > + create_unix_domain(_metadata); > > + > > + self->parent_server = socket(AF_UNIX, variant->type, 0); > > + ASSERT_NE(-1, self->parent_server); > > + ASSERT_EQ(0, bind(self->parent_server, &self->parent_address.unix_addr, > > + (self->parent_address).unix_addr_len)); > > + > > + if (variant->type == SOCK_STREAM) > > + ASSERT_EQ(0, listen(self->parent_server, backlog)); > > + > > + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); > > + ASSERT_EQ(child, waitpid(child, &status, 0)); > > + if (WIFSIGNALED(status) || !WIFEXITED(status) || > > + WEXITSTATUS(status) != EXIT_SUCCESS) > > + _metadata->exit_code = KSFT_FAIL; > > +} > > + > > +/* > > + * Since the special case of scoping only happens when the connecting socket > > + * is scoped, the client's domain is true for all the following test cases. > > + */ > > +/* clang-format off */ > > +FIXTURE(unix_sock_special_cases) { > > + int server_socket, client; > > + int stream_server, stream_client; > > Same here, these variables should be local. > > > + struct service_fixture address, transit_address; > > +}; > > + > > +/* clang-format on */ > > +FIXTURE_VARIANT(unix_sock_special_cases) > > +{ > > + const bool domain_server; > > + const bool domain_server_socket; > > + const int type; > > +}; > > + > > +/* clang-format off */ > > +FIXTURE_VARIANT_ADD(unix_sock_special_cases, allow_dgram_server_sock_domain) { > > + /* clang-format on */ > > + .domain_server = false, > > + .domain_server_socket = true, > > + .type = SOCK_DGRAM, > > +}; > > + > > +/* clang-format off */ > > +FIXTURE_VARIANT_ADD(unix_sock_special_cases, deny_dgram_server_domain) { > > + /* clang-format on */ > > + .domain_server = true, > > + .domain_server_socket = false, > > + .type = SOCK_DGRAM, > > +}; > > + > > +/* clang-format off */ > > +FIXTURE_VARIANT_ADD(unix_sock_special_cases, allow_stream_server_sock_domain) { > > + /* clang-format on */ > > + .domain_server = false, > > + .domain_server_socket = true, > > + .type = SOCK_STREAM, > > +}; > > + > > +/* clang-format off */ > > +FIXTURE_VARIANT_ADD(unix_sock_special_cases, deny_stream_server_domain) { > > + /* clang-format on */ > > + .domain_server = true, > > + .domain_server_socket = false, > > + .type = SOCK_STREAM, > > +}; > > + > > +FIXTURE_SETUP(unix_sock_special_cases) > > +{ > > + memset(&self->transit_address, 0, sizeof(self->transit_address)); > > + memset(&self->address, 0, sizeof(self->address)); > > + set_unix_address(&self->transit_address, 0); > > + set_unix_address(&self->address, 1); > > +} > > + > > +FIXTURE_TEARDOWN(unix_sock_special_cases) > > +{ > > + close(self->client); > > + close(self->server_socket); > > + close(self->stream_server); > > + close(self->stream_client); > > These EXPECT_EQ(0, close()) calls should be local too. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 2/5] selftests/Landlock: Abstract unix socket restriction tests 2024-08-16 23:08 ` Tahera Fahimi @ 2024-08-19 15:38 ` Mickaël Salaün 0 siblings, 0 replies; 21+ messages in thread From: Mickaël Salaün @ 2024-08-19 15:38 UTC (permalink / raw) To: Tahera Fahimi Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Fri, Aug 16, 2024 at 05:08:26PM -0600, Tahera Fahimi wrote: > On Fri, Aug 16, 2024 at 11:23:05PM +0200, Mickaël Salaün wrote: > > Please make sure all subject's prefixes have "landlock", not "Landlock" > > for consistency with current commits. > > > > On Wed, Aug 14, 2024 at 12:22:20AM -0600, Tahera Fahimi wrote: > > > The patch introduces Landlock ABI version 6 and has three types of tests > > > that examines different scenarios for abstract unix socket connection: > > > 1) unix_socket: base tests of the abstract socket scoping mechanism for a > > > landlocked process, same as the ptrace test. > > > 2) optional_scoping: generates three processes with different domains and > > > tests if a process with a non-scoped domain can connect to other > > > processes. > > > 3) unix_sock_special_cases: since the socket's creator credentials are used > > > for scoping sockets, this test examines the cases where the socket's > > > credentials are different from the process using it. > > > > > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > > > --- > > > Changes in versions: > > > v9: > > > - Move pathname_address_sockets to a different patch. > > > - Extend optional_scoping test scenarios. > > > - Removing hardcoded numbers and using "backlog" instead. > > > > You may have missed some parts of my previous review (e.g. local > > variables). > Hi Mickaël, > Thanks for the feedback. I actually did not apply the local variable for > the reason below. > > > V8: > > > - Move tests to scoped_abstract_unix_test.c file. > > > - To avoid potential conflicts among Unix socket names in different tests, > > > set_unix_address is added to common.h to set different sun_path for Unix sockets. > > > - protocol_variant and service_fixture structures are also moved to common.h > > > - Adding pathname_address_sockets to cover all types of address formats > > > for unix sockets, and moving remove_path() to common.h to reuse in this test. > > > V7: > > > - Introducing landlock ABI version 6. > > > - Adding some edge test cases to optional_scoping test. > > > - Using `enum` for different domains in optional_scoping tests. > > > - Extend unix_sock_special_cases test cases for connected(SOCK_STREAM) sockets. > > > - Modifying inline comments. > > > V6: > > > - Introducing optional_scoping test which ensures a sandboxed process with a > > > non-scoped domain can still connect to another abstract unix socket(either > > > sandboxed or non-sandboxed). > > > - Introducing unix_sock_special_cases test which tests examines scenarios where > > > the connecting sockets have different domain than the process using them. > > > V4: > > > - Introducing unix_socket to evaluate the basic scoping mechanism for abstract > > > unix sockets. > > > --- > > > tools/testing/selftests/landlock/base_test.c | 2 +- > > > tools/testing/selftests/landlock/common.h | 38 + > > > tools/testing/selftests/landlock/net_test.c | 31 +- > > > .../landlock/scoped_abstract_unix_test.c | 942 ++++++++++++++++++ > > > 4 files changed, 982 insertions(+), 31 deletions(-) > > > create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c > > > > > > > > +static void create_unix_domain(struct __test_metadata *const _metadata) > > > +{ > > > + int ruleset_fd; > > > + const struct landlock_ruleset_attr ruleset_attr = { > > > + .scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET, > > > + }; > > > + > > > + ruleset_fd = > > > + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); > > > + EXPECT_LE(0, ruleset_fd) > > > + { > > > + TH_LOG("Failed to create a ruleset: %s", strerror(errno)); > > > + } > > > + enforce_ruleset(_metadata, ruleset_fd); > > > + EXPECT_EQ(0, close(ruleset_fd)); > > > +} > > > + > > > +/* clang-format off */ > > > > It should not be required to add this clang-format comment here nor in > > most places, except variant declarations (that might change if we remove > > variables though). > > > > > +FIXTURE(unix_socket) > > > +{ > > > + struct service_fixture stream_address, dgram_address; > > > + int server, client, dgram_server, dgram_client; > > > > These variables don't need to be in the fixture but they should be local > > instead (and scoped to the if/else condition where they are used). > > > > > +}; > > > + > > > +/* clang-format on */ > > > +FIXTURE_VARIANT(unix_socket) > > > +{ > > > + bool domain_both; > > > + bool domain_parent; > > > + bool domain_child; > > > + bool connect_to_parent; > > > +}; > > > > > +/* > > > + * Test UNIX_STREAM_CONNECT and UNIX_MAY_SEND for parent and child, > > > + * when they have scoped domain or no domain. > > > + */ > > > +TEST_F(unix_socket, abstract_unix_socket) > > > +{ > > > + int status; > > > + pid_t child; > > > + bool can_connect_to_parent, can_connect_to_child; > > > + int err, err_dgram; > > > + int pipe_child[2], pipe_parent[2]; > > > + char buf_parent; > > > + > > > + /* > > > + * can_connect_to_child is true if a parent process can connect to its > > > + * child process. The parent process is not isolated from the child > > > + * with a dedicated Landlock domain. > > > + */ > > > + can_connect_to_child = !variant->domain_parent; > > > + /* > > > + * can_connect_to_parent is true if a child process can connect to its > > > + * parent process. This depends on the child process is not isolated from > > > + * the parent with a dedicated Landlock domain. > > > + */ > > > + can_connect_to_parent = !variant->domain_child; > > > + > > > + ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC)); > > > + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC)); > > > + if (variant->domain_both) { > > > + create_unix_domain(_metadata); > > > + if (!__test_passed(_metadata)) > > > + return; > > > + } > > > + > > > + child = fork(); > > > + ASSERT_LE(0, child); > > > + if (child == 0) { > > > + char buf_child; > > > + > > > + ASSERT_EQ(0, close(pipe_parent[1])); > > > + ASSERT_EQ(0, close(pipe_child[0])); > > > + if (variant->domain_child) > > > + create_unix_domain(_metadata); > > > + > > > + /* Waits for the parent to be in a domain, if any. */ > > > + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); > > > + > > > + if (variant->connect_to_parent) { > > > + self->client = socket(AF_UNIX, SOCK_STREAM, 0); > > > > int stream_client; > > > > stream_client = socket(AF_UNIX, SOCK_STREAM, 0); > > > > ditto for dgram_client > > > > > + self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0); > > > + > > > + ASSERT_NE(-1, self->client); > > > + ASSERT_NE(-1, self->dgram_client); > > > + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); > > > + > > > + err = connect(self->client, > > > + &self->stream_address.unix_addr, > > > + (self->stream_address).unix_addr_len); > > > + err_dgram = > > > + connect(self->dgram_client, > > > + &self->dgram_address.unix_addr, > > > + (self->dgram_address).unix_addr_len); > > > + > > > + if (can_connect_to_parent) { > > > + EXPECT_EQ(0, err); > > > + EXPECT_EQ(0, err_dgram); > > > + } else { > > > + EXPECT_EQ(-1, err); > > > + EXPECT_EQ(-1, err_dgram); > > > + EXPECT_EQ(EPERM, errno); > > > + } > > > > EXPECT_EQ(0, close(stream_client)); > > EXPECT_EQ(0, close(dgram_client)); > > > > > + } else { > > > + self->server = socket(AF_UNIX, SOCK_STREAM, 0); > > > > int stream_server; > > > > server = socket(AF_UNIX, SOCK_STREAM, 0); > > > > ditto for dgram_server > > > > > + self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0); > > > + ASSERT_NE(-1, self->server); > > > + ASSERT_NE(-1, self->dgram_server); > > > + > > > + ASSERT_EQ(0, > > > + bind(self->server, > > > + &self->stream_address.unix_addr, > > > + (self->stream_address).unix_addr_len)); > > > + ASSERT_EQ(0, bind(self->dgram_server, > > > + &self->dgram_address.unix_addr, > > > + (self->dgram_address).unix_addr_len)); > > > + ASSERT_EQ(0, listen(self->server, backlog)); > > > + > > > + /* signal to parent that child is listening */ > > > + ASSERT_EQ(1, write(pipe_child[1], ".", 1)); > > > + /* wait to connect */ > > > + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); > > > > ditto > > > > > + } > > > + _exit(_metadata->exit_code); > > > + return; > > > + } > > > + > > > + ASSERT_EQ(0, close(pipe_child[1])); > > > + ASSERT_EQ(0, close(pipe_parent[0])); > > > + > > > + if (variant->domain_parent) > > > + create_unix_domain(_metadata); > > > + > > > + /* Signals that the parent is in a domain, if any. */ > > > + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); > > > + > > > + if (!variant->connect_to_parent) { > > > + self->client = socket(AF_UNIX, SOCK_STREAM, 0); > > > + self->dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0); > > > > ditto > > > > > + > > > + ASSERT_NE(-1, self->client); > > > + ASSERT_NE(-1, self->dgram_client); > > > + > > > + /* Waits for the child to listen */ > > > + ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1)); > > > + err = connect(self->client, &self->stream_address.unix_addr, > > > + (self->stream_address).unix_addr_len); > > > + err_dgram = connect(self->dgram_client, > > > + &self->dgram_address.unix_addr, > > > + (self->dgram_address).unix_addr_len); > > > + > > > + if (can_connect_to_child) { > > > + EXPECT_EQ(0, err); > > > + EXPECT_EQ(0, err_dgram); > > > + } else { > > > + EXPECT_EQ(-1, err); > > > + EXPECT_EQ(-1, err_dgram); > > > + EXPECT_EQ(EPERM, errno); > > > + } > > > + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); > > > > ditto > > > > > + } else { > > > + self->server = socket(AF_UNIX, SOCK_STREAM, 0); > > > + self->dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0); > > > > ditto > > > > > + ASSERT_NE(-1, self->server); > > > + ASSERT_NE(-1, self->dgram_server); > > > + ASSERT_EQ(0, bind(self->server, &self->stream_address.unix_addr, > > > + (self->stream_address).unix_addr_len)); > > > + ASSERT_EQ(0, bind(self->dgram_server, > > > + &self->dgram_address.unix_addr, > > > + (self->dgram_address).unix_addr_len)); > > > + ASSERT_EQ(0, listen(self->server, backlog)); > > > + > > > + /* signal to child that parent is listening */ > > > + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); > > > > ditto > I think if I define "server" and "dgram_server" as local variables, then > in here, we should ensure that the clients connections are finished and > then close the server sockets. The client can write on the pipe after the > connection test is finished and then servers can close the sockets, but > the current version is much easier. Simply when the test is finished, > the FIXTURE_TEARDOWN closes all the sockets. What do you think about this? Right, a close() call here would not work, but calling close(stream_server) and close(dgram_server) at the end of this TEST_F() will be OK and cleaner than in the teardown. The scope of these variable should then be TEST_F() too. > > > + } > > > + > > > + ASSERT_EQ(child, waitpid(child, &status, 0)); > > > + > > > + if (WIFSIGNALED(status) || !WIFEXITED(status) || > > > + WEXITSTATUS(status) != EXIT_SUCCESS) > > > + _metadata->exit_code = KSFT_FAIL; > > > +} > > > + ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 2/5] selftests/Landlock: Abstract unix socket restriction tests 2024-08-14 6:22 ` [PATCH v9 2/5] selftests/Landlock: Abstract unix socket restriction tests Tahera Fahimi 2024-08-16 21:23 ` Mickaël Salaün @ 2024-08-19 15:42 ` Mickaël Salaün 2024-08-19 19:55 ` Tahera Fahimi 1 sibling, 1 reply; 21+ messages in thread From: Mickaël Salaün @ 2024-08-19 15:42 UTC (permalink / raw) To: Tahera Fahimi Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Wed, Aug 14, 2024 at 12:22:20AM -0600, Tahera Fahimi wrote: > The patch introduces Landlock ABI version 6 and has three types of tests "and adds three types" ? > that examines different scenarios for abstract unix socket connection: Not only connection. > 1) unix_socket: base tests of the abstract socket scoping mechanism for a > landlocked process, same as the ptrace test. > 2) optional_scoping: generates three processes with different domains and > tests if a process with a non-scoped domain can connect to other > processes. > 3) unix_sock_special_cases: since the socket's creator credentials are used "unix_sock_special_cases" seems a bit too generic and is not self-explanatory. What about "outside_socket"? > for scoping sockets, this test examines the cases where the socket's > credentials are different from the process using it. > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > --- > --- /dev/null > +++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c > @@ -0,0 +1,942 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Landlock tests - Abstract Unix Socket > + * > + * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net> > + * Copyright © 2019-2020 ANSSI You can replace these two lines with your copyright (same for the signal test file): Copyright © 2024 Tahera Fahimi <fahimitahera@gmail.com> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 2/5] selftests/Landlock: Abstract unix socket restriction tests 2024-08-19 15:42 ` Mickaël Salaün @ 2024-08-19 19:55 ` Tahera Fahimi 0 siblings, 0 replies; 21+ messages in thread From: Tahera Fahimi @ 2024-08-19 19:55 UTC (permalink / raw) To: Mickaël Salaün Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Mon, Aug 19, 2024 at 05:42:59PM +0200, Mickaël Salaün wrote: > On Wed, Aug 14, 2024 at 12:22:20AM -0600, Tahera Fahimi wrote: > > The patch introduces Landlock ABI version 6 and has three types of tests > > "and adds three types" ? > > > that examines different scenarios for abstract unix socket connection: > > Not only connection. > > > 1) unix_socket: base tests of the abstract socket scoping mechanism for a > > landlocked process, same as the ptrace test. > > 2) optional_scoping: generates three processes with different domains and > > tests if a process with a non-scoped domain can connect to other > > processes. > > 3) unix_sock_special_cases: since the socket's creator credentials are used > > "unix_sock_special_cases" seems a bit too generic and is not > self-explanatory. What about "outside_socket"? Sure, I'll change it to "outside_socket" > > for scoping sockets, this test examines the cases where the socket's > > credentials are different from the process using it. > > > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > > --- > > > --- /dev/null > > +++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c > > @@ -0,0 +1,942 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Landlock tests - Abstract Unix Socket > > + * > > + * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net> > > + * Copyright © 2019-2020 ANSSI > > You can replace these two lines with your copyright (same for the signal > test file): > Copyright © 2024 Tahera Fahimi <fahimitahera@gmail.com> Right. I copied this from ptrace_test.c and forgot to change it. Thanks :) ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 3/5] selftests/Landlock: Adding pathname Unix socket tests 2024-08-14 6:22 [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction Tahera Fahimi 2024-08-14 6:22 ` [PATCH v9 1/5] " Tahera Fahimi 2024-08-14 6:22 ` [PATCH v9 2/5] selftests/Landlock: Abstract unix socket restriction tests Tahera Fahimi @ 2024-08-14 6:22 ` Tahera Fahimi 2024-08-19 19:47 ` Mickaël Salaün 2024-08-14 6:22 ` [PATCH v9 4/5] sample/Landlock: Support abstract unix socket restriction Tahera Fahimi ` (2 subsequent siblings) 5 siblings, 1 reply; 21+ messages in thread From: Tahera Fahimi @ 2024-08-14 6:22 UTC (permalink / raw) To: outreachy Cc: mic, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev, Tahera Fahimi This patch expands abstract Unix socket restriction tests by testing pathname sockets connection with scoped domain. pathname_address_sockets ensures that Unix sockets bound to a null-terminated filesystem can still connect to a socket outside of their scoped domain. This means that even if the domain is scoped with LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET, the socket can connect to a socket outside the scoped domain. Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> --- changes in versions: v9: - Moving remove_path() back to fs_test.c, and using unlink(2) and rmdir(2) instead. - Removing hard-coded numbers and using "backlog" instead. V8: - Adding pathname_address_sockets to cover all types of address formats for unix sockets, and moving remove_path() to common.h to reuse in this test. --- .../landlock/scoped_abstract_unix_test.c | 204 ++++++++++++++++++ 1 file changed, 204 insertions(+) diff --git a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c index 232c3b767b8a..21285a7158b6 100644 --- a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c +++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c @@ -939,4 +939,208 @@ TEST_F(unix_sock_special_cases, socket_with_different_domain) WEXITSTATUS(status) != EXIT_SUCCESS) _metadata->exit_code = KSFT_FAIL; } + +static const char path1[] = TMP_DIR "/s1_variant1"; +static const char path2[] = TMP_DIR "/s2_variant1"; + +/* clang-format off */ +FIXTURE(pathname_address_sockets) { + struct service_fixture stream_address, dgram_address; +}; + +/* clang-format on */ +FIXTURE_VARIANT(pathname_address_sockets) +{ + const int domain; +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(pathname_address_sockets, pathname_socket_scoped_domain) { + /* clang-format on */ + .domain = SCOPE_SANDBOX, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(pathname_address_sockets, pathname_socket_other_domain) { + /* clang-format on */ + .domain = OTHER_SANDBOX, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(pathname_address_sockets, pathname_socket_no_domain) { + /* clang-format on */ + .domain = NO_SANDBOX, +}; + +FIXTURE_SETUP(pathname_address_sockets) +{ + /* setup abstract addresses */ + memset(&self->stream_address, 0, sizeof(self->stream_address)); + set_unix_address(&self->stream_address, 0); + + memset(&self->dgram_address, 0, sizeof(self->dgram_address)); + set_unix_address(&self->dgram_address, 0); + + disable_caps(_metadata); + umask(0077); + ASSERT_EQ(0, mkdir(TMP_DIR, 0700)); + + ASSERT_EQ(0, mknod(path1, S_IFREG | 0700, 0)) + { + TH_LOG("Failed to create file \"%s\": %s", path1, + strerror(errno)); + ASSERT_EQ(0, unlink(TMP_DIR) & rmdir(TMP_DIR)); + } + ASSERT_EQ(0, mknod(path2, S_IFREG | 0700, 0)) + { + TH_LOG("Failed to create file \"%s\": %s", path2, + strerror(errno)); + ASSERT_EQ(0, unlink(TMP_DIR) & rmdir(TMP_DIR)); + } +} + +FIXTURE_TEARDOWN(pathname_address_sockets) +{ + ASSERT_EQ(0, unlink(path1) & rmdir(path1)); + ASSERT_EQ(0, unlink(path2) & rmdir(path2)); + ASSERT_EQ(0, unlink(TMP_DIR) & rmdir(TMP_DIR)); +} + +TEST_F(pathname_address_sockets, scoped_pathname_sockets) +{ + const char *const stream_path = path1; + const char *const dgram_path = path2; + int srv_fd, srv_fd_dg; + socklen_t size, size_dg; + struct sockaddr_un srv_un, srv_un_dg; + int pipe_parent[2]; + pid_t child; + int status; + char buf_child; + int socket_fds_stream[2]; + int server, client, dgram_server, dgram_client; + int recv_data; + + ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, + socket_fds_stream)); + + srv_un.sun_family = AF_UNIX; + snprintf(srv_un.sun_path, sizeof(srv_un.sun_path), "%s", stream_path); + size = offsetof(struct sockaddr_un, sun_path) + strlen(srv_un.sun_path); + + srv_un_dg.sun_family = AF_UNIX; + snprintf(srv_un_dg.sun_path, sizeof(srv_un_dg.sun_path), "%s", + dgram_path); + size_dg = offsetof(struct sockaddr_un, sun_path) + + strlen(srv_un_dg.sun_path); + + ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC)); + + child = fork(); + ASSERT_LE(0, child); + if (child == 0) { + int cli_fd, cli_fd_dg; + int err, err_dg; + int sample = socket(AF_UNIX, SOCK_STREAM, 0); + + ASSERT_LE(0, sample); + ASSERT_EQ(0, close(pipe_parent[1])); + + /* scope the domain */ + if (variant->domain == SCOPE_SANDBOX) + create_unix_domain(_metadata); + else if (variant->domain == OTHER_SANDBOX) + create_fs_domain(_metadata); + + ASSERT_EQ(0, close(socket_fds_stream[1])); + ASSERT_EQ(0, send_fd(socket_fds_stream[0], sample)); + ASSERT_EQ(0, close(sample)); + ASSERT_EQ(0, close(socket_fds_stream[0])); + + /* wait for server to listen */ + ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1)); + + /* connect with pathname sockets */ + cli_fd = socket(AF_UNIX, SOCK_STREAM, 0); + ASSERT_LE(0, cli_fd); + ASSERT_EQ(0, connect(cli_fd, (struct sockaddr *)&srv_un, size)); + ASSERT_EQ(0, close(cli_fd)); + + cli_fd_dg = socket(AF_UNIX, SOCK_DGRAM, 0); + ASSERT_LE(0, cli_fd_dg); + ASSERT_EQ(0, connect(cli_fd_dg, (struct sockaddr *)&srv_un_dg, + size_dg)); + + ASSERT_EQ(0, close(cli_fd_dg)); + + /* check connection with abstract sockets */ + client = socket(AF_UNIX, SOCK_STREAM, 0); + dgram_client = socket(AF_UNIX, SOCK_DGRAM, 0); + + ASSERT_NE(-1, client); + ASSERT_NE(-1, dgram_client); + + err = connect(client, &self->stream_address.unix_addr, + (self->stream_address).unix_addr_len); + err_dg = connect(dgram_client, &self->dgram_address.unix_addr, + (self->dgram_address).unix_addr_len); + if (variant->domain == SCOPE_SANDBOX) { + EXPECT_EQ(-1, err); + EXPECT_EQ(-1, err_dg); + EXPECT_EQ(EPERM, errno); + } else { + EXPECT_EQ(0, err); + EXPECT_EQ(0, err_dg); + } + ASSERT_EQ(0, close(client)); + ASSERT_EQ(0, close(dgram_client)); + + _exit(_metadata->exit_code); + return; + } + + ASSERT_EQ(0, close(pipe_parent[0])); + + recv_data = recv_fd(socket_fds_stream[1]); + ASSERT_LE(0, recv_data); + ASSERT_LE(0, close(socket_fds_stream[1])); + + /* Sets up a server */ + ASSERT_EQ(0, unlink(stream_path)); + srv_fd = socket(AF_UNIX, SOCK_STREAM, 0); + ASSERT_LE(0, srv_fd); + ASSERT_EQ(0, bind(srv_fd, (struct sockaddr *)&srv_un, size)); + ASSERT_EQ(0, listen(srv_fd, 10)); + + /* set up a datagram server */ + ASSERT_EQ(0, unlink(dgram_path)); + srv_fd_dg = socket(AF_UNIX, SOCK_DGRAM, 0); + ASSERT_LE(0, srv_fd_dg); + size_dg = offsetof(struct sockaddr_un, sun_path) + + strlen(srv_un_dg.sun_path); + ASSERT_EQ(0, bind(srv_fd_dg, (struct sockaddr *)&srv_un_dg, size_dg)); + + /*set up abstract servers */ + server = socket(AF_UNIX, SOCK_STREAM, 0); + dgram_server = socket(AF_UNIX, SOCK_DGRAM, 0); + ASSERT_NE(-1, server); + ASSERT_NE(-1, dgram_server); + ASSERT_EQ(0, bind(server, &self->stream_address.unix_addr, + self->stream_address.unix_addr_len)); + ASSERT_EQ(0, bind(dgram_server, &self->dgram_address.unix_addr, + self->dgram_address.unix_addr_len)); + ASSERT_EQ(0, listen(server, backlog)); + + /* servers are listening, signal to child */ + ASSERT_EQ(1, write(pipe_parent[1], ".", 1)); + ASSERT_EQ(child, waitpid(child, &status, 0)); + ASSERT_EQ(0, close(srv_fd)); + ASSERT_EQ(0, close(srv_fd_dg)); + ASSERT_EQ(0, close(server)); + ASSERT_EQ(0, close(dgram_server)); + + if (WIFSIGNALED(status) || !WIFEXITED(status) || + WEXITSTATUS(status) != EXIT_SUCCESS) + _metadata->exit_code = KSFT_FAIL; +} TEST_HARNESS_MAIN -- 2.34.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v9 3/5] selftests/Landlock: Adding pathname Unix socket tests 2024-08-14 6:22 ` [PATCH v9 3/5] selftests/Landlock: Adding pathname Unix socket tests Tahera Fahimi @ 2024-08-19 19:47 ` Mickaël Salaün 0 siblings, 0 replies; 21+ messages in thread From: Mickaël Salaün @ 2024-08-19 19:47 UTC (permalink / raw) To: Tahera Fahimi Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Wed, Aug 14, 2024 at 12:22:21AM -0600, Tahera Fahimi wrote: > This patch expands abstract Unix socket restriction tests by > testing pathname sockets connection with scoped domain. > > pathname_address_sockets ensures that Unix sockets bound to > a null-terminated filesystem can still connect to a socket "bound to a filesystem path name" > outside of their scoped domain. This means that even if the > domain is scoped with LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET, > the socket can connect to a socket outside the scoped domain. > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > --- > changes in versions: > v9: > - Moving remove_path() back to fs_test.c, and using unlink(2) > and rmdir(2) instead. > - Removing hard-coded numbers and using "backlog" instead. > V8: > - Adding pathname_address_sockets to cover all types of address > formats for unix sockets, and moving remove_path() to > common.h to reuse in this test. > --- > .../landlock/scoped_abstract_unix_test.c | 204 ++++++++++++++++++ > 1 file changed, 204 insertions(+) > > diff --git a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c > index 232c3b767b8a..21285a7158b6 100644 > --- a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c > +++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c > @@ -939,4 +939,208 @@ TEST_F(unix_sock_special_cases, socket_with_different_domain) > WEXITSTATUS(status) != EXIT_SUCCESS) > _metadata->exit_code = KSFT_FAIL; > } > + > +static const char path1[] = TMP_DIR "/s1_variant1"; > +static const char path2[] = TMP_DIR "/s2_variant1"; > + > +/* clang-format off */ > +FIXTURE(pathname_address_sockets) { > + struct service_fixture stream_address, dgram_address; > +}; > + > +/* clang-format on */ Please minimize the use of these tags (e.g. don't include new lines) and remove them when they don't change the formatting. > + if (WIFSIGNALED(status) || !WIFEXITED(status) || > + WEXITSTATUS(status) != EXIT_SUCCESS) > + _metadata->exit_code = KSFT_FAIL; > +} Please always add a newline before TEST_HARNESS_MAIN. `check-linux.sh all` prints an error. > TEST_HARNESS_MAIN > -- > 2.34.1 > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 4/5] sample/Landlock: Support abstract unix socket restriction 2024-08-14 6:22 [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction Tahera Fahimi ` (2 preceding siblings ...) 2024-08-14 6:22 ` [PATCH v9 3/5] selftests/Landlock: Adding pathname Unix socket tests Tahera Fahimi @ 2024-08-14 6:22 ` Tahera Fahimi 2024-08-19 19:47 ` Mickaël Salaün 2024-08-14 6:22 ` [PATCH v9 5/5] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning Tahera Fahimi 2024-08-19 19:58 ` [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction Mickaël Salaün 5 siblings, 1 reply; 21+ messages in thread From: Tahera Fahimi @ 2024-08-14 6:22 UTC (permalink / raw) To: outreachy Cc: mic, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev, Tahera Fahimi A sandboxer can receive the character "a" as input from the environment variable LL_SCOPE to restrict the abstract unix sockets from connecting to a process outside its scoped domain. Example ======= Create an abstract unix socket to listen with socat(1): socat abstract-listen:mysocket - Create a sandboxed shell and pass the character "a" to LL_SCOPED: LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash If the sandboxed process tries to connect to the listening socket with command "socat - abstract-connect:mysocket", the connection will fail. Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> --- v9: - Add a restrict approach on input of LL_SCOPED, so it only allows zero or one "a" to be the input. v8: - Adding check_ruleset_scope function to parse the scope environment variable and update the landlock attribute based on the restriction provided by the user. - Adding Mickaël Salaün reviews on version 7. v7: - Adding IPC scoping to the sandbox demo by defining a new "LL_SCOPED" environment variable. "LL_SCOPED" gets value "a" to restrict abstract unix sockets. - Change LANDLOCK_ABI_LAST to 6. --- samples/landlock/sandboxer.c | 58 +++++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 4 deletions(-) diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c index e8223c3e781a..bec201eb96f7 100644 --- a/samples/landlock/sandboxer.c +++ b/samples/landlock/sandboxer.c @@ -14,6 +14,7 @@ #include <fcntl.h> #include <linux/landlock.h> #include <linux/prctl.h> +#include <linux/socket.h> #include <stddef.h> #include <stdio.h> #include <stdlib.h> @@ -22,6 +23,7 @@ #include <sys/stat.h> #include <sys/syscall.h> #include <unistd.h> +#include <stdbool.h> #ifndef landlock_create_ruleset static inline int @@ -55,6 +57,7 @@ static inline int landlock_restrict_self(const int ruleset_fd, #define ENV_FS_RW_NAME "LL_FS_RW" #define ENV_TCP_BIND_NAME "LL_TCP_BIND" #define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT" +#define ENV_SCOPED_NAME "LL_SCOPED" #define ENV_DELIMITER ":" static int parse_path(char *env_path, const char ***const path_list) @@ -184,6 +187,40 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd, return ret; } +static bool check_ruleset_scope(const char *const env_var, + struct landlock_ruleset_attr *ruleset_attr) +{ + bool abstract_scoping = false; + bool ret = true; + char *env_type_scope, *env_type_scope_next, *ipc_scoping_name; + + ruleset_attr->scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET; + env_type_scope = getenv(env_var); + /* scoping is not supported by the user */ + if (!env_type_scope) + return true; + + env_type_scope = strdup(env_type_scope); + unsetenv(env_var); + env_type_scope_next = env_type_scope; + while ((ipc_scoping_name = + strsep(&env_type_scope_next, ENV_DELIMITER))) { + if (strcmp("a", ipc_scoping_name) == 0 && !abstract_scoping) { + abstract_scoping = true; + ruleset_attr->scoped |= + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET; + } else { + fprintf(stderr, "Unsupported scoping \"%s\"\n", + ipc_scoping_name); + ret = false; + goto out_free_name; + } + } +out_free_name: + free(env_type_scope); + return ret; +} + /* clang-format off */ #define ACCESS_FS_ROUGHLY_READ ( \ @@ -208,7 +245,7 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd, /* clang-format on */ -#define LANDLOCK_ABI_LAST 5 +#define LANDLOCK_ABI_LAST 6 int main(const int argc, char *const argv[], char *const *const envp) { @@ -223,14 +260,15 @@ int main(const int argc, char *const argv[], char *const *const envp) .handled_access_fs = access_fs_rw, .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP | LANDLOCK_ACCESS_NET_CONNECT_TCP, + .scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET, }; if (argc < 2) { fprintf(stderr, - "usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s " + "usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s " "<cmd> [args]...\n\n", ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME, - ENV_TCP_CONNECT_NAME, argv[0]); + ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]); fprintf(stderr, "Execute a command in a restricted environment.\n\n"); fprintf(stderr, @@ -251,15 +289,18 @@ int main(const int argc, char *const argv[], char *const *const envp) fprintf(stderr, "* %s: list of ports allowed to connect (client).\n", ENV_TCP_CONNECT_NAME); + fprintf(stderr, "* %s: list of restrictions on IPCs.\n", + ENV_SCOPED_NAME); fprintf(stderr, "\nexample:\n" "%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" " "%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" " "%s=\"9418\" " "%s=\"80:443\" " + "%s=\"a\" " "%s bash -i\n\n", ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME, - ENV_TCP_CONNECT_NAME, argv[0]); + ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]); fprintf(stderr, "This sandboxer can use Landlock features " "up to ABI version %d.\n", @@ -327,6 +368,10 @@ int main(const int argc, char *const argv[], char *const *const envp) /* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */ ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV; + __attribute__((fallthrough)); + case 5: + /* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */ + ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET; fprintf(stderr, "Hint: You should update the running kernel " "to leverage Landlock features " @@ -358,6 +403,11 @@ int main(const int argc, char *const argv[], char *const *const envp) ~LANDLOCK_ACCESS_NET_CONNECT_TCP; } + if (abi >= 6 && !check_ruleset_scope(ENV_SCOPED_NAME, &ruleset_attr)) { + perror("Unsupported IPC scoping requested"); + return 1; + } + ruleset_fd = landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); if (ruleset_fd < 0) { -- 2.34.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v9 4/5] sample/Landlock: Support abstract unix socket restriction 2024-08-14 6:22 ` [PATCH v9 4/5] sample/Landlock: Support abstract unix socket restriction Tahera Fahimi @ 2024-08-19 19:47 ` Mickaël Salaün 0 siblings, 0 replies; 21+ messages in thread From: Mickaël Salaün @ 2024-08-19 19:47 UTC (permalink / raw) To: Tahera Fahimi Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Wed, Aug 14, 2024 at 12:22:22AM -0600, Tahera Fahimi wrote: > A sandboxer can receive the character "a" as input from the environment > variable LL_SCOPE to restrict the abstract unix sockets from connecting > to a process outside its scoped domain. > > Example > ======= > Create an abstract unix socket to listen with socat(1): > socat abstract-listen:mysocket - > Create a sandboxed shell and pass the character "a" to LL_SCOPED: > LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash > If the sandboxed process tries to connect to the listening socket > with command "socat - abstract-connect:mysocket", the connection > will fail. > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > --- > @@ -358,6 +403,11 @@ int main(const int argc, char *const argv[], char *const *const envp) > ~LANDLOCK_ACCESS_NET_CONNECT_TCP; > } > > + if (abi >= 6 && !check_ruleset_scope(ENV_SCOPED_NAME, &ruleset_attr)) { > + perror("Unsupported IPC scoping requested"); If LL_SCOPE="", the sandboxer prints: "Unsupported IPC scoping requested: Success" > + return 1; > + } > + > ruleset_fd = > landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); > if (ruleset_fd < 0) { > -- > 2.34.1 > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 5/5] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning 2024-08-14 6:22 [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction Tahera Fahimi ` (3 preceding siblings ...) 2024-08-14 6:22 ` [PATCH v9 4/5] sample/Landlock: Support abstract unix socket restriction Tahera Fahimi @ 2024-08-14 6:22 ` Tahera Fahimi 2024-08-19 19:49 ` Mickaël Salaün 2024-08-19 19:58 ` [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction Mickaël Salaün 5 siblings, 1 reply; 21+ messages in thread From: Tahera Fahimi @ 2024-08-14 6:22 UTC (permalink / raw) To: outreachy Cc: mic, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev, Tahera Fahimi Introducing LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET as an IPC scoping mechanism in Landlock ABI version 6, and updating ruleset_attr, Landlock ABI version, and access rights code blocks based on that. Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> --- v8: - Improving documentation by specifying differences between scoped and non-scoped domains. - Adding review notes of version 7. - Update date v7: - Add "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" explanation to IPC scoping section and updating ABI to version 6. - Adding "scoped" attribute to the Access rights section. - In current limitation, unnamed sockets are specified as sockets that are not restricted. - Update date --- Documentation/userspace-api/landlock.rst | 33 ++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst index 07b63aec56fa..0582f93bd952 100644 --- a/Documentation/userspace-api/landlock.rst +++ b/Documentation/userspace-api/landlock.rst @@ -8,7 +8,7 @@ Landlock: unprivileged access control ===================================== :Author: Mickaël Salaün -:Date: April 2024 +:Date: July 2024 The goal of Landlock is to enable to restrict ambient rights (e.g. global filesystem or network access) for a set of processes. Because Landlock @@ -81,6 +81,8 @@ to be explicit about the denied-by-default access rights. .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP | LANDLOCK_ACCESS_NET_CONNECT_TCP, + .scoped = + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET, }; Because we may not know on which kernel version an application will be @@ -119,6 +121,9 @@ version, and only use the available subset of access rights: case 4: /* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */ ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV; + case 5: + /* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */ + ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET; } This enables to create an inclusive ruleset that will contain our rules. @@ -306,6 +311,23 @@ To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target process, a sandboxed process should have a subset of the target process rules, which means the tracee must be in a sub-domain of the tracer. +IPC Scoping +----------- + +Similar to the implicit `Ptrace restrictions`_, we may want to further restrict +interactions between sandboxes. Each Landlock domain can be explicitly scoped +for a set of actions by specifying it on a ruleset. For example, if a sandboxed +process should not be able to :manpage:`connect(2)` to a non-sandboxed process +through abstract :manpage:`unix(7)` sockets, we can specify such restriction +with ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``. + +A sandboxed process can connect to a non-sandboxed process when its domain is +not scoped. If a process's domain is scoped, it can only connect to sockets +created by processes in the same scoped domain. + +IPC scoping does not support Landlock rules, so if a domain is scoped, no rules +can be added to allow accessing to a resource outside of the scoped domain. + Truncating files ---------------- @@ -404,7 +426,7 @@ Access rights ------------- .. kernel-doc:: include/uapi/linux/landlock.h - :identifiers: fs_access net_access + :identifiers: fs_access net_access scope Creating a new ruleset ---------------------- @@ -541,6 +563,13 @@ earlier ABI. Starting with the Landlock ABI version 5, it is possible to restrict the use of :manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right. +Abstract UNIX sockets Restriction (ABI < 6) +-------------------------------------------- + +With ABI version 6, it is possible to restrict connection to an abstract Unix socket +through ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``, thanks to the ``scoped`` ruleset +attribute. + .. _kernel_support: Kernel support -- 2.34.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v9 5/5] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning 2024-08-14 6:22 ` [PATCH v9 5/5] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning Tahera Fahimi @ 2024-08-19 19:49 ` Mickaël Salaün 0 siblings, 0 replies; 21+ messages in thread From: Mickaël Salaün @ 2024-08-19 19:49 UTC (permalink / raw) To: Tahera Fahimi Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Wed, Aug 14, 2024 at 12:22:23AM -0600, Tahera Fahimi wrote: > Introducing LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET as an IPC scoping > mechanism in Landlock ABI version 6, and updating ruleset_attr, > Landlock ABI version, and access rights code blocks based on that. > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > --- > v8: > - Improving documentation by specifying differences between scoped and > non-scoped domains. > - Adding review notes of version 7. > - Update date > v7: > - Add "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" explanation to IPC scoping > section and updating ABI to version 6. > - Adding "scoped" attribute to the Access rights section. > - In current limitation, unnamed sockets are specified as sockets that > are not restricted. > - Update date > --- > Documentation/userspace-api/landlock.rst | 33 ++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst > index 07b63aec56fa..0582f93bd952 100644 > --- a/Documentation/userspace-api/landlock.rst > +++ b/Documentation/userspace-api/landlock.rst > @@ -8,7 +8,7 @@ Landlock: unprivileged access control > ===================================== > > :Author: Mickaël Salaün > -:Date: April 2024 > +:Date: July 2024 "August" Please rebase the two patch series on v6.11-rc4 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction 2024-08-14 6:22 [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction Tahera Fahimi ` (4 preceding siblings ...) 2024-08-14 6:22 ` [PATCH v9 5/5] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning Tahera Fahimi @ 2024-08-19 19:58 ` Mickaël Salaün 2024-08-19 20:16 ` Tahera Fahimi 5 siblings, 1 reply; 21+ messages in thread From: Mickaël Salaün @ 2024-08-19 19:58 UTC (permalink / raw) To: Tahera Fahimi Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev There are still some issues (mainly with tests) but overall the kernel part looks good! I pushed this patch series to the -next branch. I'll update with the next versions of this series. I'll do the same with the next signal scoping patch series once the lifetime issue that Jann reported is fixed. On Wed, Aug 14, 2024 at 12:22:18AM -0600, Tahera Fahimi wrote: > This patch series adds scoping mechanism for abstract unix sockets. > Closes: https://github.com/landlock-lsm/linux/issues/7 > > Problem > ======= > > Abstract unix sockets are used for local inter-process communications > independent of the filesystem. Currently, a sandboxed process can > connect to a socket outside of the sandboxed environment, since Landlock > has no restriction for connecting to an abstract socket address(see more > details in [1,2]). Access to such sockets for a sandboxed process should > be scoped the same way ptrace is limited. > > [1] https://lore.kernel.org/all/20231023.ahphah4Wii4v@digikod.net/ > [2] https://lore.kernel.org/all/20231102.MaeWaepav8nu@digikod.net/ > > Solution > ======== > > To solve this issue, we extend the user space interface by adding a new > "scoped" field to Landlock ruleset attribute structure. This field can > contains different rights to restrict different functionalities. For > abstract unix sockets, we introduce > "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" field to specify that a ruleset > will deny any connection from within the sandbox domain to its parent > (i.e. any parent sandbox or non-sandbox processes). > > Example > ======= > > Starting a listening socket with socat(1): > socat abstract-listen:mysocket - > > Starting a sandboxed shell from $HOME with samples/landlock/sandboxer: > LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash > > If we try to connect to the listening socket, the connection would be > refused. > socat - abstract-connect:mysocket --> fails > > > Notes of Implementation > ======================= > > * Using the "scoped" field provides enough compatibility and flexibility > to extend the scoping mechanism for other IPCs(e.g. signals). > > * To access the domain of a socket, we use its credentials of the file's FD > which point to the credentials of the process that created the socket. > (see more details in [3]). Cases where the process using the socket has > a different domain than the process created it are covered in the > unix_sock_special_cases test. > > [3]https://lore.kernel.org/all/20240611.Pi8Iph7ootae@digikod.net/ > > Previous Versions > ================= > v8: https://lore.kernel.org/all/cover.1722570749.git.fahimitahera@gmail.com/ > v7: https://lore.kernel.org/all/cover.1721269836.git.fahimitahera@gmail.com/ > v6: https://lore.kernel.org/all/Zn32CYZiu7pY+rdI@tahera-OptiPlex-5000/ > and https://lore.kernel.org/all/Zn32KKIJrY7Zi51K@tahera-OptiPlex-5000/ > v5: https://lore.kernel.org/all/ZnSZnhGBiprI6FRk@tahera-OptiPlex-5000/ > v4: https://lore.kernel.org/all/ZnNcE3ph2SWi1qmd@tahera-OptiPlex-5000/ > v3: https://lore.kernel.org/all/ZmJJ7lZdQuQop7e5@tahera-OptiPlex-5000/ > v2: https://lore.kernel.org/all/ZgX5TRTrSDPrJFfF@tahera-OptiPlex-5000/ > v1: https://lore.kernel.org/all/ZgXN5fi6A1YQKiAQ@tahera-OptiPlex-5000/ > > Tahera Fahimi (5): > Landlock: Add abstract unix socket connect restriction > selftests/Landlock: Abstract unix socket restriction tests > selftests/Landlock: Adding pathname Unix socket tests > sample/Landlock: Support abstract unix socket restriction > Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI > versioning > > Documentation/userspace-api/landlock.rst | 33 +- > include/uapi/linux/landlock.h | 27 + > samples/landlock/sandboxer.c | 58 +- > security/landlock/limits.h | 3 + > security/landlock/ruleset.c | 7 +- > security/landlock/ruleset.h | 23 +- > security/landlock/syscalls.c | 17 +- > security/landlock/task.c | 129 ++ > tools/testing/selftests/landlock/base_test.c | 2 +- > tools/testing/selftests/landlock/common.h | 38 + > tools/testing/selftests/landlock/net_test.c | 31 +- > .../landlock/scoped_abstract_unix_test.c | 1146 +++++++++++++++++ > 12 files changed, 1469 insertions(+), 45 deletions(-) > create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c > > -- > 2.34.1 > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction 2024-08-19 19:58 ` [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction Mickaël Salaün @ 2024-08-19 20:16 ` Tahera Fahimi 0 siblings, 0 replies; 21+ messages in thread From: Tahera Fahimi @ 2024-08-19 20:16 UTC (permalink / raw) To: Mickaël Salaün Cc: outreachy, gnoack, paul, jmorris, serge, linux-security-module, linux-kernel, bjorn3_gh, jannh, netdev On Mon, Aug 19, 2024 at 09:58:27PM +0200, Mickaël Salaün wrote: > There are still some issues (mainly with tests) but overall the kernel > part looks good! I pushed this patch series to the -next branch. I'll > update with the next versions of this series. Thank you :) > I'll do the same with the next signal scoping patch series once the > lifetime issue that Jann reported is fixed. I have already applied those changes, but still need to add a test case for file_send_sigiotask() > On Wed, Aug 14, 2024 at 12:22:18AM -0600, Tahera Fahimi wrote: > > This patch series adds scoping mechanism for abstract unix sockets. > > Closes: https://github.com/landlock-lsm/linux/issues/7 > > > > Problem > > ======= > > > > Abstract unix sockets are used for local inter-process communications > > independent of the filesystem. Currently, a sandboxed process can > > connect to a socket outside of the sandboxed environment, since Landlock > > has no restriction for connecting to an abstract socket address(see more > > details in [1,2]). Access to such sockets for a sandboxed process should > > be scoped the same way ptrace is limited. > > > > [1] https://lore.kernel.org/all/20231023.ahphah4Wii4v@digikod.net/ > > [2] https://lore.kernel.org/all/20231102.MaeWaepav8nu@digikod.net/ > > > > Solution > > ======== > > > > To solve this issue, we extend the user space interface by adding a new > > "scoped" field to Landlock ruleset attribute structure. This field can > > contains different rights to restrict different functionalities. For > > abstract unix sockets, we introduce > > "LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET" field to specify that a ruleset > > will deny any connection from within the sandbox domain to its parent > > (i.e. any parent sandbox or non-sandbox processes). > > > > Example > > ======= > > > > Starting a listening socket with socat(1): > > socat abstract-listen:mysocket - > > > > Starting a sandboxed shell from $HOME with samples/landlock/sandboxer: > > LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="a" ./sandboxer /bin/bash > > > > If we try to connect to the listening socket, the connection would be > > refused. > > socat - abstract-connect:mysocket --> fails > > > > > > Notes of Implementation > > ======================= > > > > * Using the "scoped" field provides enough compatibility and flexibility > > to extend the scoping mechanism for other IPCs(e.g. signals). > > > > * To access the domain of a socket, we use its credentials of the file's FD > > which point to the credentials of the process that created the socket. > > (see more details in [3]). Cases where the process using the socket has > > a different domain than the process created it are covered in the > > unix_sock_special_cases test. > > > > [3]https://lore.kernel.org/all/20240611.Pi8Iph7ootae@digikod.net/ > > > > Previous Versions > > ================= > > v8: https://lore.kernel.org/all/cover.1722570749.git.fahimitahera@gmail.com/ > > v7: https://lore.kernel.org/all/cover.1721269836.git.fahimitahera@gmail.com/ > > v6: https://lore.kernel.org/all/Zn32CYZiu7pY+rdI@tahera-OptiPlex-5000/ > > and https://lore.kernel.org/all/Zn32KKIJrY7Zi51K@tahera-OptiPlex-5000/ > > v5: https://lore.kernel.org/all/ZnSZnhGBiprI6FRk@tahera-OptiPlex-5000/ > > v4: https://lore.kernel.org/all/ZnNcE3ph2SWi1qmd@tahera-OptiPlex-5000/ > > v3: https://lore.kernel.org/all/ZmJJ7lZdQuQop7e5@tahera-OptiPlex-5000/ > > v2: https://lore.kernel.org/all/ZgX5TRTrSDPrJFfF@tahera-OptiPlex-5000/ > > v1: https://lore.kernel.org/all/ZgXN5fi6A1YQKiAQ@tahera-OptiPlex-5000/ > > > > Tahera Fahimi (5): > > Landlock: Add abstract unix socket connect restriction > > selftests/Landlock: Abstract unix socket restriction tests > > selftests/Landlock: Adding pathname Unix socket tests > > sample/Landlock: Support abstract unix socket restriction > > Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI > > versioning > > > > Documentation/userspace-api/landlock.rst | 33 +- > > include/uapi/linux/landlock.h | 27 + > > samples/landlock/sandboxer.c | 58 +- > > security/landlock/limits.h | 3 + > > security/landlock/ruleset.c | 7 +- > > security/landlock/ruleset.h | 23 +- > > security/landlock/syscalls.c | 17 +- > > security/landlock/task.c | 129 ++ > > tools/testing/selftests/landlock/base_test.c | 2 +- > > tools/testing/selftests/landlock/common.h | 38 + > > tools/testing/selftests/landlock/net_test.c | 31 +- > > .../landlock/scoped_abstract_unix_test.c | 1146 +++++++++++++++++ > > 12 files changed, 1469 insertions(+), 45 deletions(-) > > create mode 100644 tools/testing/selftests/landlock/scoped_abstract_unix_test.c > > > > -- > > 2.34.1 > > > > ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2024-08-20 15:56 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-14 6:22 [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction Tahera Fahimi 2024-08-14 6:22 ` [PATCH v9 1/5] " Tahera Fahimi 2024-08-16 21:19 ` Mickaël Salaün 2024-08-19 15:37 ` Mickaël Salaün 2024-08-19 22:20 ` Tahera Fahimi 2024-08-20 15:56 ` Mickaël Salaün 2024-08-19 19:35 ` Mickaël Salaün 2024-08-14 6:22 ` [PATCH v9 2/5] selftests/Landlock: Abstract unix socket restriction tests Tahera Fahimi 2024-08-16 21:23 ` Mickaël Salaün 2024-08-16 23:08 ` Tahera Fahimi 2024-08-19 15:38 ` Mickaël Salaün 2024-08-19 15:42 ` Mickaël Salaün 2024-08-19 19:55 ` Tahera Fahimi 2024-08-14 6:22 ` [PATCH v9 3/5] selftests/Landlock: Adding pathname Unix socket tests Tahera Fahimi 2024-08-19 19:47 ` Mickaël Salaün 2024-08-14 6:22 ` [PATCH v9 4/5] sample/Landlock: Support abstract unix socket restriction Tahera Fahimi 2024-08-19 19:47 ` Mickaël Salaün 2024-08-14 6:22 ` [PATCH v9 5/5] Landlock: Document LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET and ABI versioning Tahera Fahimi 2024-08-19 19:49 ` Mickaël Salaün 2024-08-19 19:58 ` [PATCH v9 0/5] Landlock: Add abstract unix socket connect restriction Mickaël Salaün 2024-08-19 20:16 ` Tahera Fahimi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).