[PATCH v3 0/6] Landlock: Signal Scoping Support

[PATCH v3 0/6] Landlock: Signal Scoping Support

[Tahera Fahimi]

This patch series adds scoping mechanism for signals.
Closes: https://github.com/landlock-lsm/linux/issues/8

Problem
=======

A sandboxed process is currently not restricted from sending signals
(e.g. SIGKILL) to processes outside the sandbox since Landlock has
no restriction on signals(see more details in [1]).

A simple way to apply this restriction would be to scope signals the
same way abstract unix sockets are restricted.

[1]https://lore.kernel.org/all/20231023.ahphah4Wii4v@digikod.net/

Solution
========

To solve this issue, we extend the "scoped" field in the Landlock
ruleset attribute structure by introducing "LANDLOCK_SCOPED_SIGNAL"
field to specify that a ruleset will deny sending any signals from
within the sandbox domain to its parent(i.e. any parent sandbox or
non-sandbox processes).

Example
=======

Create a sansboxed shell and pass the character "s" to LL_SCOPED:
LL_FD_RO=/ LL_FS_RW=. LL_SCOPED="s" ./sandboxer /bin/bash
Try to send a signal(like SIGTRAP) to a process ID <PID> through:
kill -SIGTRAP <PID>
The sandboxed process should not be able to send the signal.

Previous Versions
=================
v2:https://lore.kernel.org/all/cover.1722966592.git.fahimitahera@gmail.com/
v1:https://lore.kernel.org/all/cover.1720203255.git.fahimitahera@gmail.com/

Tahera Fahimi (6):
  Landlock: Add signal control
  Landlock: Adding file_send_sigiotask signal scoping support
  selftest/Landlock: Signal restriction tests
  selftest/Landlock: pthread_kill(3) tests
  sample/Landlock: Support signal scoping restriction
  Landlock: Document LANDLOCK_SCOPED_SIGNAL

 Documentation/userspace-api/landlock.rst      |  25 +-
 include/uapi/linux/landlock.h                 |   3 +
 samples/landlock/sandboxer.c                  |  16 +-
 security/landlock/fs.c                        |  18 +
 security/landlock/fs.h                        |   6 +
 security/landlock/limits.h                    |   2 +-
 security/landlock/task.c                      |  54 +++
 .../selftests/landlock/scoped_signal_test.c   | 331 ++++++++++++++++++
 8 files changed, 443 insertions(+), 12 deletions(-)
 create mode 100644 tools/testing/selftests/landlock/scoped_signal_test.c

-- 
2.34.1

[PATCH v3 1/6] Landlock: Add signal control

[Tahera Fahimi]

Currently, a sandbox process is not restricted to send a signal
(e.g. SIGKILL) to a process outside of the sandbox environment.
Ability to sending a signal for a sandboxed process should be
scoped the same way abstract unix sockets are scoped. Therefore,
we extend "scoped" field in a ruleset with
"LANDLOCK_SCOPED_SIGNAL" to specify that a ruleset will deny
sending any signal from within a sandbox process to its
parent(i.e. any parent sandbox or non-sandboxed procsses).

Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
---
Changes in versions:
V3:
* Moving file_send_sigiotask to another patch.
* Minor code refactoring.
V2:
* Remove signal_is_scoped function
* Applying reviews of V1
V1:
* Introducing LANDLOCK_SCOPE_SIGNAL
* Adding two hooks, hook_task_kill and hook_file_send_sigiotask
  for signal scoping.
---
 include/uapi/linux/landlock.h |  3 +++
 security/landlock/limits.h    |  2 +-
 security/landlock/task.c      | 27 +++++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 057a4811ed06..46301b47f502 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -289,8 +289,11 @@ struct landlock_net_port_attr {
  *   from connecting to an abstract unix socket created by a process
  *   outside the related Landlock domain (e.g. a parent domain or a
  *   non-sandboxed process).
+ * - %LANDLOCK_SCOPED_SIGNAL: Restrict a sandboxed process from sending a signal
+ *   to another process outside sandbox domain.
  */
 /* clang-format off */
 #define LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET		(1ULL << 0)
+#define LANDLOCK_SCOPED_SIGNAL		                (1ULL << 1)
 /* clang-format on*/
 #endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index eb01d0fb2165..fa28f9236407 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -26,7 +26,7 @@
 #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
 
-#define LANDLOCK_LAST_SCOPE		LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET
+#define LANDLOCK_LAST_SCOPE		LANDLOCK_SCOPED_SIGNAL
 #define LANDLOCK_MASK_SCOPE		((LANDLOCK_LAST_SCOPE << 1) - 1)
 #define LANDLOCK_NUM_SCOPE		__const_hweight64(LANDLOCK_MASK_SCOPE)
 /* clang-format on */
diff --git a/security/landlock/task.c b/security/landlock/task.c
index a461923c0571..9de96a5005c4 100644
--- a/security/landlock/task.c
+++ b/security/landlock/task.c
@@ -235,11 +235,38 @@ static int hook_unix_may_send(struct socket *const sock,
 	return 0;
 }
 
+static int hook_task_kill(struct task_struct *const p,
+			  struct kernel_siginfo *const info, const int sig,
+			  const struct cred *const cred)
+{
+	bool is_scoped;
+	const struct landlock_ruleset *target_dom, *dom;
+
+	dom = landlock_get_current_domain();
+	rcu_read_lock();
+	target_dom = landlock_get_task_domain(p);
+	if (cred)
+		/* dealing with USB IO */
+		is_scoped = domain_is_scoped(landlock_cred(cred)->domain,
+					     target_dom,
+					     LANDLOCK_SCOPED_SIGNAL);
+	else
+		is_scoped = (!dom) ? false :
+				     domain_is_scoped(dom, target_dom,
+						      LANDLOCK_SCOPED_SIGNAL);
+	rcu_read_unlock();
+	if (is_scoped)
+		return -EPERM;
+
+	return 0;
+}
+
 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),
+	LSM_HOOK_INIT(task_kill, hook_task_kill),
 };
 
 __init void landlock_add_task_hooks(void)
-- 
2.34.1

[PATCH v3 2/6] Landlock: Adding file_send_sigiotask signal scoping support

[Tahera Fahimi]

This patch adds two new hooks "hook_file_set_fowner" and
"hook_file_free_security" to set and release a pointer to the
domain of the file owner. This pointer "fown_domain" in
"landlock_file_security" will be used in "file_send_sigiotask"
to check if the process can send a signal.

Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
---
 security/landlock/fs.c   | 18 ++++++++++++++++++
 security/landlock/fs.h   |  6 ++++++
 security/landlock/task.c | 27 +++++++++++++++++++++++++++
 3 files changed, 51 insertions(+)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 7877a64cc6b8..d05f0e9c5e54 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1636,6 +1636,21 @@ static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
 	return -EACCES;
 }
 
+static void hook_file_set_fowner(struct file *file)
+{
+	write_lock_irq(&file->f_owner.lock);
+	landlock_file(file)->fown_domain = landlock_get_current_domain();
+	landlock_get_ruleset(landlock_file(file)->fown_domain);
+	write_unlock_irq(&file->f_owner.lock);
+}
+
+static void hook_file_free_security(struct file *file)
+{
+	write_lock_irq(&file->f_owner.lock);
+	landlock_put_ruleset(landlock_file(file)->fown_domain);
+	write_unlock_irq(&file->f_owner.lock);
+}
+
 static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
 
@@ -1660,6 +1675,9 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(file_truncate, hook_file_truncate),
 	LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
 	LSM_HOOK_INIT(file_ioctl_compat, hook_file_ioctl_compat),
+
+	LSM_HOOK_INIT(file_set_fowner, hook_file_set_fowner),
+	LSM_HOOK_INIT(file_free_security, hook_file_free_security),
 };
 
 __init void landlock_add_fs_hooks(void)
diff --git a/security/landlock/fs.h b/security/landlock/fs.h
index 488e4813680a..6054563295d8 100644
--- a/security/landlock/fs.h
+++ b/security/landlock/fs.h
@@ -52,6 +52,12 @@ struct landlock_file_security {
 	 * needed to authorize later operations on the open file.
 	 */
 	access_mask_t allowed_access;
+	/**
+	 * @fown_domain: A pointer to a &landlock_ruleset of the process own
+	 * the file. This ruleset is protected by fowner_struct.lock same as
+	 * pid, uid, euid fields in fown_struct.
+	 */
+	struct landlock_ruleset *fown_domain;
 };
 
 /**
diff --git a/security/landlock/task.c b/security/landlock/task.c
index 9de96a5005c4..568292dbfe7d 100644
--- a/security/landlock/task.c
+++ b/security/landlock/task.c
@@ -18,6 +18,7 @@
 
 #include "common.h"
 #include "cred.h"
+#include "fs.h"
 #include "ruleset.h"
 #include "setup.h"
 #include "task.h"
@@ -261,12 +262,38 @@ static int hook_task_kill(struct task_struct *const p,
 	return 0;
 }
 
+static int hook_file_send_sigiotask(struct task_struct *tsk,
+				    struct fown_struct *fown, int signum)
+{
+	struct file *file;
+	bool is_scoped;
+	const struct landlock_ruleset *dom, *target_dom;
+
+	/* struct fown_struct is never outside the context of a struct file */
+	file = container_of(fown, struct file, f_owner);
+
+	read_lock_irq(&file->f_owner.lock);
+	dom = landlock_file(file)->fown_domain;
+	read_unlock_irq(&file->f_owner.lock);
+	if (!dom)
+		return 0;
+
+	rcu_read_lock();
+	target_dom = landlock_get_task_domain(tsk);
+	is_scoped = domain_is_scoped(dom, target_dom, LANDLOCK_SCOPED_SIGNAL);
+	rcu_read_unlock();
+	if (is_scoped)
+		return -EPERM;
+	return 0;
+}
+
 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),
 	LSM_HOOK_INIT(task_kill, hook_task_kill),
