Re: [RFC PATCH v1] landlock: Allow signals between threads of the same process

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 13, 2025 at 03:59:04PM +0100, Mickaël Salaün wrote:
> Because Linux credentials are managed per thread, user space relies on
> some hack to synchronize credential update across threads from the same
> process.  This is required by the Native POSIX Threads Library and
> implemented by set*id(2) wrappers and libcap(3) to use tgkill(2) to
> synchronize threads.  See nptl(7) and libpsx(3).  Furthermore, some
> runtimes like Go do not enable developers to have control over threads
> [1].
> 
> To avoid potential issues, and because threads are not security
> boundaries, let's relax the Landlock signal scoping to always allow
> signals sent between threads of the same process.  This exception is
> similar to the __ptrace_may_access() one.
> 
> hook_file_set_fowner() now checks if the target task is part of the same
> process as the caller.  If this is the case, then the related signal
> triggered by the socket will always be allowed.

Seems fine,
Acked-by: Christian Brauner <brauner@xxxxxxxxxx>

> 
> Scoping of abstract UNIX sockets is not changed because kernel objects
> (e.g. sockets) should be tied to their creator's domain at creation
> time.
> 
> Note that creating one Landlock domain per thread puts each of these
> threads (and their future children) in their own scope, which is
> probably not what users expect, especially in Go where we do not control
> threads.  However, being able to drop permissions on all threads should
> not be restricted by signal scoping.  We are working on a way to make it
> possible to atomically restrict all threads of a process with the same
> domain [2].
> 
> Closes: https://github.com/landlock-lsm/go-landlock/issues/36
> Fixes: 54a6e6bbf3be ("landlock: Add signal scoping")
> Fixes: c8994965013e ("selftests/landlock: Test signal scoping for threads")
> Depends-on: 26f204380a3c ("fs: Fix file_set_fowner LSM hook inconsistencies")
> Link: https://pkg.go.dev/kernel.org/pub/linux/libs/security/libcap/psx [1]
> Link: https://github.com/landlock-lsm/linux/issues/2 [2]
> Cc: Christian Brauner <brauner@xxxxxxxxxx>
> Cc: Günther Noack <gnoack@xxxxxxxxxx>
> Cc: Paul Moore <paul@xxxxxxxxxxxxxx>
> Cc: Serge Hallyn <serge@xxxxxxxxxx>
> Cc: Tahera Fahimi <fahimitahera@xxxxxxxxx>
> Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx>
> Link: https://lore.kernel.org/r/20250313145904.3238184-1-mic@xxxxxxxxxxx
> ---
> 
> I'm still not sure how we could reliably detect if the running kernel
> has this fix or not, especially in Go.
> ---
>  security/landlock/fs.c                        | 22 +++++++++++++++----
>  security/landlock/task.c                      | 12 ++++++++++
>  .../selftests/landlock/scoped_signal_test.c   |  2 +-
>  3 files changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 71b9dc331aae..47c862fe14e4 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -27,7 +27,9 @@
>  #include <linux/mount.h>
>  #include <linux/namei.h>
>  #include <linux/path.h>
> +#include <linux/pid.h>
>  #include <linux/rcupdate.h>
> +#include <linux/sched/signal.h>
>  #include <linux/spinlock.h>
>  #include <linux/stat.h>
>  #include <linux/types.h>
> @@ -1630,15 +1632,27 @@ static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
>  
>  static void hook_file_set_fowner(struct file *file)
>  {
> -	struct landlock_ruleset *new_dom, *prev_dom;
> +	struct fown_struct *fown = file_f_owner(file);
> +	struct landlock_ruleset *new_dom = NULL;
> +	struct landlock_ruleset *prev_dom;
> +	struct task_struct *p;
>  
>  	/*
>  	 * Lock already held by __f_setown(), see commit 26f204380a3c ("fs: Fix
>  	 * file_set_fowner LSM hook inconsistencies").
>  	 */
> -	lockdep_assert_held(&file_f_owner(file)->lock);
> -	new_dom = landlock_get_current_domain();
> -	landlock_get_ruleset(new_dom);
> +	lockdep_assert_held(&fown->lock);
> +
> +	/*
> +	 * Always allow sending signals between threads of the same process.  This
> +	 * ensures consistency with hook_task_kill().
> +	 */
> +	p = pid_task(fown->pid, fown->pid_type);
> +	if (!same_thread_group(p, current)) {
> +		new_dom = landlock_get_current_domain();
> +		landlock_get_ruleset(new_dom);
> +	}
> +
>  	prev_dom = landlock_file(file)->fown_domain;
>  	landlock_file(file)->fown_domain = new_dom;
>  
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index dc7dab78392e..4578ce6e319d 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 <linux/sched/signal.h>
>  #include <net/af_unix.h>
>  #include <net/sock.h>
>  
> @@ -264,6 +265,17 @@ static int hook_task_kill(struct task_struct *const p,
>  		/* Dealing with USB IO. */
>  		dom = landlock_cred(cred)->domain;
>  	} else {
> +		/*
> +		 * Always allow sending signals between threads of the same process.
> +		 * This is required for process credential changes by the Native POSIX
> +		 * Threads Library and implemented by the set*id(2) wrappers and
> +		 * libcap(3) with tgkill(2).  See nptl(7) and libpsx(3).
> +		 *
> +		 * This exception is similar to the __ptrace_may_access() one.
> +		 */
> +		if (same_thread_group(p, current))
> +			return 0;
> +
>  		dom = landlock_get_current_domain();
>  	}
>  	dom = landlock_get_applicable_domain(dom, signal_scope);
> diff --git a/tools/testing/selftests/landlock/scoped_signal_test.c b/tools/testing/selftests/landlock/scoped_signal_test.c
> index 475ee62a832d..767f117703b7 100644
> --- a/tools/testing/selftests/landlock/scoped_signal_test.c
> +++ b/tools/testing/selftests/landlock/scoped_signal_test.c
> @@ -281,7 +281,7 @@ TEST(signal_scoping_threads)
>  	/* Restricts the domain after creating the first thread. */
>  	create_scoped_domain(_metadata, LANDLOCK_SCOPE_SIGNAL);
>  
> -	ASSERT_EQ(EPERM, pthread_kill(no_sandbox_thread, 0));
> +	ASSERT_EQ(0, pthread_kill(no_sandbox_thread, 0));
>  	ASSERT_EQ(1, write(thread_pipe[1], ".", 1));
>  
>  	ASSERT_EQ(0, pthread_create(&scoped_thread, NULL, thread_func, NULL));
> 
> base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6
> -- 
> 2.48.1
> 




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux