Received-SPF: pass (google.com: domain of linux-kernel+bounces-127665-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 2 Apr 2024 11:53:09 +0200
From: =?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?= <mic@digikod.net>
To: TaheraFahimi <fahimitahera@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>, James Morris <jmorris@namei.org>, 
	"Serge E. Hallyn" <serge@hallyn.com>, linux-security-module@vger.kernel.org, 
	linux-kernel@vger.kernel.org, outreachy@lists.linux.dev, netdev@vger.kernel.org, 
	=?utf-8?B?QmrDtnJu?= Roy Baron <bjorn3_gh@protonmail.com>, Jann Horn <jannh@google.com>
Subject: Re: [PATCH v2] landlock: Add abstract unix socket connect
 restrictions
Message-ID: <20240401.ieC2uqua5sha@digikod.net>
References: <ZgX5TRTrSDPrJFfF@tahera-OptiPlex-5000>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZgX5TRTrSDPrJFfF@tahera-OptiPlex-5000>

Thanks for this patch.  Please CC the netdev mailing list too, they may
be interested by this feature. I also added a few folks that previously
showed their interest for this feature.

On Thu, Mar 28, 2024 at 05:12:13PM -0600, TaheraFahimi wrote:
> Abstract unix sockets are used for local interprocess communication without
> relying on filesystem. Since landlock has no restriction for connecting to
> a UNIX socket in the abstract namespace, a sandboxed process can connect to
> a socket outside the sandboxed environment. Access to such sockets should
> be scoped the same way ptrace access is limited.

This is good but it would be better to explain that Landlock doesn't
currently control abstract unix sockets and that it would make sense for
a sandbox.


> 
> For a landlocked process to be allowed to connect to a target process, it
> must have a subset of the target process’s rules (the connecting socket
> must be in a sub-domain of the listening socket). This patch adds a new
> LSM hook for connect function in unix socket with the related access rights.

Because of compatibility reasons, and because Landlock should be
flexible, we need to extend the user space interface.  As explained in
the GitHub issue, we need to add a new "scoped" field to the
landlock_ruleset_attr struct. This field will optionally contain a
LANDLOCK_RULESET_SCOPED_ABSTRACT_UNIX_SOCKET flag to specify that this
ruleset will deny any connection from within the sandbox to its parents
(i.e. any parent sandbox or not-sandboxed processes).

> 
> Link to first draft:
> 	https://lore.kernel.org/outreachy/20240328.ShoR4Iecei8o@digikod.net/

You can move this sentence in the below changelog.

> 

You can add this:

Closes: https://github.com/landlock-lsm/linux/issues/7

> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>

Your Git (or email app) configuration doesn't use the same name as here.

Please run ./scripts/checkpatch.pl on this patch and fix the warnings.

> 
> ----
> Changes in v2:
> - Remove wrapper functions, noted by Casey Schaufler <casey@schaufler-ca.com>
> ---
>  security/landlock/task.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 849f5123610b..67528f87b7de 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c
> @@ -13,6 +13,7 @@
>  #include <linux/lsm_hooks.h>
>  #include <linux/rcupdate.h>
>  #include <linux/sched.h>
> +#include <net/sock.h>
>  
>  #include "common.h"
>  #include "cred.h"
> @@ -108,9 +109,48 @@ static int hook_ptrace_traceme(struct task_struct *const parent)
>  	return task_ptrace(parent, current);
>  }
>  
> +static bool unix_sock_is_scoped(struct sock *const sock,

For consistency with task_is_scoped(), you can rename this to
sock_is_scoped().

> +				struct sock *const other)
> +{
> +	bool is_scoped = true;
> +
> +	/* get the ruleset of connecting sock*/

These comments don't help more than the following line, you can remove
them.

> +	const struct landlock_ruleset *const dom_sock =

According to the name it looks like the domain of the socket but it is
just the domain of the current task. Just "dom" would be clearer and
more consistent with security/landlock/fs.c

> +		landlock_get_current_domain();
> +
> +	if (!dom_sock)
> +		return true;
> +
> +	/* get credential of listening sock*/
> +	const struct cred *cred_other = get_cred(other->sk_peer_cred);

We have a get but not a put call, so the credentials will never be
freed.  The put call must be called before any return, so you
probably need to follow the goto/error pattern.

In the context of these LSM hooks, only unix_listen() sets the "other"
socket credential, and unix_listen() is guarded by unix_state_lock()
which locks unix_sk(s)->lock .  When security_unix_stream_connect() or
security_unix_may_send() are called, unix_sk(s)->lock is locked as well,
which protects the credentials against race-conditions (TOCTOU:
time-of-check to time-of-use).  We should then make that explicit with
this assertion (which also documents it):

lockdep_assert_held(&unix_sk(other)->lock);

In theory it is then not required to call get_cred().  However, because
the performance impact should be negligible and to avoid a potential
use-after-free (not possible in theory with the current code), it would
be safer to still call get/put.  It would be worse to have a
use-after-free rather than an access control issue.

Another thing to keep in mind is that for this hook to be
race-condition-free, the credential must not change anyway.  A comment
should highlight that.

> +
> +	if (!cred_other)
> +		return true;
> +
> +	/* retrieve the landlock_rulesets */
> +	const struct landlock_ruleset *dom_parent;

All declarations should be at the top of functions.

> +
> +	rcu_read_lock();

No need for this RCU lock because the lock is managed by
unix_state_lock() in this case.

> +	dom_parent = landlock_cred(cred_other)->domain;
> +	is_scoped = domain_scope_le(dom_parent, dom_sock);
> +	rcu_read_unlock();
> +
> +	return is_scoped;
> +}
> +
> +static int hook_unix_stream_connect(struct sock *const sock,
> +				    struct sock *const other,
> +				    struct sock *const newsk)
> +{
> +	if (unix_sock_is_scoped(sock, other))
> +		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),

Please add a hook for security_unix_may_send() too, it should be quite
similar, and simplify the patch's subject accordingly.

You now need to add tests (in a dedicated patch) extending
tools/testing/selftests/landlock/ptrace_test.c (I'll rename the file
later).

These tests should also check with unnamed and named unix sockets.  I
guess the current code doesn't differentiate them and control all kind
of unix sockets.  Because they must explicitly be passed, sockets
created with socketpair(2) (i.e. unnamed socket) should never be denied.

>  };
>  
>  __init void landlock_add_task_hooks(void)
> -- 
> 2.34.1
> 
>