[PATCH v1 1/2] Landlock: Add signal control

[PATCH v1 1/2] 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.

The same way as abstract unix socket, 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>
---
 include/uapi/linux/landlock.h |  3 +++
 security/landlock/limits.h    |  2 +-
 security/landlock/task.c      | 49 +++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 010aaca5b05a..878479a1b9dd 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -291,8 +291,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 process
  *   which is not sandboxed).
+ * - %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 acc6e0fbc111..caee485b97b2 100644
--- a/security/landlock/task.c
+++ b/security/landlock/task.c
@@ -168,11 +168,60 @@ static int hook_unix_may_send(struct socket *const sock,
 	return -EPERM;
 }
 
+static bool signal_is_scoped(const struct landlock_ruleset *const sender_dom,
+			     struct task_struct *const target)
+{
+	const struct landlock_ruleset *target_dom =
+		landlock_get_task_domain(target);
+
+	/* quick return if there is no domain or .scoped is not set */
+	if (!sender_dom || !get_scoped_accesses(sender_dom))
+		return true;
+
+	if (!target_dom || !get_scoped_accesses(target_dom))
+		return false;
+
+	/* other is scoped, they connect if they are in the same domain */
+	return domain_scope_le(sender_dom, target_dom);
+}
+
+static int hook_task_kill(struct task_struct *const p,
+			  struct kernel_siginfo *const info, const int sig,
+			  const struct cred *const cred)
+{
+	const struct landlock_ruleset *const dom =
+		landlock_get_current_domain();
+	bool ret = false;
+
+	if (!cred)
+		ret = signal_is_scoped(dom, p);
+	else
+		ret = signal_is_scoped(landlock_cred(cred)->domain, p);
+	if (ret)
+		return 0;
+	return EPERM;
+}
+
+static int hook_file_send_sigiotask(struct task_struct *tsk,
+				    struct fown_struct *fown, int signum)
+{
+	const struct task_struct *result =
+		get_pid_task(fown->pid, fown->pid_type);
+
+	const struct landlock_ruleset *const dom =
+		landlock_get_task_domain(result);
+	if (signal_is_scoped(dom, tsk))
+		return 0;
+	return EPERM;
+}
+
 static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme),
 	LSM_HOOK_INIT(unix_stream_connect, hook_unix_stream_connect),
 	LSM_HOOK_INIT(unix_may_send, hook_unix_may_send),
+	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

[PATCH v1 2/2] Landlock: Signal scoping tests

[Tahera Fahimi]

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

diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
index a19db4d0b3bd..e092b67f8b67 100644
--- a/tools/testing/selftests/landlock/ptrace_test.c
+++ b/tools/testing/selftests/landlock/ptrace_test.c
@@ -17,6 +17,8 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
+#include <signal.h>
+
 #include "common.h"
 
 /* Copied from security/yama/yama_lsm.c */
@@ -25,6 +27,8 @@
 #define YAMA_SCOPE_CAPABILITY 2
 #define YAMA_SCOPE_NO_ATTACH 3
 
+static sig_atomic_t signaled;
+
 static void create_domain(struct __test_metadata *const _metadata)
 {
 	int ruleset_fd;
@@ -436,4 +440,216 @@ TEST_F(hierarchy, trace)
 		_metadata->exit_code = KSFT_FAIL;
 }
 
