Message-ID: <20240808.aiQu4ohho1xi@digikod.net>
Date: Fri, 9 Aug 2024 16:13:08 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Tahera Fahimi <fahimitahera@...il.com>
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 v8 1/4] Landlock: Add abstract unix socket connect
 restriction

On Wed, Aug 07, 2024 at 09:37:15AM -0600, Tahera Fahimi wrote:
> On Sat, Aug 03, 2024 at 01:29:04PM +0200, Mickaël Salaün wrote:
> > On Thu, Aug 01, 2024 at 10:02:33PM -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>
> > > 
> > > ---
> > > 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 |  30 +++++++
> > >  security/landlock/limits.h    |   3 +
> > >  security/landlock/ruleset.c   |   7 +-
> > >  security/landlock/ruleset.h   |  23 ++++-
> > >  security/landlock/syscalls.c  |  14 ++-
> > >  security/landlock/task.c      | 155 ++++++++++++++++++++++++++++++++++
> > >  6 files changed, 225 insertions(+), 7 deletions(-)
> > 
> > > diff --git a/security/landlock/task.c b/security/landlock/task.c
> > > index 849f5123610b..7e8579ebae83 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,162 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> > >  	return task_ptrace(parent, current);
> > >  }
> > >  
> > > +static bool walk_and_check(const struct landlock_ruleset *const child,
> > > +			   struct landlock_hierarchy **walker,
> > > +			   size_t base_layer, size_t deep_layer,
> > > +			   access_mask_t check_scoping)
> > 
> > s/check_scoping/scope/
> > 
> > > +{
> > > +	if (!child || base_layer < 0 || !(*walker))
> > 
> > I guess it should be:
> > WARN_ON_ONCE(!child || base_layer < 0 || !(*walker))
> > 
> > > +		return false;
> > > +
> > > +	for (deep_layer; base_layer < deep_layer; deep_layer--) {
> > 
> > No need to pass deep_layer as argument:
> > deep_layer = child->num_layers - 1
> > 
> > > +		if (check_scoping & landlock_get_scope_mask(child, deep_layer))
> > > +			return false;
> > > +		*walker = (*walker)->parent;
> > > +		if (WARN_ON_ONCE(!*walker))
> > > +			/* there is an inconsistency between num_layers
> > 
> > Please use full sentences starting with a capital letter and ending with
> > a dot, and in this case start with "/*"
> > 
> > > +			 * and landlock_hierarchy in the ruleset
> > > +			 */
> > > +			return false;
> > > +	}
> > > +	return true;
> > > +}
> > > +
> > > +/**
> > > + * domain_IPC_scope - Checks if the client domain is scoped in the same
> > > + *		      domain as the server.
> > 
> > Actually, you can remove IPC from the function name.
> > 
> > > + *
> > > + * @client: IPC sender domain.
> > > + * @server: IPC receiver domain.
> > > + *
> > > + * Check if the @client domain is scoped to access the @server; the @server
> > > + * must be scoped in the same domain.
> > 
> > Returns true if...
> > 
> > > + */
> > > +static bool domain_IPC_scope(const struct landlock_ruleset *const client,
> > > +			     const struct landlock_ruleset *const server,
> > > +			     access_mask_t ipc_type)
> > > +{
> > > +	size_t client_layer, server_layer = 0;
> > > +	int base_layer;
> > > +	struct landlock_hierarchy *client_walker, *server_walker;
> > > +	bool is_scoped;
> > > +
> > > +	/* Quick return if client has no domain */
> > > +	if (!client)
> > > +		return true;
> > > +
> > > +	client_layer = client->num_layers - 1;
> > > +	client_walker = client->hierarchy;
> > > +	if (server) {
> > > +		server_layer = server->num_layers - 1;
> > > +		server_walker = server->hierarchy;
> > > +	}
> > 
> > } else {
> > 	server_layer = 0;
> > 	server_walker = NULL;
> > }
> > 
> > > +	base_layer = (client_layer > server_layer) ? server_layer :
> > > +						     client_layer;
> > > +
> > > +	/* For client domain, walk_and_check ensures the client domain is
> > > +	 * not scoped until gets to base_layer.
> > 
> > until gets?
> > 
> > > +	 * For server_domain, it only ensures that the server domain exist.
> > > +	 */
> > > +	if (client_layer != server_layer) {
> > 
> > bool is_scoped;
> It is defined above. 

Yes, but it should be defined here because it is not used outside of
this block.

> > > +		if (client_layer > server_layer)
> > > +			is_scoped = walk_and_check(client, &client_walker,
> > > +						   server_layer, client_layer,
> > > +						   ipc_type);
> > > +		else
> > 
> > server_walker may be uninitialized and still read here, and maybe later
> > in the for loop.  The whole code should maks sure this cannot happen,
> > and a test case should check this.
> I think this case never happens, since the server_walker can be read
> here if there are more than one layers in server domain which means that
> the server_walker is not unintialized.

Yes, but this code makes it more difficult to convince yourself.  The
proposed refactoring should help.

> 
> > > +			is_scoped = walk_and_check(server, &server_walker,
> > > +						   client_layer, server_layer,
> > > +						   ipc_type & 0);
> > 
> > "ipc_type & 0" is the same as "0"
> > 
> > > +		if (!is_scoped)
> > 
> > The name doesn't reflect the semantic. walk_and_check() should return
> > the inverse.
> > 
> > > +			return false;
> > > +	}
> > 
> > This code would be simpler:
> > 
> > if (client_layer > server_layer) {
> > 	base_layer = server_layer;
> > 	// TODO: inverse boolean logic
> > 	if (!walk_and_check(client, &client_walker,
> > 				   base_layer, ipc_type))
> > 		return false;
> > } else (client_layer < server_layer) {
> > 	base_layer = client_layer;
> > 	// TODO: inverse boolean logic
> > 	if (!walk_and_check(server, &server_walker,
> > 				   base_layer, 0))
> > 		return false;
> > } else {
> > 	base_layer = client_layer;
> > }
> > 
> > 
> > I think we can improve more to make sure there is no path/risk of
> > inconsistent pointers.
> > 
> > 
> > > +	/* client and server are at the same level in hierarchy. If client is
> > > +	 * scoped, the server must be scoped in the same domain
> > > +	 */
> > > +	for (base_layer; base_layer >= 0; base_layer--) {
> > > +		if (landlock_get_scope_mask(client, base_layer) & ipc_type) {
> > 
> > With each multi-line comment, the first line should be empty:
> > /*
> >  * This check must be here since access would be denied only if
> > 
> > > +			/* This check must be here since access would be denied only if
> > > +			 * the client is scoped and the server has no domain, so
> > > +			 * if the client has a domain but is not scoped and the server
> > > +			 * has no domain, access is guaranteed.
> > > +			 */
> > > +			if (!server)
> > > +				return false;
> > > +
> > > +			if (server_walker == client_walker)
> > > +				return true;
> > > +
> > > +			return false;
> > > +		}
> > > +		client_walker = client_walker->parent;
> > > +		server_walker = server_walker->parent;
> > > +		/* Warn if there is an incosistenncy between num_layers and
> > 
> > Makes sure there is no inconsistency between num_layers and
> > 
> > 
> > > +		 * landlock_hierarchy in each of rulesets
> > > +		 */
> > > +		if (WARN_ON_ONCE(base_layer > 0 &&
> > > +				 (!server_walker || !client_walker)))
> > > +			return false;
> > > +	}
> > > +	return true;
> > > +}
> 

Powered by blists - more mailing lists