Re: [PATCH v2 1/4] Landlock: Add signal control

On Wed, Aug 07, 2024 at 08:16:47PM +0200, Mickaël Salaün wrote:
> On Tue, Aug 06, 2024 at 11:55:27PM +0200, Jann Horn wrote:
> > On Tue, Aug 6, 2024 at 8:56 PM Jann Horn <[email protected]> wrote:
> > > On Tue, Aug 6, 2024 at 8:11 PM Tahera Fahimi <[email protected]> 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. 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).
> > [...]
> > > > +       if (is_scoped)
> > > > +               return 0;
> > > > +
> > > > +       return -EPERM;
> > > > +}
> > > > +
> > > > +static int hook_file_send_sigiotask(struct task_struct *tsk,
> > > > +                                   struct fown_struct *fown, int signum)
> 
> I was wondering if we should handle this case, but I guess it makes
> sense to have a consistent policy for all kind of user-triggerable
> signals.
> 
> > > > +{
> > > > +       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 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.
> > 
> > (For what it's worth, I think the first option would probably be
> > easier to implement and ship for now, since you can basically copy
> > what Smack and SELinux are already doing in their implementations of
> > these hooks. I think the second option would theoretically result in
> > nicer code, but it might require a bit more work, and you'd have to
> > include the maintainers of the file locking code in the review of such
> > refactoring and have them approve those changes. So if you want to get
> > this patchset into the kernel quickly, the first option might be
> > better for now?)
> > 
> 
> I agree, let's extend landlock_file_security with a new "fown" pointer
> to a Landlock domain. We'll need to call landlock_get_ruleset() in
> hook_file_send_sigiotask(), and landlock_put_ruleset() in a new
> hook_file_free_security().
I think we should add a new hook (hook_file_set_owner()) to initialize
the "fown" pointer and call landlock_get_ruleset() in that? If we do not
have hook_file_set_owner to store domain in "fown", can you please give
me a hint on where to do that?
Thanks 
> I would be nice to to replace the redundant informations in fown_struct
> but that can wait.