| Message ID | d7bad636c2e3609ade32fd02875fa43ec1b1d526.1721269836.git.fahimitahera@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Landlock: Abstract Unix Socket Scoping Support | expand |
On Wed, Jul 17, 2024 at 10:15:19PM -0600, Tahera Fahimi wrote: > The 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. > > This patch implement two hooks, "unix_stream_connect" and "unix_may_send" to > enforce this restriction. > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > > ------- Only "---" > v7: Thanks for the detailed changelog, it helps! > - 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 file's FD 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/outreachy/Zmi8Ydz4Z6tYtpY1@tahera-OptiPlex-5000/T/#m8cdf33180d86c7ec22932e2eb4ef7dd4fc94c792 > ------- > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> No need for this hunk. > --- > include/uapi/linux/landlock.h | 29 +++++++++ > security/landlock/limits.h | 3 + > security/landlock/ruleset.c | 7 ++- > security/landlock/ruleset.h | 23 ++++++- > security/landlock/syscalls.c | 14 +++-- > security/landlock/task.c | 112 ++++++++++++++++++++++++++++++++++ > 6 files changed, 181 insertions(+), 7 deletions(-) > diff --git a/security/landlock/task.c b/security/landlock/task.c > index 849f5123610b..597d89e54aae 100644 > --- a/security/landlock/task.c > +++ b/security/landlock/task.c > @@ -13,6 +13,8 @@ > #include <linux/lsm_hooks.h> > #include <linux/rcupdate.h> > #include <linux/sched.h> > +#include <net/sock.h> > +#include <net/af_unix.h> > > #include "common.h" > #include "cred.h" > @@ -108,9 +110,119 @@ static int hook_ptrace_traceme(struct task_struct *const parent) > return task_ptrace(parent, current); > } > > +static int walk_and_check(const struct landlock_ruleset *const child, > + struct landlock_hierarchy **walker, int i, int j, We don't know what are "i" and "j" are while reading this function's signature. They need a better name. Also, they are ingegers (signed), whereas l1 and l2 are size_t (unsigned). > + bool check) > +{ > + if (!child || i < 0) > + return -1; > + > + while (i < j && *walker) { This would be more readable with a for() loop. > + if (check && landlock_get_scope_mask(child, j)) This is correct now but it will be a bug when we'll have other scope. Instead, you can replace the "check" boolean with a variable containing LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET. > + return -1; > + *walker = (*walker)->parent; > + j--; > + } > + if (!*walker) > + pr_warn_once("inconsistency in landlock hierarchy and layers"); This must indeed never happen, but WARN_ON_ONCE(!*walker) would be better than this check+pr_warn. Anyway, if this happen this pointer will still be dereferenced in domain_sock_scope() right? This must not be possible. > + return j; Because j is now equal to i, no need to return it. This function can return a boolean instead, or a struct landlock_ruleset pointer/NULL to avoid the pointer of pointer? > +} > + > +/** > + * domain_sock_scope - Checks if client domain is scoped in the same > + * domain as server. > + * > + * @client: Connecting socket domain. > + * @server: Listening socket domain. > + * > + * Checks if the @client domain is scoped, then the server should be > + * in the same domain to connect. If not, @client can connect to @server. > + */ > +static bool domain_sock_scope(const struct landlock_ruleset *const client, This function can have a more generic name if LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET is passed as argument. This could be reused as-is for other kind of scope. > + const struct landlock_ruleset *const server) > +{ > + size_t l1, l2; > + int scope_layer; > + struct landlock_hierarchy *cli_walker, *srv_walker; We have some room for a bit more characters ;) client_walker, server_walker; > + > + if (!client) > + return true; > + > + l1 = client->num_layers - 1; Please rename variables in a consistent way, in this case something like client_layer? > + cli_walker = client->hierarchy; > + if (server) { > + l2 = server->num_layers - 1; > + srv_walker = server->hierarchy; > + } else > + l2 = 0; > + > + if (l1 > l2) > + scope_layer = walk_and_check(client, &cli_walker, l2, l1, true); Instead of mixing the layer number with an error code, walk_and_check() can return a boolean, take as argument &scope_layer, and update it. > + else if (l2 > l1) > + scope_layer = > + walk_and_check(server, &srv_walker, l1, l2, false); > + else > + scope_layer = l1; > + > + if (scope_layer == -1) > + return false; All these domains and layers checks are difficult to review. It needs at least some comments, and preferably also some code refactoring to avoid potential inconsistencies (checks). > + > + while (scope_layer >= 0 && cli_walker) { Why srv_walker is not checked? Could this happen? What would be the result? Please also use a for() loop here. > + if (landlock_get_scope_mask(client, scope_layer) & > + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET) { The logic needs to be explained. > + if (!server) > + return false; > + > + if (srv_walker == cli_walker) > + return true; > + > + return false; > + } > + cli_walker = cli_walker->parent; > + srv_walker = srv_walker->parent; > + scope_layer--; > + } > + return true; > +} > + > +static bool sock_is_scoped(struct sock *const other) > +{ > + const struct landlock_ruleset *dom_other; > + const struct landlock_ruleset *const dom = > + landlock_get_current_domain(); > + > + /* the credentials will not change */ > + lockdep_assert_held(&unix_sk(other)->lock); > + dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain; > + > + /* other is scoped, they connect if they are in the same domain */ > + return domain_sock_scope(dom, dom_other); > +} > + > +static int hook_unix_stream_connect(struct sock *const sock, > + struct sock *const other, > + struct sock *const newsk) > +{ > + if (sock_is_scoped(other)) > + return 0; > + > + return -EPERM; > +} > + > +static int hook_unix_may_send(struct socket *const sock, > + struct socket *const other) > +{ > + if (sock_is_scoped(other->sk)) > + return 0; > + > + return -EPERM; > +} > + > static struct security_hook_list landlock_hooks[] __ro_after_init = { > LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check), > LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme), > + LSM_HOOK_INIT(unix_stream_connect, hook_unix_stream_connect), > + LSM_HOOK_INIT(unix_may_send, hook_unix_may_send), > }; > > __init void landlock_add_task_hooks(void) > -- > 2.34.1 > >
On Fri, Jul 19, 2024 at 08:14:02PM +0200, Mickaël Salaün wrote: > On Wed, Jul 17, 2024 at 10:15:19PM -0600, Tahera Fahimi wrote: > > The 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. > > > > This patch implement two hooks, "unix_stream_connect" and "unix_may_send" to > > enforce this restriction. > > > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > > > > ------- Hello Mickaël, Thanks for the feedback. > Only "---" > > > v7: > > Thanks for the detailed changelog, it helps! > > > - 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 file's FD 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/outreachy/Zmi8Ydz4Z6tYtpY1@tahera-OptiPlex-5000/T/#m8cdf33180d86c7ec22932e2eb4ef7dd4fc94c792 > > > > ------- > > > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > > No need for this hunk. > > > > --- > > include/uapi/linux/landlock.h | 29 +++++++++ > > security/landlock/limits.h | 3 + > > security/landlock/ruleset.c | 7 ++- > > security/landlock/ruleset.h | 23 ++++++- > > security/landlock/syscalls.c | 14 +++-- > > security/landlock/task.c | 112 ++++++++++++++++++++++++++++++++++ > > 6 files changed, 181 insertions(+), 7 deletions(-) > > > diff --git a/security/landlock/task.c b/security/landlock/task.c > > index 849f5123610b..597d89e54aae 100644 > > --- a/security/landlock/task.c > > +++ b/security/landlock/task.c > > @@ -13,6 +13,8 @@ > > #include <linux/lsm_hooks.h> > > #include <linux/rcupdate.h> > > #include <linux/sched.h> > > +#include <net/sock.h> > > +#include <net/af_unix.h> > > > > #include "common.h" > > #include "cred.h" > > @@ -108,9 +110,119 @@ static int hook_ptrace_traceme(struct task_struct *const parent) > > return task_ptrace(parent, current); > > } > > > > +static int walk_and_check(const struct landlock_ruleset *const child, > > + struct landlock_hierarchy **walker, int i, int j, > > We don't know what are "i" and "j" are while reading this function's > signature. They need a better name. > > Also, they are ingegers (signed), whereas l1 and l2 are size_t (unsigned). > > > + bool check) > > +{ > > + if (!child || i < 0) > > + return -1; > > + > > + while (i < j && *walker) { > > This would be more readable with a for() loop. > > > + if (check && landlock_get_scope_mask(child, j)) > > This is correct now but it will be a bug when we'll have other scope. > Instead, you can replace the "check" boolean with a variable containing > LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET. > > > + return -1; > > + *walker = (*walker)->parent; > > + j--; > > + } > > + if (!*walker) > > + pr_warn_once("inconsistency in landlock hierarchy and layers"); > > This must indeed never happen, but WARN_ON_ONCE(!*walker) would be > better than this check+pr_warn. > > Anyway, if this happen this pointer will still be dereferenced in > domain_sock_scope() right? This must not be possible. > > > > + return j; > > Because j is now equal to i, no need to return it. This function can > return a boolean instead, or a struct landlock_ruleset pointer/NULL to > avoid the pointer of pointer? corret, in the next version, this function will return a boolean that shows chcking the hierarchy of domain is successful or not. > > +} > > + > > +/** > > + * domain_sock_scope - Checks if client domain is scoped in the same > > + * domain as server. > > + * > > + * @client: Connecting socket domain. > > + * @server: Listening socket domain. > > + * > > + * Checks if the @client domain is scoped, then the server should be > > + * in the same domain to connect. If not, @client can connect to @server. > > + */ > > +static bool domain_sock_scope(const struct landlock_ruleset *const client, > > This function can have a more generic name if > LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET is passed as argument. This could > be reused as-is for other kind of scope. > > > + const struct landlock_ruleset *const server) > > +{ > > + size_t l1, l2; > > + int scope_layer; > > + struct landlock_hierarchy *cli_walker, *srv_walker; > > We have some room for a bit more characters ;) > client_walker, server_walker; > > > + > > + if (!client) > > + return true; > > + > > + l1 = client->num_layers - 1; > > Please rename variables in a consistent way, in this case something like > client_layer? done > > + cli_walker = client->hierarchy; > > + if (server) { > > + l2 = server->num_layers - 1; > > + srv_walker = server->hierarchy; > > + } else > > + l2 = 0; > > + > > + if (l1 > l2) > > + scope_layer = walk_and_check(client, &cli_walker, l2, l1, true); > > Instead of mixing the layer number with an error code, walk_and_check() > can return a boolean, take as argument &scope_layer, and update it. > > > + else if (l2 > l1) > > + scope_layer = > > + walk_and_check(server, &srv_walker, l1, l2, false); > > + else > > + scope_layer = l1; > > + > > + if (scope_layer == -1) > > + return false; > > All these domains and layers checks are difficult to review. It needs at > least some comments, and preferably also some code refactoring to avoid > potential inconsistencies (checks). > > > + > > + while (scope_layer >= 0 && cli_walker) { > > Why srv_walker is not checked? Could this happen? What would be the > result? This is the same scenario as "walk_and_check". If the loop breaks because of cli_walker is null, then there is an inconsistency between num_layers and landlock_hierarchy. In normal scenario, we expect the loop breaks with condition(scope_layer>=0). > Please also use a for() loop here. > > > + if (landlock_get_scope_mask(client, scope_layer) & > > + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET) { > > The logic needs to be explained. > > > + if (!server) > > + return false; > > + > > + if (srv_walker == cli_walker) > > + return true; > > + > > + return false; > > + } > > + cli_walker = cli_walker->parent; > > + srv_walker = srv_walker->parent; > > + scope_layer--; > > + } > > + return true; > > +} > > + > > +static bool sock_is_scoped(struct sock *const other) > > +{ > > + const struct landlock_ruleset *dom_other; > > + const struct landlock_ruleset *const dom = > > + landlock_get_current_domain(); > > + > > + /* the credentials will not change */ > > + lockdep_assert_held(&unix_sk(other)->lock); > > + dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain; > > + > > + /* other is scoped, they connect if they are in the same domain */ > > + return domain_sock_scope(dom, dom_other); > > +} > > + > > +static int hook_unix_stream_connect(struct sock *const sock, > > + struct sock *const other, > > + struct sock *const newsk) > > +{ > > + if (sock_is_scoped(other)) > > + return 0; > > + > > + return -EPERM; > > +} > > + > > +static int hook_unix_may_send(struct socket *const sock, > > + struct socket *const other) > > +{ > > + if (sock_is_scoped(other->sk)) > > + return 0; > > + > > + return -EPERM; > > +} > > + > > static struct security_hook_list landlock_hooks[] __ro_after_init = { > > LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check), > > LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme), > > + LSM_HOOK_INIT(unix_stream_connect, hook_unix_stream_connect), > > + LSM_HOOK_INIT(unix_may_send, hook_unix_may_send), > > }; > > > > __init void landlock_add_task_hooks(void) > > -- > > 2.34.1 > > > >
On Wed, Jul 17, 2024 at 10:15:19PM -0600, Tahera Fahimi wrote: > The 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. > > This patch implement two hooks, "unix_stream_connect" and "unix_may_send" to > enforce this restriction. > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > > ------- > diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c > index 03b470f5a85a..799a50f11d79 100644 > --- a/security/landlock/syscalls.c > +++ b/security/landlock/syscalls.c > @@ -97,8 +97,9 @@ static void build_check_abi(void) > */ > ruleset_size = sizeof(ruleset_attr.handled_access_fs); > ruleset_size += sizeof(ruleset_attr.handled_access_net); > + ruleset_size += sizeof(ruleset_attr.scoped); > BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size); > - BUILD_BUG_ON(sizeof(ruleset_attr) != 16); > + BUILD_BUG_ON(sizeof(ruleset_attr) != 24); > > path_beneath_size = sizeof(path_beneath_attr.allowed_access); > path_beneath_size += sizeof(path_beneath_attr.parent_fd); > @@ -149,7 +150,7 @@ static const struct file_operations ruleset_fops = { > .write = fop_dummy_write, > }; > > -#define LANDLOCK_ABI_VERSION 5 > +#define LANDLOCK_ABI_VERSION 6 > > /** > * sys_landlock_create_ruleset - Create a new ruleset > @@ -170,7 +171,7 @@ static const struct file_operations ruleset_fops = { > * Possible returned errors are: > * > * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time; > - * - %EINVAL: unknown @flags, or unknown access, or too small @size; > + * - %EINVAL: unknown @flags, or unknown access, or uknown scope, or too small @size; You'll need to rebase on top of my next branch to take into account recent Günther's changes. > * - %E2BIG or %EFAULT: @attr or @size inconsistencies; > * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs. > */
On Thu, Jul 25, 2024 at 04:18:29PM +0200, Mickaël Salaün wrote: > On Wed, Jul 17, 2024 at 10:15:19PM -0600, Tahera Fahimi wrote: > > diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c > > index 03b470f5a85a..799a50f11d79 100644 > > --- a/security/landlock/syscalls.c > > +++ b/security/landlock/syscalls.c > > /** > > * sys_landlock_create_ruleset - Create a new ruleset > > @@ -170,7 +171,7 @@ static const struct file_operations ruleset_fops = { > > * Possible returned errors are: > > * > > * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time; > > - * - %EINVAL: unknown @flags, or unknown access, or too small @size; > > + * - %EINVAL: unknown @flags, or unknown access, or uknown scope, or too small @size; > > You'll need to rebase on top of my next branch to take into account > recent Günther's changes. Actually, I have missed this particular line in my recent documentation changes, but I agree, we should follow the advice from man-pages(7) consistently -- the preferred style is to list the same error code multiple times, if there are multiple possible conditions under which it can be returned. (Please also fix the typo in "uknown".) Thanks, —Günther
On Wed, Jul 17, 2024 at 10:15:19PM -0600, Tahera Fahimi wrote: > The 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. > > This patch implement two hooks, "unix_stream_connect" and "unix_may_send" to > enforce this restriction. > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > > ------- > 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 file's FD 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/outreachy/Zmi8Ydz4Z6tYtpY1@tahera-OptiPlex-5000/T/#m8cdf33180d86c7ec22932e2eb4ef7dd4fc94c792 > ------- > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > --- > include/uapi/linux/landlock.h | 29 +++++++++ > security/landlock/limits.h | 3 + > security/landlock/ruleset.c | 7 ++- > security/landlock/ruleset.h | 23 ++++++- > security/landlock/syscalls.c | 14 +++-- > security/landlock/task.c | 112 ++++++++++++++++++++++++++++++++++ > 6 files changed, 181 insertions(+), 7 deletions(-) > > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h > index 68625e728f43..9cd881673434 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,27 @@ struct landlock_net_port_attr { > #define LANDLOCK_ACCESS_NET_BIND_TCP (1ULL << 0) > #define LANDLOCK_ACCESS_NET_CONNECT_TCP (1ULL << 1) > /* clang-format on */ > + > +/** > + * DOC: scope > + * > + * .scoped attribute handles a set of restrictions on kernel IPCs through > + * the following flags. If you look at the generated documentation (once this doc is properly included), you'll see that this line ends in the Network flags section. > + * > + * 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: There is a formating issue here. > + * - %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 */
On Wed, Jul 17, 2024 at 10:15:19PM -0600, Tahera Fahimi wrote: > The 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. > > This patch implement two hooks, "unix_stream_connect" and "unix_may_send" to > enforce this restriction. > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > > ------- > 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 file's FD 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/outreachy/Zmi8Ydz4Z6tYtpY1@tahera-OptiPlex-5000/T/#m8cdf33180d86c7ec22932e2eb4ef7dd4fc94c792 > ------- > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > --- > include/uapi/linux/landlock.h | 29 +++++++++ > security/landlock/limits.h | 3 + > security/landlock/ruleset.c | 7 ++- > security/landlock/ruleset.h | 23 ++++++- > security/landlock/syscalls.c | 14 +++-- > security/landlock/task.c | 112 ++++++++++++++++++++++++++++++++++ > 6 files changed, 181 insertions(+), 7 deletions(-) > diff --git a/security/landlock/task.c b/security/landlock/task.c > index 849f5123610b..597d89e54aae 100644 > --- a/security/landlock/task.c > +++ b/security/landlock/task.c > +static int hook_unix_stream_connect(struct sock *const sock, > + struct sock *const other, > + struct sock *const newsk) > +{ We must check if the unix sockets use an abstract address. We need a new test to check against a the three types of addresse: pathname, unnamed (socketpair), and abstract. This should result into 6 variants because of the stream-oriented or dgram-oriented sockets to check both hooks. The test can do 3 checks: without any Landlock domain, with the client being sandboxed with a Landlock domain for filesystem restrictions only, and with a domain scoped for abstract unix sockets. For pathname you'll need to move the TMP_DIR define from fs_test.c into common.h and create a static path for the named socket. A fixture teardown should remove the socket and the directory. > + if (sock_is_scoped(other)) > + return 0; > + > + return -EPERM; > +} > + > +static int hook_unix_may_send(struct socket *const sock, > + struct socket *const other) > +{ Same here > + if (sock_is_scoped(other->sk)) > + return 0; > + > + return -EPERM; > +} > + > static struct security_hook_list landlock_hooks[] __ro_after_init = { > LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check), > LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme), > + LSM_HOOK_INIT(unix_stream_connect, hook_unix_stream_connect), > + LSM_HOOK_INIT(unix_may_send, hook_unix_may_send), Future access controls dedicated to pathname unix sockets may need these hooks too, but I guess we'll see where tehy fit the best when we'll be there. > }; > > __init void landlock_add_task_hooks(void) > -- > 2.34.1 > >
On Tue, Jul 30, 2024 at 06:05:15PM +0200, Mickaël Salaün wrote: > On Wed, Jul 17, 2024 at 10:15:19PM -0600, Tahera Fahimi wrote: > > The 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. > > > > This patch implement two hooks, "unix_stream_connect" and "unix_may_send" to > > enforce this restriction. > > > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > > > > ------- > > 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 file's FD 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/outreachy/Zmi8Ydz4Z6tYtpY1@tahera-OptiPlex-5000/T/#m8cdf33180d86c7ec22932e2eb4ef7dd4fc94c792 > > ------- > > > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com> > > --- > > include/uapi/linux/landlock.h | 29 +++++++++ > > security/landlock/limits.h | 3 + > > security/landlock/ruleset.c | 7 ++- > > security/landlock/ruleset.h | 23 ++++++- > > security/landlock/syscalls.c | 14 +++-- > > security/landlock/task.c | 112 ++++++++++++++++++++++++++++++++++ > > 6 files changed, 181 insertions(+), 7 deletions(-) > > > diff --git a/security/landlock/task.c b/security/landlock/task.c > > index 849f5123610b..597d89e54aae 100644 > > --- a/security/landlock/task.c > > +++ b/security/landlock/task.c > > > +static int hook_unix_stream_connect(struct sock *const sock, > > + struct sock *const other, > > + struct sock *const newsk) > > +{ > > We must check if the unix sockets use an abstract address. We need a > new test to check against a the three types of addresse: pathname, > unnamed (socketpair), and abstract. This should result into 6 variants > because of the stream-oriented or dgram-oriented sockets to check both > hooks. The test can do 3 checks: without any Landlock domain, with the > client being sandboxed with a Landlock domain for filesystem > restrictions only, and with a domain scoped for abstract unix sockets. correct. Since the current tests check all the possible scenarios for abstract unix sockets, we only need to add the scenarios where a socket with pathname or unnamed address and scoped domain wants to connect to a listening socket. In this case, the access should guaranteed since there is no scoping mechanism for these sockets yet. > For pathname you'll need to move the TMP_DIR define from fs_test.c into > common.h and create a static path for the named socket. A fixture > teardown should remove the socket and the directory. Thanks for the help :) > > + if (sock_is_scoped(other)) > > + return 0; > > + > > + return -EPERM; > > +} > > + > > +static int hook_unix_may_send(struct socket *const sock, > > + struct socket *const other) > > +{ > > Same here > > > + if (sock_is_scoped(other->sk)) > > + return 0; > > + > > + return -EPERM; > > +} > > + > > static struct security_hook_list landlock_hooks[] __ro_after_init = { > > LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check), > > LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme), > > + LSM_HOOK_INIT(unix_stream_connect, hook_unix_stream_connect), > > + LSM_HOOK_INIT(unix_may_send, hook_unix_may_send), > > Future access controls dedicated to pathname unix sockets may need these > hooks too, but I guess we'll see where tehy fit the best when we'll be > there. > > > }; > > > > __init void landlock_add_task_hooks(void) > > -- > > 2.34.1 > > > >
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h index 68625e728f43..9cd881673434 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,27 @@ struct landlock_net_port_attr { #define LANDLOCK_ACCESS_NET_BIND_TCP (1ULL << 0) #define LANDLOCK_ACCESS_NET_CONNECT_TCP (1ULL << 1) /* clang-format on */ + +/** + * DOC: scope + * + * .scoped attribute handles a set of restrictions on kernel IPCs through + * the following flags. + * + * Scope flags + * ~~~~~~~~~~~ + * + * These flags enable to restrict a sandboxed process from a set of IPC + * actions. Setting a flag for a ruleset will isolate the Landlock domain + * to forbid connections to resources outside the domain. + * + * IPCs with scoped actions: + * - %LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET: Restrict a sandboxed process + * from connecting to an abstract unix socket created by a process + * outside the related Landlock domain (e.g. a parent domain or a + * non-sandboxed process). + */ +/* clang-format off */ +#define LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET (1ULL << 0) +/* clang-format on*/ #endif /* _UAPI_LINUX_LANDLOCK_H */ diff --git a/security/landlock/limits.h b/security/landlock/limits.h index 4eb643077a2a..eb01d0fb2165 100644 --- a/security/landlock/limits.h +++ b/security/landlock/limits.h @@ -26,6 +26,9 @@ #define LANDLOCK_MASK_ACCESS_NET ((LANDLOCK_LAST_ACCESS_NET << 1) - 1) #define LANDLOCK_NUM_ACCESS_NET __const_hweight64(LANDLOCK_MASK_ACCESS_NET) +#define LANDLOCK_LAST_SCOPE LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET +#define LANDLOCK_MASK_SCOPE ((LANDLOCK_LAST_SCOPE << 1) - 1) +#define LANDLOCK_NUM_SCOPE __const_hweight64(LANDLOCK_MASK_SCOPE) /* clang-format on */ #endif /* _SECURITY_LANDLOCK_LIMITS_H */ diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c index 6ff232f58618..a93bdbf52fff 100644 --- a/security/landlock/ruleset.c +++ b/security/landlock/ruleset.c @@ -52,12 +52,13 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers) struct landlock_ruleset * landlock_create_ruleset(const access_mask_t fs_access_mask, - const access_mask_t net_access_mask) + const access_mask_t net_access_mask, + const access_mask_t scope_mask) { struct landlock_ruleset *new_ruleset; /* Informs about useless ruleset. */ - if (!fs_access_mask && !net_access_mask) + if (!fs_access_mask && !net_access_mask && !scope_mask) return ERR_PTR(-ENOMSG); new_ruleset = create_ruleset(1); if (IS_ERR(new_ruleset)) @@ -66,6 +67,8 @@ landlock_create_ruleset(const access_mask_t fs_access_mask, landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0); if (net_access_mask) landlock_add_net_access_mask(new_ruleset, net_access_mask, 0); + if (scope_mask) + landlock_add_scope_mask(new_ruleset, scope_mask, 0); return new_ruleset; } diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h index 0f1b5b4c8f6b..c749fa0b3ecd 100644 --- a/security/landlock/ruleset.h +++ b/security/landlock/ruleset.h @@ -35,6 +35,8 @@ typedef u16 access_mask_t; static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS); /* Makes sure all network access rights can be stored. */ static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET); +/* Makes sure all scoped rights can be stored*/ +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE); /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t)); @@ -42,6 +44,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t)); struct access_masks { access_mask_t fs : LANDLOCK_NUM_ACCESS_FS; access_mask_t net : LANDLOCK_NUM_ACCESS_NET; + access_mask_t scoped : LANDLOCK_NUM_SCOPE; }; typedef u16 layer_mask_t; @@ -233,7 +236,8 @@ struct landlock_ruleset { struct landlock_ruleset * landlock_create_ruleset(const access_mask_t access_mask_fs, - const access_mask_t access_mask_net); + const access_mask_t access_mask_net, + const access_mask_t scope_mask); void landlock_put_ruleset(struct landlock_ruleset *const ruleset); void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset); @@ -280,6 +284,16 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset, ruleset->access_masks[layer_level].net |= net_mask; } +static inline void +landlock_add_scope_mask(struct landlock_ruleset *const ruleset, + const access_mask_t scope_mask, const u16 layer_level) +{ + access_mask_t scoped_mask = scope_mask & LANDLOCK_MASK_SCOPE; + + WARN_ON_ONCE(scope_mask != scoped_mask); + ruleset->access_masks[layer_level].scoped |= scoped_mask; +} + static inline access_mask_t landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset, const u16 layer_level) @@ -303,6 +317,13 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset, return ruleset->access_masks[layer_level].net; } +static inline access_mask_t +landlock_get_scope_mask(const struct landlock_ruleset *const ruleset, + const u16 layer_level) +{ + return ruleset->access_masks[layer_level].scoped; +} + bool landlock_unmask_layers(const struct landlock_rule *const rule, const access_mask_t access_request, layer_mask_t (*const layer_masks)[], diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c index 03b470f5a85a..799a50f11d79 100644 --- a/security/landlock/syscalls.c +++ b/security/landlock/syscalls.c @@ -97,8 +97,9 @@ static void build_check_abi(void) */ ruleset_size = sizeof(ruleset_attr.handled_access_fs); ruleset_size += sizeof(ruleset_attr.handled_access_net); + ruleset_size += sizeof(ruleset_attr.scoped); BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size); - BUILD_BUG_ON(sizeof(ruleset_attr) != 16); + BUILD_BUG_ON(sizeof(ruleset_attr) != 24); path_beneath_size = sizeof(path_beneath_attr.allowed_access); path_beneath_size += sizeof(path_beneath_attr.parent_fd); @@ -149,7 +150,7 @@ static const struct file_operations ruleset_fops = { .write = fop_dummy_write, }; -#define LANDLOCK_ABI_VERSION 5 +#define LANDLOCK_ABI_VERSION 6 /** * sys_landlock_create_ruleset - Create a new ruleset @@ -170,7 +171,7 @@ static const struct file_operations ruleset_fops = { * Possible returned errors are: * * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time; - * - %EINVAL: unknown @flags, or unknown access, or too small @size; + * - %EINVAL: unknown @flags, or unknown access, or uknown scope, or too small @size; * - %E2BIG or %EFAULT: @attr or @size inconsistencies; * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs. */ @@ -213,9 +214,14 @@ SYSCALL_DEFINE3(landlock_create_ruleset, LANDLOCK_MASK_ACCESS_NET) return -EINVAL; + /* Checks IPC scoping content (and 32-bits cast). */ + if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE) + return -EINVAL; + /* Checks arguments and transforms to kernel struct. */ ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs, - ruleset_attr.handled_access_net); + ruleset_attr.handled_access_net, + ruleset_attr.scoped); if (IS_ERR(ruleset)) return PTR_ERR(ruleset); diff --git a/security/landlock/task.c b/security/landlock/task.c index 849f5123610b..597d89e54aae 100644 --- a/security/landlock/task.c +++ b/security/landlock/task.c @@ -13,6 +13,8 @@ #include <linux/lsm_hooks.h> #include <linux/rcupdate.h> #include <linux/sched.h> +#include <net/sock.h> +#include <net/af_unix.h> #include "common.h" #include "cred.h" @@ -108,9 +110,119 @@ static int hook_ptrace_traceme(struct task_struct *const parent) return task_ptrace(parent, current); } +static int walk_and_check(const struct landlock_ruleset *const child, + struct landlock_hierarchy **walker, int i, int j, + bool check) +{ + if (!child || i < 0) + return -1; + + while (i < j && *walker) { + if (check && landlock_get_scope_mask(child, j)) + return -1; + *walker = (*walker)->parent; + j--; + } + if (!*walker) + pr_warn_once("inconsistency in landlock hierarchy and layers"); + return j; +} + +/** + * domain_sock_scope - Checks if client domain is scoped in the same + * domain as server. + * + * @client: Connecting socket domain. + * @server: Listening socket domain. + * + * Checks if the @client domain is scoped, then the server should be + * in the same domain to connect. If not, @client can connect to @server. + */ +static bool domain_sock_scope(const struct landlock_ruleset *const client, + const struct landlock_ruleset *const server) +{ + size_t l1, l2; + int scope_layer; + struct landlock_hierarchy *cli_walker, *srv_walker; + + if (!client) + return true; + + l1 = client->num_layers - 1; + cli_walker = client->hierarchy; + if (server) { + l2 = server->num_layers - 1; + srv_walker = server->hierarchy; + } else + l2 = 0; + + if (l1 > l2) + scope_layer = walk_and_check(client, &cli_walker, l2, l1, true); + else if (l2 > l1) + scope_layer = + walk_and_check(server, &srv_walker, l1, l2, false); + else + scope_layer = l1; + + if (scope_layer == -1) + return false; + + while (scope_layer >= 0 && cli_walker) { + if (landlock_get_scope_mask(client, scope_layer) & + LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET) { + if (!server) + return false; + + if (srv_walker == cli_walker) + return true; + + return false; + } + cli_walker = cli_walker->parent; + srv_walker = srv_walker->parent; + scope_layer--; + } + return true; +} + +static bool sock_is_scoped(struct sock *const other) +{ + const struct landlock_ruleset *dom_other; + const struct landlock_ruleset *const dom = + landlock_get_current_domain(); + + /* the credentials will not change */ + lockdep_assert_held(&unix_sk(other)->lock); + dom_other = landlock_cred(other->sk_socket->file->f_cred)->domain; + + /* other is scoped, they connect if they are in the same domain */ + return domain_sock_scope(dom, dom_other); +} + +static int hook_unix_stream_connect(struct sock *const sock, + struct sock *const other, + struct sock *const newsk) +{ + if (sock_is_scoped(other)) + return 0; + + return -EPERM; +} + +static int hook_unix_may_send(struct socket *const sock, + struct socket *const other) +{ + if (sock_is_scoped(other->sk)) + return 0; + + return -EPERM; +} + static struct security_hook_list landlock_hooks[] __ro_after_init = { LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check), LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme), + LSM_HOOK_INIT(unix_stream_connect, hook_unix_stream_connect), + LSM_HOOK_INIT(unix_may_send, hook_unix_may_send), }; __init void landlock_add_task_hooks(void)