Re: [PATCH] security/commoncap: don't assume "setid" if all ids are identical

["Serge E. Hallyn" <serge@hallyn.com>]

On Mon, Apr 28, 2025 at 01:43:43PM +0200, Max Kellermann wrote:
> On Sun, Mar 9, 2025 at 4:19 PM Serge E. Hallyn <serge@hallyn.com> wrote:
> >

Adding Kees and Andrew Morgan for their opinions.

Sorry, I had snipped the actual patch below.  Pre-b4 I would have appended it
here, but as you can just
   b4 mbox CAKPOu+_vTuZqsBLfRH+kyphiWAtRfWq=nKAcAYu=Wn2JBAkkYg@mail.gmail.com
I won't do so unless you ask me to.

> > On Thu, Mar 06, 2025 at 09:26:15AM +0100, Max Kellermann wrote:
> > > If a program enables `NO_NEW_PRIVS` and sets up
> > > differing real/effective/saved/fs ids, the effective ids are
> > > downgraded during exec because the kernel believes it should "get no
> > > more than they had, and maybe less".

Just to quibble here: I don't use NO_NEW_PRIVS, but it seems to me quite
likely that your claim is wrong here.  The whole SECBIT_KEEP_CAPS etc
dance is based on the idea that you understand that once you exec, you
lose some of your existing privilege.  Similarly, it seems quite
likely to me that people using NO_NEW_PRIVS understand, expect, and
count on the fact that their effective ids will be cleared on exec.

Note also that so far I'm only asking about the intent of the patch.
Apart from that, I do think the implementation is wrong, because you
are impacting non-NO_NEW_PRIVS behavior as well, such as calculation
of cap_permitted and the clearing of ambient capabilities.
And, I'm not sure the has_identical_uids_gids() is quite right, as I'm
not sure what the bprm->cred->fsuid and suid make sense, though the
process's fsuid and suid of course need to be checked.

> > > I believe it is safe to keep differing ids even if `NO_NEW_PRIVS` is
> > > set.  The newly executed program doesn't get any more, but there's no
> > > reason to give it less.
> > >
> > > This is different from "set[ug]id/setpcap" execution where privileges
> > > may be raised; here, the assumption that it's "set[ug]id" if
> > > effective!=real is too broad.
> > >
> > > If we verify that all user/group ids remain as they were, we can
> > > safely allow the new program to keep them.
> >
> > Thanks, it's an interesting point.  Seems to mainly depend on what users
> > of the feature have come to expect.
> >
> > Andy, what do you think?
> 
> Serge & Andy, what's your opinion on my patch?