+	LSM_HOOK_INIT(file_send_sigiotask, hook_file_send_sigiotask),
 };
 
 __init void landlock_add_task_hooks(void)
-- 
2.34.1

Re: [PATCH v3 2/6] Landlock: Adding file_send_sigiotask signal scoping support

[Jann Horn]

On Thu, Aug 15, 2024 at 8:29 PM Tahera Fahimi <fahimitahera@gmail.com> wrote:
> This patch adds two new hooks "hook_file_set_fowner" and
> "hook_file_free_security" to set and release a pointer to the
> domain of the file owner. This pointer "fown_domain" in
> "landlock_file_security" will be used in "file_send_sigiotask"
> to check if the process can send a signal.
>
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> ---
>  security/landlock/fs.c   | 18 ++++++++++++++++++
>  security/landlock/fs.h   |  6 ++++++
>  security/landlock/task.c | 27 +++++++++++++++++++++++++++
>  3 files changed, 51 insertions(+)
>
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 7877a64cc6b8..d05f0e9c5e54 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -1636,6 +1636,21 @@ static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
>         return -EACCES;
>  }
>
> +static void hook_file_set_fowner(struct file *file)
> +{
> +       write_lock_irq(&file->f_owner.lock);

Before updating landlock_file(file)->fown_domain, this hook must also
drop a reference on the old domain - maybe by just calling
landlock_put_ruleset_deferred(landlock_file(file)->fown_domain) here.

> +       landlock_file(file)->fown_domain = landlock_get_current_domain();
> +       landlock_get_ruleset(landlock_file(file)->fown_domain);
> +       write_unlock_irq(&file->f_owner.lock);
> +}
> +
> +static void hook_file_free_security(struct file *file)
> +{
> +       write_lock_irq(&file->f_owner.lock);
> +       landlock_put_ruleset(landlock_file(file)->fown_domain);
> +       write_unlock_irq(&file->f_owner.lock);
> +}
> +
>  static struct security_hook_list landlock_hooks[] __ro_after_init = {
>         LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
>
> @@ -1660,6 +1675,9 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
>         LSM_HOOK_INIT(file_truncate, hook_file_truncate),
>         LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
>         LSM_HOOK_INIT(file_ioctl_compat, hook_file_ioctl_compat),
> +
> +       LSM_HOOK_INIT(file_set_fowner, hook_file_set_fowner),
> +       LSM_HOOK_INIT(file_free_security, hook_file_free_security),
>  };
>
>  __init void landlock_add_fs_hooks(void)
> diff --git a/security/landlock/fs.h b/security/landlock/fs.h
> index 488e4813680a..6054563295d8 100644
> --- a/security/landlock/fs.h
> +++ b/security/landlock/fs.h
> @@ -52,6 +52,12 @@ struct landlock_file_security {
>          * needed to authorize later operations on the open file.
>          */
>         access_mask_t allowed_access;
> +       /**
> +        * @fown_domain: A pointer to a &landlock_ruleset of the process own
> +        * the file. This ruleset is protected by fowner_struct.lock same as
> +        * pid, uid, euid fields in fown_struct.
> +        */
> +       struct landlock_ruleset *fown_domain;
>  };
>
>  /**
> diff --git a/security/landlock/task.c b/security/landlock/task.c
> index 9de96a5005c4..568292dbfe7d 100644
> --- a/security/landlock/task.c
> +++ b/security/landlock/task.c
> @@ -18,6 +18,7 @@
>
>  #include "common.h"
>  #include "cred.h"
> +#include "fs.h"
>  #include "ruleset.h"
>  #include "setup.h"
>  #include "task.h"
> @@ -261,12 +262,38 @@ static int hook_task_kill(struct task_struct *const p,
>         return 0;
>  }
>
> +static int hook_file_send_sigiotask(struct task_struct *tsk,
> +                                   struct fown_struct *fown, int signum)
> +{
> +       struct file *file;
> +       bool is_scoped;
> +       const struct landlock_ruleset *dom, *target_dom;
> +
> +       /* struct fown_struct is never outside the context of a struct file */
> +       file = container_of(fown, struct file, f_owner);
> +
> +       read_lock_irq(&file->f_owner.lock);
> +       dom = landlock_file(file)->fown_domain;
> +       read_unlock_irq(&file->f_owner.lock);

At this point, the ->fown_domain pointer could concurrently change,
and (once you apply my suggestion above) the old ->fown_domain could
therefore be freed concurrently. One way to avoid that would be to use
landlock_get_ruleset() to grab a reference before calling
read_unlock_irq(), and drop that reference with
landlock_put_ruleset_deferred() before exiting from this function.

> +       if (!dom)
> +               return 0;
> +
> +       rcu_read_lock();
> +       target_dom = landlock_get_task_domain(tsk);
> +       is_scoped = domain_is_scoped(dom, target_dom, LANDLOCK_SCOPED_SIGNAL);
> +       rcu_read_unlock();
> +       if (is_scoped)
> +               return -EPERM;
> +       return 0;
> +}
> +
>  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),
>         LSM_HOOK_INIT(task_kill, hook_task_kill),
> +       LSM_HOOK_INIT(file_send_sigiotask, hook_file_send_sigiotask),
>  };
>
>  __init void landlock_add_task_hooks(void)
> --
> 2.34.1
>

Re: [PATCH v3 2/6] Landlock: Adding file_send_sigiotask signal scoping support

[Tahera Fahimi]

On Thu, Aug 15, 2024 at 10:25:15PM +0200, Jann Horn wrote:
> On Thu, Aug 15, 2024 at 8:29 PM Tahera Fahimi <fahimitahera@gmail.com> wrote:
> > This patch adds two new hooks "hook_file_set_fowner" and
> > "hook_file_free_security" to set and release a pointer to the
> > domain of the file owner. This pointer "fown_domain" in
> > "landlock_file_security" will be used in "file_send_sigiotask"
> > to check if the process can send a signal.
> >
> > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > ---
> >  security/landlock/fs.c   | 18 ++++++++++++++++++
> >  security/landlock/fs.h   |  6 ++++++
> >  security/landlock/task.c | 27 +++++++++++++++++++++++++++
> >  3 files changed, 51 insertions(+)
> >
> > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > index 7877a64cc6b8..d05f0e9c5e54 100644
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> > @@ -1636,6 +1636,21 @@ static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
> >         return -EACCES;
> >  }
> >
> > +static void hook_file_set_fowner(struct file *file)
> > +{
> > +       write_lock_irq(&file->f_owner.lock);
> 
> Before updating landlock_file(file)->fown_domain, this hook must also
> drop a reference on the old domain - maybe by just calling
> landlock_put_ruleset_deferred(landlock_file(file)->fown_domain) here.
Hi Jann,

Thanks for the feedback :)
It totally make sense.
> > +       landlock_file(file)->fown_domain = landlock_get_current_domain();
> > +       landlock_get_ruleset(landlock_file(file)->fown_domain);
> > +       write_unlock_irq(&file->f_owner.lock);
> > +}
> > +
> > +static void hook_file_free_security(struct file *file)
> > +{
> > +       write_lock_irq(&file->f_owner.lock);
> > +       landlock_put_ruleset(landlock_file(file)->fown_domain);
I was thinking of if we can replace this landlock_put_ruleset with
landlock_put_ruleset_deferred. In this case, it would be better use of
handling the lock?

> > +       write_unlock_irq(&file->f_owner.lock);
> > +}
> > +
> >  static struct security_hook_list landlock_hooks[] __ro_after_init = {
> >         LSM_HOOK_INIT(inode_free_security, hook_inode_free_security),
> >
> > @@ -1660,6 +1675,9 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
> >         LSM_HOOK_INIT(file_truncate, hook_file_truncate),
> >         LSM_HOOK_INIT(file_ioctl, hook_file_ioctl),
> >         LSM_HOOK_INIT(file_ioctl_compat, hook_file_ioctl_compat),
> > +
> > +       LSM_HOOK_INIT(file_set_fowner, hook_file_set_fowner),
> > +       LSM_HOOK_INIT(file_free_security, hook_file_free_security),
> >  };
> >
> >  __init void landlock_add_fs_hooks(void)
> > diff --git a/security/landlock/fs.h b/security/landlock/fs.h
> > index 488e4813680a..6054563295d8 100644
> > --- a/security/landlock/fs.h
> > +++ b/security/landlock/fs.h
> > @@ -52,6 +52,12 @@ struct landlock_file_security {
> >          * needed to authorize later operations on the open file.
> >          */
> >         access_mask_t allowed_access;
> > +       /**
> > +        * @fown_domain: A pointer to a &landlock_ruleset of the process own
> > +        * the file. This ruleset is protected by fowner_struct.lock same as
> > +        * pid, uid, euid fields in fown_struct.
> > +        */
> > +       struct landlock_ruleset *fown_domain;
> >  };
> >
> >  /**
> > diff --git a/security/landlock/task.c b/security/landlock/task.c
> > index 9de96a5005c4..568292dbfe7d 100644
> > --- a/security/landlock/task.c
> > +++ b/security/landlock/task.c
> > @@ -18,6 +18,7 @@
> >
> >  #include "common.h"
> >  #include "cred.h"
> > +#include "fs.h"
> >  #include "ruleset.h"
> >  #include "setup.h"
> >  #include "task.h"
> > @@ -261,12 +262,38 @@ static int hook_task_kill(struct task_struct *const p,
> >         return 0;
> >  }
> >
> > +static int hook_file_send_sigiotask(struct task_struct *tsk,
> > +                                   struct fown_struct *fown, int signum)
> > +{
> > +       struct file *file;
> > +       bool is_scoped;
> > +       const struct landlock_ruleset *dom, *target_dom;
> > +
> > +       /* struct fown_struct is never outside the context of a struct file */
> > +       file = container_of(fown, struct file, f_owner);
> > +
> > +       read_lock_irq(&file->f_owner.lock);
> > +       dom = landlock_file(file)->fown_domain;
> > +       read_unlock_irq(&file->f_owner.lock);
> 
> At this point, the ->fown_domain pointer could concurrently change,
> and (once you apply my suggestion above) the old ->fown_domain could
> therefore be freed concurrently. One way to avoid that would be to use
> landlock_get_ruleset() to grab a reference before calling
> read_unlock_irq(), and drop that reference with
> landlock_put_ruleset_deferred() before exiting from this function.
Correct, I applied the changes. 
> > +       if (!dom)
> > +               return 0;
> > +
> > +       rcu_read_lock();
> > +       target_dom = landlock_get_task_domain(tsk);
> > +       is_scoped = domain_is_scoped(dom, target_dom, LANDLOCK_SCOPED_SIGNAL);
> > +       rcu_read_unlock();
> > +       if (is_scoped)
> > +               return -EPERM;
> > +       return 0;
> > +}
> > +
> >  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),
> >         LSM_HOOK_INIT(task_kill, hook_task_kill),
> > +       LSM_HOOK_INIT(file_send_sigiotask, hook_file_send_sigiotask),
> >  };
> >
> >  __init void landlock_add_task_hooks(void)
> > --
> > 2.34.1
> >

