[PATCH v7 1/4] Landlock: Add abstract unix socket connect restriction
Tahera Fahimi
fahimitahera at gmail.com
Tue Jul 23 01:13:44 UTC 2024
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 at 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 at 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
> >
> >
More information about the Linux-security-module-archive
mailing list