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 >