Re: [PATCH v3 2/6] Landlock: Adding file_send_sigiotask signal scoping support

[Jann Horn]

On Thu, Aug 15, 2024 at 11:28 PM Tahera Fahimi <fahimitahera@gmail.com> wrote:
>
> On Thu, Aug 15, 2024 at 10:25:15PM +0200, Jann Horn wrote:
> > On Thu, Aug 15, 2024 at 8:29 PM Tahera Fahimi <fahimitahera@gmail.com> wrote:
> > > This patch adds two new hooks "hook_file_set_fowner" and
> > > "hook_file_free_security" to set and release a pointer to the
> > > domain of the file owner. This pointer "fown_domain" in
> > > "landlock_file_security" will be used in "file_send_sigiotask"
> > > to check if the process can send a signal.
> > >
> > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > > ---
> > >  security/landlock/fs.c   | 18 ++++++++++++++++++
> > >  security/landlock/fs.h   |  6 ++++++
> > >  security/landlock/task.c | 27 +++++++++++++++++++++++++++
> > >  3 files changed, 51 insertions(+)
> > >
> > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > > index 7877a64cc6b8..d05f0e9c5e54 100644
> > > --- a/security/landlock/fs.c
> > > +++ b/security/landlock/fs.c
> > > @@ -1636,6 +1636,21 @@ static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
> > >         return -EACCES;
> > >  }
> > >
> > > +static void hook_file_set_fowner(struct file *file)
> > > +{
> > > +       write_lock_irq(&file->f_owner.lock);
> >
> > Before updating landlock_file(file)->fown_domain, this hook must also
> > drop a reference on the old domain - maybe by just calling
> > landlock_put_ruleset_deferred(landlock_file(file)->fown_domain) here.
> Hi Jann,
>
> Thanks for the feedback :)
> It totally make sense.
> > > +       landlock_file(file)->fown_domain = landlock_get_current_domain();
> > > +       landlock_get_ruleset(landlock_file(file)->fown_domain);
> > > +       write_unlock_irq(&file->f_owner.lock);
> > > +}
> > > +
> > > +static void hook_file_free_security(struct file *file)
> > > +{
> > > +       write_lock_irq(&file->f_owner.lock);
> > > +       landlock_put_ruleset(landlock_file(file)->fown_domain);
> I was thinking of if we can replace this landlock_put_ruleset with
> landlock_put_ruleset_deferred. In this case, it would be better use of
> handling the lock?

I don't think you have to take the "file->f_owner.lock" in this hook -
the file has already been torn down pretty far, nothing is going to be
able to trigger the file_set_fowner hook anymore.

But either way, you're right that we can't just use
landlock_put_ruleset() here because landlock_put_ruleset() can sleep
and the file_free_security hook can be invoked from non-sleepable
context. (This only happens when fput() directly calls file_free(),
and I think that only happens with ->fown_domain==NULL, so technically
it would also be fine to do something like "if (domain)
landlock_put_ruleset(domain);".)
If you test your current code in a kernel that was built with
CONFIG_DEBUG_ATOMIC_SLEEP=y, this will probably print an warning
message in the kernel log (dmesg). You're right that using
landlock_put_ruleset_deferred() instead would fix that.

I think the right solution here is probably just to do:

static void hook_file_free_security(struct file *file)
{
  landlock_put_ruleset_deferred(landlock_file(file)->fown_domain);
}

Alternatively it would also work to do this - this code is probably a
bit more efficient but also a little less clear:

static void hook_file_free_security(struct file *file)
{
  /* don't trigger might_sleep() for tearing down unopened file */
  if (landlock_file(file)->fown_domain)
    landlock_put_ruleset(landlock_file(file)->fown_domain);
}

>
> > > +       write_unlock_irq(&file->f_owner.lock);
> > > +}

Re: [PATCH v3 2/6] Landlock: Adding file_send_sigiotask signal scoping support

[Tahera Fahimi]

On Fri, Aug 16, 2024 at 12:10:44AM +0200, Jann Horn wrote:
> On Thu, Aug 15, 2024 at 11:28 PM Tahera Fahimi <fahimitahera@gmail.com> wrote:
> >
> > On Thu, Aug 15, 2024 at 10:25:15PM +0200, Jann Horn wrote:
> > > On Thu, Aug 15, 2024 at 8:29 PM Tahera Fahimi <fahimitahera@gmail.com> wrote:
> > > > This patch adds two new hooks "hook_file_set_fowner" and
> > > > "hook_file_free_security" to set and release a pointer to the
> > > > domain of the file owner. This pointer "fown_domain" in
> > > > "landlock_file_security" will be used in "file_send_sigiotask"
> > > > to check if the process can send a signal.
> > > >
> > > > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > > > ---
> > > >  security/landlock/fs.c   | 18 ++++++++++++++++++
> > > >  security/landlock/fs.h   |  6 ++++++
> > > >  security/landlock/task.c | 27 +++++++++++++++++++++++++++
> > > >  3 files changed, 51 insertions(+)
> > > >
> > > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > > > index 7877a64cc6b8..d05f0e9c5e54 100644
> > > > --- a/security/landlock/fs.c
> > > > +++ b/security/landlock/fs.c
> > > > @@ -1636,6 +1636,21 @@ static int hook_file_ioctl_compat(struct file *file, unsigned int cmd,
> > > >         return -EACCES;
> > > >  }
> > > >
> > > > +static void hook_file_set_fowner(struct file *file)
> > > > +{
> > > > +       write_lock_irq(&file->f_owner.lock);
> > >
> > > Before updating landlock_file(file)->fown_domain, this hook must also
> > > drop a reference on the old domain - maybe by just calling
> > > landlock_put_ruleset_deferred(landlock_file(file)->fown_domain) here.
> > Hi Jann,
> >
> > Thanks for the feedback :)
> > It totally make sense.
> > > > +       landlock_file(file)->fown_domain = landlock_get_current_domain();
> > > > +       landlock_get_ruleset(landlock_file(file)->fown_domain);
> > > > +       write_unlock_irq(&file->f_owner.lock);
> > > > +}
> > > > +
> > > > +static void hook_file_free_security(struct file *file)
> > > > +{
> > > > +       write_lock_irq(&file->f_owner.lock);
> > > > +       landlock_put_ruleset(landlock_file(file)->fown_domain);
> > I was thinking of if we can replace this landlock_put_ruleset with
> > landlock_put_ruleset_deferred. In this case, it would be better use of
> > handling the lock?
> 
> I don't think you have to take the "file->f_owner.lock" in this hook -
> the file has already been torn down pretty far, nothing is going to be
> able to trigger the file_set_fowner hook anymore.
That's right. Thanks.

> But either way, you're right that we can't just use
> landlock_put_ruleset() here because landlock_put_ruleset() can sleep
> and the file_free_security hook can be invoked from non-sleepable
> context. (This only happens when fput() directly calls file_free(),
> and I think that only happens with ->fown_domain==NULL, so technically
> it would also be fine to do something like "if (domain)
> landlock_put_ruleset(domain);".)
> If you test your current code in a kernel that was built with
> CONFIG_DEBUG_ATOMIC_SLEEP=y, this will probably print an warning
> message in the kernel log (dmesg). You're right that using
> landlock_put_ruleset_deferred() instead would fix that.
> 
> I think the right solution here is probably just to do:
> 
> static void hook_file_free_security(struct file *file)
> {
>   landlock_put_ruleset_deferred(landlock_file(file)->fown_domain);
> }
I think I will stick to this one since it is easier to understand. 
> Alternatively it would also work to do this - this code is probably a
> bit more efficient but also a little less clear:
> 
> static void hook_file_free_security(struct file *file)
> {
>   /* don't trigger might_sleep() for tearing down unopened file */
>   if (landlock_file(file)->fown_domain)
>     landlock_put_ruleset(landlock_file(file)->fown_domain);
> }
> 
> >
> > > > +       write_unlock_irq(&file->f_owner.lock);
> > > > +}

Re: [PATCH v3 2/6] Landlock: Adding file_send_sigiotask signal scoping support

[Mickaël Salaün]

On Thu, Aug 15, 2024 at 12:29:21PM -0600, Tahera Fahimi wrote:
> This patch adds two new hooks "hook_file_set_fowner" and
> "hook_file_free_security" to set and release a pointer to the
> domain of the file owner. This pointer "fown_domain" in
> "landlock_file_security" will be used in "file_send_sigiotask"
> to check if the process can send a signal.

We need to make sure this file_send_sigiotask hook is useful and can be
triggered by user space code.  Currently, all tests pass without this
patch.

Re: [PATCH v3 2/6] Landlock: Adding file_send_sigiotask signal scoping support

[Mickaël Salaün]

On Thu, Aug 15, 2024 at 12:29:21PM -0600, Tahera Fahimi wrote:
> This patch adds two new hooks "hook_file_set_fowner" and
> "hook_file_free_security" to set and release a pointer to the
> domain of the file owner. This pointer "fown_domain" in
> "landlock_file_security" will be used in "file_send_sigiotask"
> to check if the process can send a signal.
> 
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> ---
>  security/landlock/fs.c   | 18 ++++++++++++++++++
>  security/landlock/fs.h   |  6 ++++++
>  security/landlock/task.c | 27 +++++++++++++++++++++++++++
>  3 files changed, 51 insertions(+)

Please squash this patch with the previous one, both are enforcing the
signal scoping restriction with LANDLOCK_SCOPED_SIGNAL.

You'll also need to update the scoped_test.c file with
LANDLOCK_SCOPED_SIGNAL (in this same squashed patch).

[PATCH v3 3/6] selftest/Landlock: Signal restriction tests

[Tahera Fahimi]

This patch expands Landlock ABI version 6 by providing tests for
signal scoping mechanism. Base on kill(2), if the signal is 0,
no signal will be sent, but the permission of a process to send
a signal will be checked. Likewise, this test consider one signal
for each signal category.

Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
---
Chnages in versions:
V2:
* Moving tests from ptrace_test.c to scoped_signal_test.c
* Remove debugging statements.
* Covering all basic restriction scenarios by sending 0 as signal
V1:
* Expanding Landlock ABI version 6 by providing basic tests for
  four signals to test signal scoping mechanism.
---
 .../selftests/landlock/scoped_signal_test.c   | 302 ++++++++++++++++++
 1 file changed, 302 insertions(+)
 create mode 100644 tools/testing/selftests/landlock/scoped_signal_test.c

diff --git a/tools/testing/selftests/landlock/scoped_signal_test.c b/tools/testing/selftests/landlock/scoped_signal_test.c
new file mode 100644
index 000000000000..92958c6266ca
--- /dev/null
+++ b/tools/testing/selftests/landlock/scoped_signal_test.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Landlock tests - Signal Scoping
+ *
+ * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
+ * Copyright © 2019-2020 ANSSI
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/landlock.h>
+#include <signal.h>
+#include <sys/prctl.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include "common.h"
+
+static sig_atomic_t signaled;
+
+static void create_signal_domain(struct __test_metadata *const _metadata)
+{
+	int ruleset_fd;
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.scoped = LANDLOCK_SCOPED_SIGNAL,
+	};
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	EXPECT_LE(0, ruleset_fd)
+	{
+		TH_LOG("Failed to create a ruleset: %s", strerror(errno));
+	}
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+}
+
+static void scope_signal_handler(int sig, siginfo_t *info, void *ucontext)
+{
+	if (sig == SIGHUP || sig == SIGURG || sig == SIGTSTP ||
+	    sig == SIGTRAP || sig == SIGUSR1) {
+		signaled = 1;
+	}
+}
+
+/* clang-format off */
+FIXTURE(signal_scoping) {};
+/* clang-format on */
+
+FIXTURE_VARIANT(signal_scoping)
+{
+	const int sig;
+	const bool domain_both;
+	const bool domain_parent;
+	const bool domain_child;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, allow_without_domain) {
+	/* clang-format on */
+	.sig = 0,
+	.domain_both = false,
+	.domain_parent = false,
+	.domain_child = false,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, deny_with_child_domain) {
+	/* clang-format on */
+	.sig = 0,
+	.domain_both = false,
+	.domain_parent = false,
+	.domain_child = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, allow_with_parent_domain) {
+	/* clang-format on */
+	.sig = 0,
+	.domain_both = false,
+	.domain_parent = true,
+	.domain_child = false,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, deny_with_sibling_domain) {
+	/* clang-format on */
+	.sig = 0,
+	.domain_both = false,
+	.domain_parent = true,
+	.domain_child = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, allow_sibling_domain) {
+	/* clang-format on */
+	.sig = 0,
+	.domain_both = true,
+	.domain_parent = false,
+	.domain_child = false,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, deny_with_nested_domain) {
+	/* clang-format on */
+	.sig = 0,
+	.domain_both = true,
+	.domain_parent = false,
+	.domain_child = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, allow_with_nested_and_parent_domain) {
+	/* clang-format on */
+	.sig = 0,
+	.domain_both = true,
+	.domain_parent = true,
+	.domain_child = false,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain) {
+	/* clang-format on */
+	.sig = 0,
+	.domain_both = true,
+	.domain_parent = true,
+	.domain_child = true,
+};
+
+/* Default Action: Terminate*/
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGHUP) {
+	/* clang-format on */
+	.sig = SIGHUP,
+	.domain_both = true,
+	.domain_parent = true,
+	.domain_child = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, allow_with_forked_domain_SIGHUP) {
+	/* clang-format on */
+	.sig = SIGHUP,
+	.domain_both = false,
+	.domain_parent = true,
+	.domain_child = false,
+};
+
+/* Default Action: Ignore*/
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGURG) {
+	/* clang-format on */
+	.sig = SIGURG,
+	.domain_both = true,
+	.domain_parent = true,
+	.domain_child = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, allow_with_forked_domain_SIGURG) {
+	/* clang-format on */
+	.sig = SIGURG,
+	.domain_both = false,
+	.domain_parent = true,
+	.domain_child = false,
+};
+
+/* Default Action: Stop*/
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGTSTP) {
+	/* clang-format on */
+	.sig = SIGTSTP,
+	.domain_both = true,
+	.domain_parent = true,
+	.domain_child = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, allow_with_forked_domain_SIGTSTP) {
+	/* clang-format on */
+	.sig = SIGTSTP,
+	.domain_both = false,
+	.domain_parent = true,
+	.domain_child = false,
+};
+
+/* Default Action: Coredump*/
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGTRAP) {
+	/* clang-format on */
+	.sig = SIGTRAP,
+	.domain_both = true,
+	.domain_parent = true,
+	.domain_child = true,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, allow_with_forked_domain_SIGTRAP) {
+	/* clang-format on */
+	.sig = SIGTRAP,
+	.domain_both = false,
+	.domain_parent = true,
+	.domain_child = false,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGUSR1) {
+	/* clang-format on */
+	.sig = SIGUSR1,
+	.domain_both = true,
+	.domain_parent = true,
+	.domain_child = true,
+};
+
+FIXTURE_SETUP(signal_scoping)
+{
+}
+
+FIXTURE_TEARDOWN(signal_scoping)
+{
+}
+
+TEST_F(signal_scoping, test_signal)
+{
+	pid_t child;
+	pid_t parent = getpid();
+	int status;
+	bool can_signal;
+	int pipe_parent[2];
+	struct sigaction action = {
+		.sa_sigaction = scope_signal_handler,
+		.sa_flags = SA_SIGINFO,
+
+	};
+
+	can_signal = !variant->domain_child;
+
+	if (variant->sig > 0)
+		ASSERT_LE(0, sigaction(variant->sig, &action, NULL));
+
+	if (variant->domain_both) {
+		create_signal_domain(_metadata);
+		if (!__test_passed(_metadata))
+			/* Aborts before forking. */
+			return;
+	}
+	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
+
+	child = fork();
+	ASSERT_LE(0, child);
+	if (child == 0) {
+		char buf_child;
+		int err;
+
+		ASSERT_EQ(0, close(pipe_parent[1]));
+		if (variant->domain_child)
+			create_signal_domain(_metadata);
+
+		/* Waits for the parent to be in a domain, if any. */
+		ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
+
+		err = kill(parent, variant->sig);
+		if (can_signal) {
+			ASSERT_EQ(0, err);
+		} else {
+			ASSERT_EQ(-1, err);
+			ASSERT_EQ(EPERM, errno);
+		}
+		/* no matter of the domain, a process should be able to send
+		 * a signal to itself.
+		 */
+		ASSERT_EQ(0, raise(variant->sig));
+		if (variant->sig > 0)
+			ASSERT_EQ(1, signaled);
+		_exit(_metadata->exit_code);
+		return;
+	}
+	ASSERT_EQ(0, close(pipe_parent[0]));
+	if (variant->domain_parent)
+		create_signal_domain(_metadata);
+
+	/* Signals that the parent is in a domain, if any. */
+	ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+
+	if (can_signal && variant->sig > 0) {
+		ASSERT_EQ(-1, pause());
+		ASSERT_EQ(EINTR, errno);
+		ASSERT_EQ(1, signaled);
+	} else {
+		ASSERT_EQ(0, signaled);
+	}
+
+	ASSERT_EQ(child, waitpid(child, &status, 0));
+
+	if (WIFSIGNALED(status) || !WIFEXITED(status) ||
+	    WEXITSTATUS(status) != EXIT_SUCCESS)
+		_metadata->exit_code = KSFT_FAIL;
+}
+
+TEST_HARNESS_MAIN
-- 
2.34.1

