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

From: Micah Morton <mortonm@chromium.org>
To: Kees Cook <keescook@chromium.org>
Cc: jmorris@namei.org, serge@hallyn.com, casey@schaufler-ca.com,
	sds@tycho.nsa.gov, linux-security-module@vger.kernel.org
Subject: Re: [PATCH] LSM: add SafeSetID module that gates setid calls
Date: Thu, 6 Dec 2018 09:51:36 -0800	[thread overview]
Message-ID: <CAJ-EccPf-u7PC2bzAzqFjz4-W5pFiLxSKcbKwkFYv8avfPdu3g@mail.gmail.com> (raw)
In-Reply-To: <CAGXu5jJCk8p_nkuutM0kPLhwibKEVoA6E=ZqT2r8jVK5H47ORQ@mail.gmail.com>

On Wed, Dec 5, 2018 at 4:08 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Nov 21, 2018 at 8:54 AM <mortonm@chromium.org> wrote:
> >
> > From: Micah Morton <mortonm@chromium.org>
> >
> > SafeSetID gates the setid family of syscalls to restrict UID/GID
> > transitions from a given UID/GID to only those approved by a
> > system-wide whitelist. These restrictions also prohibit the given
> > UIDs/GIDs from obtaining auxiliary privileges associated with
> > CAP_SET{U/G}ID, such as allowing a user to set up user namespace UID
> > mappings. For now, only gating the set*uid family of syscalls is
> > supported, with support for set*gid coming in a future patch set.
> >
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
> > ---
> >
> > Sending a patch developed against the 'next-general' branch of the
> > linux-security tree, since the previous patch versions wouldn't apply
> > cleanly to 'next-general'.
>
> I'm finally getting back around to this. Sorry for the delay!
>
> A few general process notes:
> - Please "version" your patches in the Subject (e.g. "[PATCH v3] LSM:
> add SafeSetID ..."). This helps track discussion.
> - Please include a "changes since last version below the first "---"
> line, to summarize what has changed. This makes review faster for
> people that have read a specific version but need to catch up (like
> me) :)

Ok thanks, will do in the future. The only code change since the
initial upload was to add a do_exit(SIGKILL) line to
setuid_policy_warning() in lsm.c, which will kill any process that
violates the whitelist policy. This way, there can never be a case
where a privileged program fails to drop privilege because of our
whitelist and continues running in an accidentally over-privileged
context.

>
> > +/*
> > + * TODO: Figuring out whether the current syscall number (saved on the kernel
> > + * stack) is one of the set*uid syscalls is an operation that requires checking
> > + * the number against arch-specific constants as seen below. The need for this
> > + * LSM to know about arch-specific syscall stuff is not ideal. Is it better to
> > + * implement an arch-specific function that gets called from this file and
> > + * update arch/Kconfig to mention that the HAVE_SAFESETID symbol should only be
> > + * selected for architectures that implement the function? Any other ideas?
> > + */
>
> What would Stephen's solution for this problem end up looking like? I
> think avoiding the arch-specific-ness would be quite valuable.

I sent a patch here that is an example of how it could be done:
https://www.spinics.net/lists/linux-security-module/msg24504.html.
AFAICT I think this is what Stephen had in mind.

>
> I think adding a capability for this isn't the way to go (there is a
> very painful history on adding capabilities). This feels much more
> like a good mapping to an LSM (it's narrowing a privilege) with a very
> specific policy.
>
> --
> Kees Cook