[PATCH v9 1/5] Landlock: Add abstract unix socket connect restriction
Mickaël Salaün
mic at digikod.net
Tue Aug 20 15:56:30 UTC 2024
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 at 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
> > >
> > >
>
More information about the Linux-security-module-archive
mailing list