Re: [PATCH v3 3/6] selftest/Landlock: Signal restriction tests

[Mickaël Salaün]

On Thu, Aug 15, 2024 at 12:29:22PM -0600, Tahera Fahimi wrote:
> This patch expands Landlock ABI version 6 by providing tests for
> signal scoping mechanism. Base on kill(2), if the signal is 0,
> no signal will be sent, but the permission of a process to send
> a signal will be checked. Likewise, this test consider one signal
> for each signal category.
> 
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> ---
> Chnages in versions:
> V2:
> * Moving tests from ptrace_test.c to scoped_signal_test.c
> * Remove debugging statements.
> * Covering all basic restriction scenarios by sending 0 as signal
> V1:
> * Expanding Landlock ABI version 6 by providing basic tests for
>   four signals to test signal scoping mechanism.
> ---
>  .../selftests/landlock/scoped_signal_test.c   | 302 ++++++++++++++++++
>  1 file changed, 302 insertions(+)
>  create mode 100644 tools/testing/selftests/landlock/scoped_signal_test.c
> 
> diff --git a/tools/testing/selftests/landlock/scoped_signal_test.c b/tools/testing/selftests/landlock/scoped_signal_test.c
> new file mode 100644
> index 000000000000..92958c6266ca
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/scoped_signal_test.c
> @@ -0,0 +1,302 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Landlock tests - Signal Scoping
> + *
> + * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
> + * Copyright © 2019-2020 ANSSI
> + */
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/landlock.h>
> +#include <signal.h>
> +#include <sys/prctl.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include "common.h"
> +
> +static sig_atomic_t signaled;
> +
> +static void create_signal_domain(struct __test_metadata *const _metadata)

