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 <[email protected]>
> > 
> > ---
> > 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/[email protected]/
> >   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/[email protected]/
> > ---
> >  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 */