Message-ID: <Zo7C7MUfnPApp0Np@tahera-OptiPlex-5000>
Date: Wed, 10 Jul 2024 11:20:44 -0600
From: Tahera Fahimi <fahimitahera@...il.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: Günther Noack <gnoack@...gle.com>,
	Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Jann Horn <jannh@...gle.com>, outreachy@...ts.linux.dev,
	netdev@...r.kernel.org
Subject: Re: [PATCH v6] landlock: Add abstract unix socket connect restriction

On Mon, Jul 08, 2024 at 05:35:48PM +0200, Mickaël Salaün wrote:
> Please add a user documentation with the next version.  You can take
> some inspiration in commits that changed
> Documentation/userspace-api/landlock.rst
> 
> You also need to extend samples/landlock/sandboxer.c with this new
> feature.  You might want to use a new environment variable (LL_SCOPED)
> with "a" (for abstract unix socket) as the only valid content.  New kind
> of sopping could add new characters.  I'm not sure this is the most
> ergonomic, but let's go this way unless you have something else in mind.
Thanks for the feedback. 
This will be added in the next patch. 

> All the related patches (kernel change, tests, sample, documentation)
> should be in the same patch series, with a cover letter introducing the
> feature and pointing to the previous versions with links to
> https://lore.kernel.org/r/...
Noted.
> 
> On Thu, Jun 27, 2024 at 05:30:17PM -0600, Tahera Fahimi wrote:
> > Abstract unix sockets are used for local inter-process communications
> > without a filesystem. Currently a sandboxed process can connect to a
> 
> "local inter-process communications independant of the filesystem."
> 
> > socket outside of the sandboxed environment, since Landlock has no
> > restriction for connecting to an abstract socket address. Access to
> > such sockets for a sandboxed process should be scoped the same way
> > ptrace is limited.
> > 
> > Because of compatibility reasons and since landlock should be flexible,
> 
> Landlock

[...]
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 68625e728f43..010aaca5b05a 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -37,6 +37,12 @@ struct landlock_ruleset_attr {
> >  	 * rule explicitly allow them.
> >  	 */
> >  	__u64 handled_access_net;
> > +	/**
> > +	 * @scoped: Bitmask of scopes (cf. `Scope flags`_)
> > +	 * restricting a Landlock domain from accessing outside
> > +	 * resources(e.g. IPCs).
> 
> A space is missing.
> 
> > +	 */
> > +	__u64 scoped;
> >  };
> >  
> >  /*
> > @@ -266,4 +272,27 @@ 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
> > + *
> > + * .scoped attribute handles a set of restrictions on kernel IPCs through
> > + * the following flags.
> 
> I think you can remove this sentence.
> 
[...] 
> > diff --git a/security/landlock/task.c b/security/landlock/task.c
> > index 849f5123610b..acc6e0fbc111 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,69 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
> >  	return task_ptrace(parent, current);
> >  }
> >  
> > +static access_mask_t
> > +get_scoped_accesses(const struct landlock_ruleset *const domain)
> > +{
> > +	access_mask_t access_dom = 0;
> > +	size_t layer_level;
> > +
> > +	for (layer_level = 0; layer_level < domain->num_layers; layer_level++)
> > +		access_dom |= landlock_get_scope_mask(domain, layer_level);
> > +	return access_dom;
> > +}
> > +
> > +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();
> > +
> > +	/* quick return if there is no domain or .scoped is not set */
> > +	if (!dom || !get_scoped_accesses(dom))
> > +		return true;
> > +
> > +	/* the credentials will not change */
> > +	lockdep_assert_held(&unix_sk(other)->lock);
> > +	if (other->sk_type != SOCK_DGRAM) {
> > +		dom_other = landlock_cred(other->sk_peer_cred)->domain;
> 
> Why using different credentials for connected or not connected sockets?
> We should use the same consistent logic for both:
> other->sk_socket->file->f_cred (the process that created the socket, not
> the one listening).
The aim was to use the process's credential that utilized the socket for
connected sockets, and the process's credential created the socket for
non-connected sockets. However, I will change it and use the same
credential to keep it consistent for both cases. 

> > +	} else {
> > +		dom_other =
> > +			landlock_cred(other->sk_socket->file->f_cred)->domain;
> > +	}
> > +
> > +	if (!dom_other || !get_scoped_accesses(dom_other))
> 
> What if only one layer in dom_other is scoped?
The function `get_scoped_accesses()` cover this. 

> > +		return false;
> > +
> > +	/* other is scoped, they connect if they are in the same domain */
> 
> This doesn't fit with each domain's scoping. It only considers no
> scopping for all domains, or all domains as scopped if any of them is.
> domain_scope_le() needs to be changed to follow each domain's contract.
Noted.

> > +	return domain_scope_le(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)
> > +{
> > +	pr_warn("XXX %s:%d sock->file:%p other->file:%p\n", __func__, __LINE__,
> > +		sock->file, other->file);
> 
> Please remove debug code.
> 
> > +	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
> > 
> > 

Powered by blists - more mailing lists