You can add a create_scoped_domain() helper in scoped_common.h with a
"scope" argument, and use it as well for the abstract unix socket tests.

> +{
> +	int ruleset_fd;
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.scoped = LANDLOCK_SCOPED_SIGNAL,
> +	};
> +
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	EXPECT_LE(0, ruleset_fd)

Again, please use ASSERT_* when it doesn't make sense to continue the
tests if something goes wrong.  In this case, if the domain creation
failed, something is very wrong and we should stop right away.
Everything that follow would not make any sense to test.

> +	{
> +		TH_LOG("Failed to create a ruleset: %s", strerror(errno));
> +	}
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	EXPECT_EQ(0, close(ruleset_fd));

Here, EXPECT is OK because not being able to close the FD would not
impact the rest of the tests.

> +}
> +
> +static void scope_signal_handler(int sig, siginfo_t *info, void *ucontext)
> +{
> +	if (sig == SIGHUP || sig == SIGURG || sig == SIGTSTP ||
> +	    sig == SIGTRAP || sig == SIGUSR1) {
> +		signaled = 1;
> +	}
> +}
> +
> +/* clang-format off */
> +FIXTURE(signal_scoping) {};
> +/* clang-format on */
> +
> +FIXTURE_VARIANT(signal_scoping)

You should include scoped_common.h which would declare these variants
(see my reviews for the abstract unix socket tests).

