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

From: Kees Cook <keescook@chromium.org>
To: Micah Morton <mortonm@chromium.org>
Cc: James Morris <jmorris@namei.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Stephen Smalley <sds@tycho.nsa.gov>,
	linux-security-module <linux-security-module@vger.kernel.org>
Subject: Re: [PATCH v2] LSM: add SafeSetID module that gates setid calls
Date: Tue, 15 Jan 2019 11:53:00 -0800	[thread overview]
Message-ID: <CAGXu5jKzp0jKKJ7aa7hcEUZHhP5s5Eev0prBP3EDuHKahV15hA@mail.gmail.com> (raw)
In-Reply-To: <CAJ-EccOoR4+WsjHC=ZGE8NgLiwGC0jO89ETx_wtB4QW9dE-tRQ@mail.gmail.com>

On Tue, Jan 15, 2019 at 11:49 AM Micah Morton <mortonm@chromium.org> wrote:
> On Mon, Jan 14, 2019 at 4:38 PM Kees Cook <keescook@chromium.org> wrote:
> > On Fri, Jan 11, 2019 at 9:13 AM <mortonm@chromium.org> wrote:
> > > From: Micah Morton <mortonm@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