Re: [PATCH v7 1/4] Landlock: Add abstract unix socket connect restriction

[Tahera Fahimi]

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@xxxxxxxxx>
> > 
> > -------
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@xxxxxxxxxx/
> >    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@xxxxxxxxx>
> 
> 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
> > 
> >