+static void create_sig_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)
+		signaled = 1;
+
+	// signal process group
+	//kill(-(t->pid), SIGKILL);
+}
+
+/* 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;
+};
+
+/* Default Action: Terminate*/
+/* clang-format off */
+FIXTURE_VARIANT_ADD(signal_scoping, deny_with_forked_domain) {
+	/* 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) {
+	/* 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,
+};
+
+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_child[2], pipe_parent[2];
+	//char buf_parent;
+
+	struct sigaction action = {
+		.sa_sigaction = scope_signal_handler,
+		.sa_flags = SA_SIGINFO,
+
+	};
+
+	can_signal = !variant->domain_child;
+
+	//sigemptyset(&act.sa_mask);
+
+	ASSERT_LE(0, sigaction(variant->sig, &action, NULL))
+	{
+		TH_LOG("ERROR in sigaction %s", strerror(errno));
+	}
+
+	if (variant->domain_both) {
+		create_sig_domain(_metadata);
+		if (!__test_passed(_metadata))
+			/* Aborts before forking. */
+			return;
+	}
+	ASSERT_EQ(0, pipe2(pipe_child, O_CLOEXEC));
+	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]));
+		ASSERT_EQ(0, close(pipe_child[0]));
+
+		if (variant->domain_child)
+			create_sig_domain(_metadata);
+
+		/* Waits for the parent to be in a domain, if any. */
+		ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
+
+		//err = raise(SIGHUP);
+		err = kill(parent, variant->sig);
+		if (can_signal) {
+			ASSERT_EQ(0, err);
+		} else {
+			ASSERT_EQ(EPERM, errno)
+			{
+				TH_LOG("Invalid error cached: %s",
+				       strerror(errno));
+			}
+		}
+		_exit(_metadata->exit_code);
+		return;
+	}
+
+	ASSERT_EQ(0, close(pipe_child[1]));
+	ASSERT_EQ(0, close(pipe_parent[0]));
+	if (variant->domain_parent)
+		create_sig_domain(_metadata);
+
+	/* Signals that the parent is in a domain, if any. */
+	ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
+
+	if (can_signal) {
+		ASSERT_EQ(-1, pause());
+		ASSERT_EQ(EINTR, errno);
+		ASSERT_EQ(1, signaled);
+	}
+
+	ASSERT_EQ(child, waitpid(child, &status, 0));
+
+	if (WIFEXITED(status)) {
+		TH_LOG("Exited with code %d:", WEXITSTATUS(status));
+		if (!can_signal)
+			ASSERT_NE(1, signaled);
+	}
+
+	if (WIFSIGNALED(status) || !WIFEXITED(status) ||
+	    WEXITSTATUS(status) != EXIT_SUCCESS)
+		_metadata->exit_code = KSFT_FAIL;
+}
+
 TEST_HARNESS_MAIN
-- 
2.34.1

Re: [PATCH v1 2/2] Landlock: Signal scoping tests

[Günther Noack]

On Fri, Jul 05, 2024 at 03:21:43PM -0600, Tahera Fahimi wrote:
> Signed-off-by: Tahera Fahimi <fahimitahera@gmail.com>
> ---
>  .../testing/selftests/landlock/ptrace_test.c  | 216 ++++++++++++++++++
>  1 file changed, 216 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
> index a19db4d0b3bd..e092b67f8b67 100644
> --- a/tools/testing/selftests/landlock/ptrace_test.c
> +++ b/tools/testing/selftests/landlock/ptrace_test.c
> [...]

> +static void scope_signal_handler(int sig, siginfo_t *info, void *ucontext)
> +{
> +	if (sig == SIGHUP || sig == SIGURG || sig == SIGTSTP || sig == SIGTRAP)
> +		signaled = 1;
> +
> +	// signal process group
> +	//kill(-(t->pid), SIGKILL);

There is commented-out code like this in various places in this patch.

I am pretty sure that scripts/checkpatch.pl should flag that.
See https://docs.kernel.org/dev-tools/checkpatch.html
and https://docs.kernel.org/process/submitting-patches.html#style-check-your-changes

I personally just keep a checklist of things to remember before sending a patch.
(rebase as needed, clang-format -i (for Landlock files), run tests, check commit
metadata, git format-patch with -v and --cover-letter, scripts/checkpatch.pl,
edit cover letter, git send-email)

—Günther

Re: [PATCH v1 1/2] Landlock: Add signal control

[Jann Horn]

On Fri, Jul 5, 2024 at 11:22 PM Tahera Fahimi <fahimitahera@gmail.com> wrote:
> 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.
>
> The same way as abstract unix socket, 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>
[...]
> +static int hook_file_send_sigiotask(struct task_struct *tsk,
> +                                   struct fown_struct *fown, int signum)
> +{
> +       const struct task_struct *result =
> +               get_pid_task(fown->pid, fown->pid_type);

get_pid_task() returns a refcounted reference; you'll have to call
put_task_struct(result) to drop this reference at the end of the
function.

> +       const struct landlock_ruleset *const dom =
> +               landlock_get_task_domain(result);
> +       if (signal_is_scoped(dom, tsk))
> +               return 0;
> +       return EPERM;
> +}