> +{
> +	const int sig;

"sig" should not be part of this variant, but a TEST_F() should be
created for each useful signal.  Instead of using
signal_scoping/scoped_domains, different signal only need to be tested
against one domain topology e.g., only a sandboxed/scoped child.

These variants should be used to test the same semantic as for the
abstract unix socket: TEST_F(scoped_domains, to_child) and
TEST_F(scoped_domains, to_parent)

> +	const bool domain_both;
> +	const bool domain_parent;
> +	const bool domain_child;
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_without_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = false,
> +	.domain_parent = false,
> +	.domain_child = false,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_child_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = false,
> +	.domain_parent = false,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_with_parent_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = false,
> +	.domain_parent = true,
> +	.domain_child = false,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_sibling_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = false,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_sibling_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = true,
> +	.domain_parent = false,
> +	.domain_child = false,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_nested_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = true,
> +	.domain_parent = false,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_with_nested_and_parent_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = false,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +/* Default Action: Terminate*/
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGHUP) {
> +	/* clang-format on */
> +	.sig = SIGHUP,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_with_forked_domain_SIGHUP) {
> +	/* clang-format on */
> +	.sig = SIGHUP,
> +	.domain_both = false,
> +	.domain_parent = true,
> +	.domain_child = false,
> +};
> +
> +/* Default Action: Ignore*/
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGURG) {
> +	/* clang-format on */
> +	.sig = SIGURG,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_with_forked_domain_SIGURG) {
> +	/* clang-format on */
> +	.sig = SIGURG,
> +	.domain_both = false,
> +	.domain_parent = true,
> +	.domain_child = false,
> +};
> +
> +/* Default Action: Stop*/
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGTSTP) {
> +	/* clang-format on */
> +	.sig = SIGTSTP,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_with_forked_domain_SIGTSTP) {
> +	/* clang-format on */
> +	.sig = SIGTSTP,
> +	.domain_both = false,
> +	.domain_parent = true,
> +	.domain_child = false,
> +};
> +
> +/* Default Action: Coredump*/
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGTRAP) {
> +	/* clang-format on */
> +	.sig = SIGTRAP,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_with_forked_domain_SIGTRAP) {
> +	/* clang-format on */
> +	.sig = SIGTRAP,
> +	.domain_both = false,
> +	.domain_parent = true,
> +	.domain_child = false,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGUSR1) {
> +	/* clang-format on */
> +	.sig = SIGUSR1,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +FIXTURE_SETUP(signal_scoping)
> +{
> +}
> +
> +FIXTURE_TEARDOWN(signal_scoping)
> +{
> +}
> +
> +TEST_F(signal_scoping, test_signal)

This test should only test one kind of signal (hence no signal
variants), but it should be splitted in two tests (i.e. to_child and
to_parent).  All the different signals should be tested with a dedicated
test with the same domain topology.

The code of this test should then be organized the same way and look
pretty similar to the other TEST_F(scoped_signal, to_child) from the
abstract unix socket test file.

> +{
> +	pid_t child;
> +	pid_t parent = getpid();
> +	int status;
> +	bool can_signal;
> +	int pipe_parent[2];
> +	struct sigaction action = {
> +		.sa_sigaction = scope_signal_handler,
> +		.sa_flags = SA_SIGINFO,
> +
> +	};
> +
> +	can_signal = !variant->domain_child;
> +
> +	if (variant->sig > 0)
> +		ASSERT_LE(0, sigaction(variant->sig, &action, NULL));
> +
> +	if (variant->domain_both) {
> +		create_signal_domain(_metadata);
> +		if (!__test_passed(_metadata))

We should not explicitly call __test_passed().  Just use ASSERT_* in
create_scoped_domain().

> +			/* Aborts before forking. */
> +			return;
> +	}
> +	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
> +
> +	child = fork();
> +	ASSERT_LE(0, child);
> +	if (child == 0) {
> +		char buf_child;
> +		int err;
> +
> +		ASSERT_EQ(0, close(pipe_parent[1]));
> +		if (variant->domain_child)
> +			create_signal_domain(_metadata);
> +
> +		/* Waits for the parent to be in a domain, if any. */
> +		ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> +
> +		err = kill(parent, variant->sig);
> +		if (can_signal) {
> +			ASSERT_EQ(0, err);
> +		} else {
> +			ASSERT_EQ(-1, err);
> +			ASSERT_EQ(EPERM, errno);
> +		}
> +		/* no matter of the domain, a process should be able to send

Please follow the comment formatting rules everywhere as I explained
before:

/*
 * No matter of the domain, a process should be able to send
 * a signal to itself.
 */

> +		 * a signal to itself.
> +		 */
> +		ASSERT_EQ(0, raise(variant->sig));
> +		if (variant->sig > 0)
> +			ASSERT_EQ(1, signaled);
> +		_exit(_metadata->exit_code);
> +		return;
> +	}
> +	ASSERT_EQ(0, close(pipe_parent[0]));
> +	if (variant->domain_parent)
> +		create_signal_domain(_metadata);
> +
> +	/* Signals that the parent is in a domain, if any. */
> +	ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> +
> +	if (can_signal && variant->sig > 0) {
> +		ASSERT_EQ(-1, pause());
> +		ASSERT_EQ(EINTR, errno);
> +		ASSERT_EQ(1, signaled);
> +	} else {
> +		ASSERT_EQ(0, signaled);
> +	}
> +
> +	ASSERT_EQ(child, waitpid(child, &status, 0));
> +
> +	if (WIFSIGNALED(status) || !WIFEXITED(status) ||
> +	    WEXITSTATUS(status) != EXIT_SUCCESS)
> +		_metadata->exit_code = KSFT_FAIL;
> +}
> +
> +TEST_HARNESS_MAIN
> -- 
> 2.34.1
> 
> 

Re: [PATCH v3 3/6] selftest/Landlock: Signal restriction tests

[Mickaël Salaün]

On Thu, Aug 15, 2024 at 12:29:22PM -0600, Tahera Fahimi wrote:
> This patch expands Landlock ABI version 6 by providing tests for
> signal scoping mechanism. Base on kill(2), if the signal is 0,
> no signal will be sent, but the permission of a process to send
> a signal will be checked. Likewise, this test consider one signal
> for each signal category.
> 
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> ---
> Chnages in versions:
> V2:
> * Moving tests from ptrace_test.c to scoped_signal_test.c
> * Remove debugging statements.
> * Covering all basic restriction scenarios by sending 0 as signal
> V1:
> * Expanding Landlock ABI version 6 by providing basic tests for
>   four signals to test signal scoping mechanism.
> ---
>  .../selftests/landlock/scoped_signal_test.c   | 302 ++++++++++++++++++
>  1 file changed, 302 insertions(+)
>  create mode 100644 tools/testing/selftests/landlock/scoped_signal_test.c
> 
> diff --git a/tools/testing/selftests/landlock/scoped_signal_test.c b/tools/testing/selftests/landlock/scoped_signal_test.c
> new file mode 100644
> index 000000000000..92958c6266ca
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/scoped_signal_test.c
> @@ -0,0 +1,302 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Landlock tests - Signal Scoping
> + *
> + * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
> + * Copyright © 2019-2020 ANSSI
> + */
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/landlock.h>
> +#include <signal.h>
> +#include <sys/prctl.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include "common.h"
> +
> +static sig_atomic_t signaled;

static volatile sig_atomic_t signaled;

> +
> +static void create_signal_domain(struct __test_metadata *const _metadata)
> +{
> +	int ruleset_fd;
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.scoped = LANDLOCK_SCOPED_SIGNAL,
> +	};
> +
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	EXPECT_LE(0, ruleset_fd)
> +	{
> +		TH_LOG("Failed to create a ruleset: %s", strerror(errno));
> +	}
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	EXPECT_EQ(0, close(ruleset_fd));
> +}
> +
> +static void scope_signal_handler(int sig, siginfo_t *info, void *ucontext)
> +{
> +	if (sig == SIGHUP || sig == SIGURG || sig == SIGTSTP ||
> +	    sig == SIGTRAP || sig == SIGUSR1) {
> +		signaled = 1;
> +	}
> +}
> +
> +/* clang-format off */
> +FIXTURE(signal_scoping) {};
> +/* clang-format on */
> +
> +FIXTURE_VARIANT(signal_scoping)
> +{
> +	const int sig;
> +	const bool domain_both;
> +	const bool domain_parent;
> +	const bool domain_child;
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_without_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = false,
> +	.domain_parent = false,
> +	.domain_child = false,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_child_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = false,
> +	.domain_parent = false,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_with_parent_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = false,
> +	.domain_parent = true,
> +	.domain_child = false,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_sibling_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = false,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_sibling_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = true,
> +	.domain_parent = false,
> +	.domain_child = false,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_nested_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = true,
> +	.domain_parent = false,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_with_nested_and_parent_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = false,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain) {
> +	/* clang-format on */
> +	.sig = 0,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +/* Default Action: Terminate*/
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGHUP) {
> +	/* clang-format on */
> +	.sig = SIGHUP,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_with_forked_domain_SIGHUP) {
> +	/* clang-format on */
> +	.sig = SIGHUP,
> +	.domain_both = false,
> +	.domain_parent = true,
> +	.domain_child = false,
> +};
> +
> +/* Default Action: Ignore*/
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGURG) {
> +	/* clang-format on */
> +	.sig = SIGURG,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_with_forked_domain_SIGURG) {
> +	/* clang-format on */
> +	.sig = SIGURG,
> +	.domain_both = false,
> +	.domain_parent = true,
> +	.domain_child = false,
> +};
> +
> +/* Default Action: Stop*/
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGTSTP) {
> +	/* clang-format on */
> +	.sig = SIGTSTP,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_with_forked_domain_SIGTSTP) {
> +	/* clang-format on */
> +	.sig = SIGTSTP,
> +	.domain_both = false,
> +	.domain_parent = true,
> +	.domain_child = false,
> +};
> +
> +/* Default Action: Coredump*/
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGTRAP) {
> +	/* clang-format on */
> +	.sig = SIGTRAP,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, allow_with_forked_domain_SIGTRAP) {
> +	/* clang-format on */
> +	.sig = SIGTRAP,
> +	.domain_both = false,
> +	.domain_parent = true,
> +	.domain_child = false,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain_SIGUSR1) {
> +	/* clang-format on */
> +	.sig = SIGUSR1,
> +	.domain_both = true,
> +	.domain_parent = true,
> +	.domain_child = true,
> +};
> +
> +FIXTURE_SETUP(signal_scoping)
> +{
> +}
> +
> +FIXTURE_TEARDOWN(signal_scoping)
> +{
> +}
> +
> +TEST_F(signal_scoping, test_signal)

Sometime, this test hang.  I suspect the following issue:

> +{
> +	pid_t child;
> +	pid_t parent = getpid();
> +	int status;
> +	bool can_signal;
> +	int pipe_parent[2];
> +	struct sigaction action = {
> +		.sa_sigaction = scope_signal_handler,
> +		.sa_flags = SA_SIGINFO,
> +
> +	};
> +
> +	can_signal = !variant->domain_child;
> +
> +	if (variant->sig > 0)
> +		ASSERT_LE(0, sigaction(variant->sig, &action, NULL));
> +
> +	if (variant->domain_both) {
> +		create_signal_domain(_metadata);
> +		if (!__test_passed(_metadata))
> +			/* Aborts before forking. */
> +			return;
> +	}
> +	ASSERT_EQ(0, pipe2(pipe_parent, O_CLOEXEC));
> +
> +	child = fork();
> +	ASSERT_LE(0, child);
> +	if (child == 0) {
> +		char buf_child;
> +		int err;
> +
> +		ASSERT_EQ(0, close(pipe_parent[1]));
> +		if (variant->domain_child)
> +			create_signal_domain(_metadata);
> +
> +		/* Waits for the parent to be in a domain, if any. */
> +		ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));

There is a race condition here when the parent process didn't yet called
pause().

> +
> +		err = kill(parent, variant->sig);
> +		if (can_signal) {
> +			ASSERT_EQ(0, err);
> +		} else {
> +			ASSERT_EQ(-1, err);
> +			ASSERT_EQ(EPERM, errno);
> +		}
> +		/* no matter of the domain, a process should be able to send
> +		 * a signal to itself.
> +		 */
> +		ASSERT_EQ(0, raise(variant->sig));
> +		if (variant->sig > 0)
> +			ASSERT_EQ(1, signaled);
> +		_exit(_metadata->exit_code);
> +		return;
> +	}
> +	ASSERT_EQ(0, close(pipe_parent[0]));
> +	if (variant->domain_parent)
> +		create_signal_domain(_metadata);
> +

/* The process should not have already been signaled. */
EXPECT_EQ(0, signaled);

> +	/* Signals that the parent is in a domain, if any. */
> +	ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> +
> +	if (can_signal && variant->sig > 0) {
> +		ASSERT_EQ(-1, pause());
> +		ASSERT_EQ(EINTR, errno);

This can hang indefinitely if the child process sent the signal after
reading from the pipe and before the call to pause().

This should be a better alternative to the use of pause():

/* Avoids race condition with the child's signal. */
while (!signaled && !usleep(1));
ASSERT_EQ(1, signaled);

BTW, we cannot reliably check for errno because usleep() may still
return 0, but that's OK.

> +		ASSERT_EQ(1, signaled);
> +	} else {
> +		ASSERT_EQ(0, signaled);
> +	}
> +
> +	ASSERT_EQ(child, waitpid(child, &status, 0));
> +
> +	if (WIFSIGNALED(status) || !WIFEXITED(status) ||
> +	    WEXITSTATUS(status) != EXIT_SUCCESS)
> +		_metadata->exit_code = KSFT_FAIL;
> +}
> +
> +TEST_HARNESS_MAIN
> -- 
> 2.34.1
> 
> 

