[PATCH v2] LSM: add SafeSetID module that gates setid calls

[Kees Cook]

On Tue, Jan 15, 2019 at 11:49 AM Micah Morton <mortonm at chromium.org> wrote:
> On Mon, Jan 14, 2019 at 4:38 PM Kees Cook <keescook at chromium.org> wrote:
> > On Fri, Jan 11, 2019 at 9:13 AM <mortonm at chromium.org> wrote:
> > > From: Micah Morton <mortonm at chromium.org>
> > > +static void setuid_policy_violation(kuid_t parent, kuid_t child)
> > > +{
> > > +       pr_warn("UID transition (%d -> %d) blocked",
> > > +               __kuid_val(parent),
> > > +               __kuid_val(child));
> > > +        /*
> > > +         * Kill this process to avoid potential security vulnerabilities
> > > +         * that could arise from a missing whitelist entry preventing a
> > > +         * privileged process from dropping to a lesser-privileged one.
> > > +         */
> > > +        do_exit(SIGKILL);
> >
> > I think I asked earlier if this should be an unblockable signal raise
> > instead of a do_exit(). I don't remember if that got answered?
>
> Could you elaborate on this a bit, or share a pointer to some code/doc
> that explains the difference? I don't recall this point being raised
> before (might have missed it), and I'm no expert on the different
> approaches to killing a process in this way.

Sure! So, do_exit() is a big hammer, and skips a lot of clean-up-like
things (for example, this will skip core dumping, if it was desired by
the process, etc). It certainly has its place (and this may be it),
but I think it would be more sensible to use:

force_sig(SIGKILL, current);

and then the regular processing continues after this, and the kernel
will check for pending signals before returning to userspace. Though
please check this with strace to make sure the bad setuid() call never
returns... if it does, then I've got this wrong and do_exit() really
is appropriate here.

-- 
Kees Cook