[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZuglWy71qvgEhJQ4@tahera-OptiPlex-5000>
Date: Mon, 16 Sep 2024 06:32:27 -0600
From: Tahera Fahimi <fahimitahera@...il.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: outreachy@...ts.linux.dev, gnoack@...gle.com, paul@...l-moore.com,
jmorris@...ei.org, serge@...lyn.com,
linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
bjorn3_gh@...tonmail.com, jannh@...gle.com, netdev@...r.kernel.org
Subject: Re: [PATCH v11 1/8] Landlock: Add abstract UNIX socket restriction
On Fri, Sep 13, 2024 at 03:32:59PM +0200, Mickaël Salaün wrote:
> On Wed, Sep 04, 2024 at 06:13:55PM -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@...il.com>
> >
> > ---
> > v11:
> > - For a connected abstract datagram socket, the hook_unix_may_send
> > allows the socket to send a data. (it is treated as a connected stream
> > socket)
> > - Minor comment revision.
> > v10:
> > - Minor code improvement based on reviews on v9.
> > 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 | 28 ++++
> > security/landlock/limits.h | 3 +
> > security/landlock/ruleset.c | 7 +-
> > security/landlock/ruleset.h | 24 +++-
> > security/landlock/syscalls.c | 17 ++-
> > security/landlock/task.c | 136 +++++++++++++++++++
> > tools/testing/selftests/landlock/base_test.c | 2 +-
> > 7 files changed, 208 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 2c8dbc74b955..dfd48d722834 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -44,6 +44,12 @@ struct landlock_ruleset_attr {
> > * flags`_).
> > */
> > __u64 handled_access_net;
> > + /**
> > + * @scoped: Bitmask of scopes (cf. `Scope flags`_)
> > + * restricting a Landlock domain from accessing outside
> > + * resources(e.g. IPCs).
> > + */
> > + __u64 scoped;
> > };
> >
> > /*
> > @@ -274,4 +280,26 @@ struct landlock_net_port_attr {
> > #define LANDLOCK_ACCESS_NET_BIND_TCP (1ULL << 0)
> > #define LANDLOCK_ACCESS_NET_CONNECT_TCP (1ULL << 1)
> > /* clang-format on */
> > +
> > +/**
> > + * DOC: scope
> > + *
> > + * 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)
>
> Thinking more about it, it makes more sense to rename it to
> LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET (s/SCOPED/SCOPE/) because it
> express a scope (not a "scoped") and it allign with the current
> LANDLOCK_ACCESS_* and other internal variables such as
> LANDLOCK_LAST_SCOPE...
>
> However, it still makes sense to keep the "scoped" ruleset's field,
> which is pretty similar to the "handled_*" semantic: it describes what
> will be *scoped* by the ruleset.
The proposed changes make sense. They are applied in commit
[0b365024c726277eb73e461849709605d1819977]/next branch, and look good
to me.
> > +/* 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 */
Powered by blists - more mailing lists