[PATCH v3 4/6] selftest/Landlock: pthread_kill(3) tests

[Tahera Fahimi]

This patch expands the signal scoping tests with pthread_kill(3)
It tests if an scoped thread can send signal to a process in
the same scoped domain, or a non-sandboxed thread.

Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
---
 .../selftests/landlock/scoped_signal_test.c   | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tools/testing/selftests/landlock/scoped_signal_test.c b/tools/testing/selftests/landlock/scoped_signal_test.c
index 92958c6266ca..2edba1e6cd82 100644
--- a/tools/testing/selftests/landlock/scoped_signal_test.c
+++ b/tools/testing/selftests/landlock/scoped_signal_test.c
@@ -10,6 +10,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <linux/landlock.h>
+#include <pthread.h>
 #include <signal.h>
 #include <sys/prctl.h>
 #include <sys/types.h>
@@ -18,6 +19,7 @@
 
 #include "common.h"
 
+#define DEFAULT_THREAD_RUNTIME 0.001
 static sig_atomic_t signaled;
 
 static void create_signal_domain(struct __test_metadata *const _metadata)
@@ -299,4 +301,31 @@ TEST_F(signal_scoping, test_signal)
 		_metadata->exit_code = KSFT_FAIL;
 }
 
+static void *thread_func(void *arg)
+{
+	sleep(DEFAULT_THREAD_RUNTIME);
+	return NULL;
+}
+
+TEST(signal_scoping_threads)
+{
+	pthread_t no_sandbox_thread, scoped_thread;
+	int err;
+
+	ASSERT_EQ(0,
+		  pthread_create(&no_sandbox_thread, NULL, thread_func, NULL));
+	create_signal_domain(_metadata);
+	ASSERT_EQ(0, pthread_create(&scoped_thread, NULL, thread_func, NULL));
+
+	/* Send signal to threads */
+	err = pthread_kill(no_sandbox_thread, 0);
+	ASSERT_EQ(EPERM, err);
+
+	err = pthread_kill(scoped_thread, 0);
+	ASSERT_EQ(0, err);
+
+	ASSERT_EQ(0, pthread_join(scoped_thread, NULL));
+	ASSERT_EQ(0, pthread_join(no_sandbox_thread, NULL));
+}
+
 TEST_HARNESS_MAIN
-- 
2.34.1

Re: [PATCH v3 4/6] selftest/Landlock: pthread_kill(3) tests

[Mickaël Salaün]

Please make sure all subject's prefixes are correct, there is two errors
in the prefix and the description can be made more consistent with other
patches, something like this: "selftests/landlock: Add
signal_scoping_threads test"

On Thu, Aug 15, 2024 at 12:29:23PM -0600, Tahera Fahimi wrote:
> This patch expands the signal scoping tests with pthread_kill(3)
> It tests if an scoped thread can send signal to a process in
> the same scoped domain, or a non-sandboxed thread.
> 
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> ---
>  .../selftests/landlock/scoped_signal_test.c   | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/scoped_signal_test.c b/tools/testing/selftests/landlock/scoped_signal_test.c
> index 92958c6266ca..2edba1e6cd82 100644
> --- a/tools/testing/selftests/landlock/scoped_signal_test.c
> +++ b/tools/testing/selftests/landlock/scoped_signal_test.c
> @@ -10,6 +10,7 @@
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <linux/landlock.h>
> +#include <pthread.h>
>  #include <signal.h>
>  #include <sys/prctl.h>
>  #include <sys/types.h>
> @@ -18,6 +19,7 @@
>  
>  #include "common.h"
>  
> +#define DEFAULT_THREAD_RUNTIME 0.001
>  static sig_atomic_t signaled;
>  
>  static void create_signal_domain(struct __test_metadata *const _metadata)
> @@ -299,4 +301,31 @@ TEST_F(signal_scoping, test_signal)
>  		_metadata->exit_code = KSFT_FAIL;
>  }
>  
> +static void *thread_func(void *arg)
> +{
> +	sleep(DEFAULT_THREAD_RUNTIME);

Using sleep() may make this test flaky.  It needs to be removed and the
test should work the same.

> +	return NULL;
> +}
> +
> +TEST(signal_scoping_threads)
> +{
> +	pthread_t no_sandbox_thread, scoped_thread;
> +	int err;
> +
> +	ASSERT_EQ(0,
> +		  pthread_create(&no_sandbox_thread, NULL, thread_func, NULL));
> +	create_signal_domain(_metadata);
> +	ASSERT_EQ(0, pthread_create(&scoped_thread, NULL, thread_func, NULL));
> +
> +	/* Send signal to threads */
> +	err = pthread_kill(no_sandbox_thread, 0);
> +	ASSERT_EQ(EPERM, err);
> +
> +	err = pthread_kill(scoped_thread, 0);
> +	ASSERT_EQ(0, err);
> +
> +	ASSERT_EQ(0, pthread_join(scoped_thread, NULL));
> +	ASSERT_EQ(0, pthread_join(no_sandbox_thread, NULL));
> +}
> +
>  TEST_HARNESS_MAIN
> -- 
> 2.34.1
> 
> 

Re: [PATCH v3 4/6] selftest/Landlock: pthread_kill(3) tests

[Mickaël Salaün]

On Tue, Aug 20, 2024 at 05:57:08PM +0200, Mickaël Salaün wrote:
> Please make sure all subject's prefixes are correct, there is two errors
> in the prefix and the description can be made more consistent with other
> patches, something like this: "selftests/landlock: Add
> signal_scoping_threads test"
> 
> On Thu, Aug 15, 2024 at 12:29:23PM -0600, Tahera Fahimi wrote:
> > This patch expands the signal scoping tests with pthread_kill(3)
> > It tests if an scoped thread can send signal to a process in
> > the same scoped domain, or a non-sandboxed thread.
> > 
> > Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> > ---
> >  .../selftests/landlock/scoped_signal_test.c   | 29 +++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/landlock/scoped_signal_test.c b/tools/testing/selftests/landlock/scoped_signal_test.c
> > index 92958c6266ca..2edba1e6cd82 100644
> > --- a/tools/testing/selftests/landlock/scoped_signal_test.c
> > +++ b/tools/testing/selftests/landlock/scoped_signal_test.c
> > @@ -10,6 +10,7 @@
> >  #include <errno.h>
> >  #include <fcntl.h>
> >  #include <linux/landlock.h>
> > +#include <pthread.h>
> >  #include <signal.h>
> >  #include <sys/prctl.h>
> >  #include <sys/types.h>
> > @@ -18,6 +19,7 @@
> >  
> >  #include "common.h"
> >  
> > +#define DEFAULT_THREAD_RUNTIME 0.001
> >  static sig_atomic_t signaled;
> >  
> >  static void create_signal_domain(struct __test_metadata *const _metadata)
> > @@ -299,4 +301,31 @@ TEST_F(signal_scoping, test_signal)
> >  		_metadata->exit_code = KSFT_FAIL;
> >  }
> >  
> > +static void *thread_func(void *arg)
> > +{
> > +	sleep(DEFAULT_THREAD_RUNTIME);
> 
> Using sleep() may make this test flaky.  It needs to be removed and the
> test should work the same.
> 
> > +	return NULL;
> > +}
> > +
> > +TEST(signal_scoping_threads)
> > +{
> > +	pthread_t no_sandbox_thread, scoped_thread;
> > +	int err;
> > +
> > +	ASSERT_EQ(0,
> > +		  pthread_create(&no_sandbox_thread, NULL, thread_func, NULL));
> > +	create_signal_domain(_metadata);
> > +	ASSERT_EQ(0, pthread_create(&scoped_thread, NULL, thread_func, NULL));
> > +
> > +	/* Send signal to threads */
> > +	err = pthread_kill(no_sandbox_thread, 0);
> > +	ASSERT_EQ(EPERM, err);

Sometime the test failed because err == 0, I guess it's because I
removed the call to sleep(), but this just highlight a race condition in
the code anyway.  We would need a synchronization primitive to make sure
the thread is still alive, something like the pipe read/write.

> > +
> > +	err = pthread_kill(scoped_thread, 0);
> > +	ASSERT_EQ(0, err);
> > +
> > +	ASSERT_EQ(0, pthread_join(scoped_thread, NULL));
> > +	ASSERT_EQ(0, pthread_join(no_sandbox_thread, NULL));
> > +}
> > +
> >  TEST_HARNESS_MAIN
> > -- 
> > 2.34.1
> > 
> > 

[PATCH v3 5/6] sample/Landlock: Support signal scoping restriction

[Tahera Fahimi]

A sandboxer can receive the character "s" as input from the environment
variable LL_SCOPE to restrict itself from sending a signal to a process
outside its scoped domain.

Example
=======
Create a sandboxed shell and pass the character "s" to LL_SCOPED:
LL_FS_RO=/ LL_FS_RW=. LL_SCOPED="s" ./sandboxer /bin/bash
Try to send a SIGTRAP to a process with process ID <PID> through:
kill -SIGTRAP <PID>
The sandboxed process should not be able to send the signal.

Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
---
v3:
- Add a restrict approach on input of LL_SCOPED, so it only allows
  zero or one "s" to be the input.
