On Tue, Aug 06, 2024 at 08:56:15PM +0200, Jann Horn wrote:

On Tue, Aug 6, 2024 at 8:11 PM Tahera Fahimi [off-list ref] wrote:

quoted

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).

[...]

quoted

diff --git a/security/landlock/task.c b/security/landlock/task.c
index 7e8579ebae83..a73cff27bb91 100644
--- a/security/landlock/task.c
+++ b/security/landlock/task.c

@@ -261,11 +261,54 @@ static int hook_unix_may_send(struct socket *const sock,
        return -EPERM;
 }

+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;
+
+       /* rcu is already locked */
+       target_dom = landlock_get_task_domain(p);
+       if (cred)
+               /* dealing with USB IO */
+               is_scoped = domain_IPC_scope(landlock_cred(cred)->domain,
+                                            target_dom,
+                                            LANDLOCK_SCOPED_SIGNAL);
+       else
+               is_scoped = domain_IPC_scope(landlock_get_current_domain(),
+                                            target_dom,
+                                            LANDLOCK_SCOPED_SIGNAL);

This might be a bit more concise if you turn it into something like:

/* only USB IO supplies creds */
cred = cred ?: current_cred();
is_scoped = domain_IPC_scope(landlock_cred(cred)->domain,
    target_dom, LANDLOCK_SCOPED_SIGNAL);

but that's just a question of style, feel free to keep it as-is
depending on what you prefer.

Hi Jann,
Thanks for the feedback:)

quoted

+       if (is_scoped)
+               return 0;
+
+       return -EPERM;
+}
+
+static int hook_file_send_sigiotask(struct task_struct *tsk,
+                                   struct fown_struct *fown, int signum)
+{
+       bool is_scoped;
+       const struct landlock_ruleset *dom, *target_dom;
+       struct task_struct *result = get_pid_task(fown->pid, fown->pid_type);

I'm not an expert on how the fowner stuff works, but I think this will
probably give you "result = NULL" if the file owner PID has already
exited, and then the following landlock_get_task_domain() would
probably crash? But I'm not entirely sure about how this works.

I considered since the file structure can always be obtained, then the
file owner PID always exist. I can check if we can use the credentials
stored in struct file instead.

I think the intended way to use this hook would be to instead use the
"file_set_fowner" hook to record the owning domain (though the setup
for that is going to be kind of a pain...), see the Smack and SELinux
definitions of that hook. Or alternatively maybe it would be even
nicer to change the fown_struct to record a cred* instead of a uid and
euid and then use the domain from those credentials for this hook...
I'm not sure which of those would be easier.

Because Landlock does not use any security blob for this purpose, I am
not sure how to record the owner's doamin.
The alternative way looks nice.

quoted

+       /* rcu is already locked! */
+       dom = landlock_get_task_domain(result);
+       target_dom = landlock_get_task_domain(tsk);
+       is_scoped = domain_IPC_scope(dom, target_dom, LANDLOCK_SCOPED_SIGNAL);
+       put_task_struct(result);
+       if (is_scoped)
+               return 0;
+       return -EPERM;
+}