---
 samples/landlock/sandboxer.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index bec201eb96f7..32fec6cede2c 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -191,10 +191,12 @@ static bool check_ruleset_scope(const char *const env_var,
 				struct landlock_ruleset_attr *ruleset_attr)
 {
 	bool abstract_scoping = false;
+	bool signal_scoping = false;
 	bool ret = true;
 	char *env_type_scope, *env_type_scope_next, *ipc_scoping_name;
 
-	ruleset_attr->scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
+	ruleset_attr->scoped &= ~(LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET |
+				  LANDLOCK_SCOPED_SIGNAL);
 	env_type_scope = getenv(env_var);
 	/* scoping is not supported by the user */
 	if (!env_type_scope)
@@ -209,6 +211,10 @@ static bool check_ruleset_scope(const char *const env_var,
 			abstract_scoping = true;
 			ruleset_attr->scoped |=
 				LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
+		} else if (strcmp("s", ipc_scoping_name) == 0 &&
+			   !signal_scoping) {
+			signal_scoping = true;
+			ruleset_attr->scoped |= LANDLOCK_SCOPED_SIGNAL;
 		} else {
 			fprintf(stderr, "Unsupported scoping \"%s\"\n",
 				ipc_scoping_name);
@@ -260,7 +266,8 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		.handled_access_fs = access_fs_rw,
 		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
 				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
-		.scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
+		.scoped = LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET |
+			  LANDLOCK_SCOPED_SIGNAL,
 	};
 
 	if (argc < 2) {
@@ -297,7 +304,7 @@ int main(const int argc, char *const argv[], char *const *const envp)
 			"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
 			"%s=\"9418\" "
 			"%s=\"80:443\" "
-			"%s=\"a\" "
+			"%s=\"a:s\" "
 			"%s bash -i\n\n",
 			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
 			ENV_TCP_CONNECT_NAME, ENV_SCOPED_NAME, argv[0]);
@@ -371,7 +378,8 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		__attribute__((fallthrough));
 	case 5:
 		/* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
-		ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
+		ruleset_attr.scoped &= ~(LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET |
+					 LANDLOCK_SCOPED_SIGNAL);
 		fprintf(stderr,
 			"Hint: You should update the running kernel "
 			"to leverage Landlock features "
-- 
2.34.1

[PATCH v3 6/6] Landlock: Document LANDLOCK_SCOPED_SIGNAL

[Tahera Fahimi]

Improving Landlock ABI version 6 to support signal scoping
with LANDLOCK_SCOPED_SIGNAL.

Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
---
v3:
- update date
---
 Documentation/userspace-api/landlock.rst | 25 +++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 0582f93bd952..01e4d50851af 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -8,7 +8,7 @@ Landlock: unprivileged access control
 =====================================
 
 :Author: Mickaël Salaün
-:Date: July 2024
+:Date: August 2024
 
 The goal of Landlock is to enable to restrict ambient rights (e.g. global
 filesystem or network access) for a set of processes.  Because Landlock
@@ -82,7 +82,8 @@ to be explicit about the denied-by-default access rights.
             LANDLOCK_ACCESS_NET_BIND_TCP |
             LANDLOCK_ACCESS_NET_CONNECT_TCP,
         .scoped =
-            LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
+            LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET |
+            LANDLOCK_SCOPED_SIGNAL,
     };
 
 Because we may not know on which kernel version an application will be
@@ -123,7 +124,8 @@ version, and only use the available subset of access rights:
         ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
     case 5:
         /* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
-        ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
+        ruleset_attr.scoped &= ~(LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET |
+                                 LANDLOCK_SCOPED_SIGNAL);
     }
 
 This enables to create an inclusive ruleset that will contain our rules.
@@ -319,11 +321,15 @@ interactions between sandboxes. Each Landlock domain can be explicitly scoped
 for a set of actions by specifying it on a ruleset. For example, if a sandboxed
 process should not be able to :manpage:`connect(2)` to a non-sandboxed process
 through abstract :manpage:`unix(7)` sockets, we can specify such restriction
-with ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``.
+with ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``. Moreover, if a sandboxed process
+should not be able to send a signal to a non-sandboxed process, we can specify
+this restriction with ``LANDLOCK_SCOPED_SIGNAL``.
 
 A sandboxed process can connect to a non-sandboxed process when its domain is
 not scoped. If a process's domain is scoped, it can only connect to sockets
-created by processes in the same scoped domain.
+created by processes in the same scoped domain. Moreover, If a process is
+scoped to send signal to a non-scoped process, it can only send signals to
+processes in the same scoped domain.
 
 IPC scoping does not support Landlock rules, so if a domain is scoped, no rules
 can be added to allow accessing to a resource outside of the scoped domain.
@@ -563,12 +569,17 @@ earlier ABI.
 Starting with the Landlock ABI version 5, it is possible to restrict the use of
 :manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right.
 
+<<<<<<< current
 Abstract UNIX sockets Restriction  (ABI < 6)
 --------------------------------------------
+=======
+Abstract Unix sockets and Signal Restriction  (ABI < 6)
+-------------------------------------------------------
+>>>>>>> patched
 
 With ABI version 6, it is possible to restrict connection to an abstract Unix socket
-through ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``, thanks to the ``scoped`` ruleset
-attribute.
+through ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET`` and sending signal through
+``LANDLOCK_SCOPED_SIGNAL``, thanks to the ``scoped`` ruleset attribute.
 
 .. _kernel_support:
 
-- 
2.34.1

Re: [PATCH v3 6/6] Landlock: Document LANDLOCK_SCOPED_SIGNAL

[Tahera Fahimi]

On Thu, Aug 15, 2024 at 12:29:25PM -0600, Tahera Fahimi wrote:
> Improving Landlock ABI version 6 to support signal scoping
> with LANDLOCK_SCOPED_SIGNAL.
> 
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> ---
> v3:
> - update date
> ---
>  Documentation/userspace-api/landlock.rst | 25 +++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index 0582f93bd952..01e4d50851af 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -8,7 +8,7 @@ Landlock: unprivileged access control
>  =====================================
>  
>  :Author: Mickaël Salaün
> -:Date: July 2024
> +:Date: August 2024
>  
>  The goal of Landlock is to enable to restrict ambient rights (e.g. global
>  filesystem or network access) for a set of processes.  Because Landlock
> @@ -82,7 +82,8 @@ to be explicit about the denied-by-default access rights.
>              LANDLOCK_ACCESS_NET_BIND_TCP |
>              LANDLOCK_ACCESS_NET_CONNECT_TCP,
>          .scoped =
> -            LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET,
> +            LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET |
> +            LANDLOCK_SCOPED_SIGNAL,
>      };
>  
>  Because we may not know on which kernel version an application will be
> @@ -123,7 +124,8 @@ version, and only use the available subset of access rights:
>          ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
>      case 5:
>          /* Removes LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET for ABI < 6 */
> -        ruleset_attr.scoped &= ~LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET;
> +        ruleset_attr.scoped &= ~(LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET |
> +                                 LANDLOCK_SCOPED_SIGNAL);
>      }
>  
>  This enables to create an inclusive ruleset that will contain our rules.
> @@ -319,11 +321,15 @@ interactions between sandboxes. Each Landlock domain can be explicitly scoped
>  for a set of actions by specifying it on a ruleset. For example, if a sandboxed
>  process should not be able to :manpage:`connect(2)` to a non-sandboxed process
>  through abstract :manpage:`unix(7)` sockets, we can specify such restriction
> -with ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``.
> +with ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``. Moreover, if a sandboxed process
> +should not be able to send a signal to a non-sandboxed process, we can specify
> +this restriction with ``LANDLOCK_SCOPED_SIGNAL``.
>  
>  A sandboxed process can connect to a non-sandboxed process when its domain is
>  not scoped. If a process's domain is scoped, it can only connect to sockets
> -created by processes in the same scoped domain.
> +created by processes in the same scoped domain. Moreover, If a process is
> +scoped to send signal to a non-scoped process, it can only send signals to
> +processes in the same scoped domain.
>  
>  IPC scoping does not support Landlock rules, so if a domain is scoped, no rules
>  can be added to allow accessing to a resource outside of the scoped domain.
> @@ -563,12 +569,17 @@ earlier ABI.
>  Starting with the Landlock ABI version 5, it is possible to restrict the use of
>  :manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right.
>  
> +<<<<<<< current
>  Abstract UNIX sockets Restriction  (ABI < 6)
>  --------------------------------------------
> +=======
> +Abstract Unix sockets and Signal Restriction  (ABI < 6)
> +-------------------------------------------------------
> +>>>>>>> patched
Sorry about this part. I will correct it. 
>  With ABI version 6, it is possible to restrict connection to an abstract Unix socket
> -through ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET``, thanks to the ``scoped`` ruleset
> -attribute.
> +through ``LANDLOCK_SCOPED_ABSTRACT_UNIX_SOCKET`` and sending signal through
> +``LANDLOCK_SCOPED_SIGNAL``, thanks to the ``scoped`` ruleset attribute.
>  
>  .. _kernel_support:
>  
> -- 
> 2.34.1
>