Message-ID: <CAKPOu+_eL3j9+yaDRMnoCfoyapNSp04rgGaRzpKn9ycbkif_9g@mail.gmail.com>
Date: Tue, 13 May 2025 08:25:10 +0200
From: Max Kellermann <max.kellermann@...os.com>
To: Kees Cook <kees@...nel.org>
Cc: "Serge E. Hallyn" <serge@...lyn.com>, Andy Lutomirski <luto@...nel.org>, paul@...l-moore.com, 
	jmorris@...ei.org, morgan@...nel.org, Eric Biederman <ebiederm@...ssion.com>, 
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] security/commoncap: don't assume "setid" if all ids are identical

On Tue, May 13, 2025 at 3:24 AM Kees Cook <kees@...nel.org> wrote:
>
> On Sat, May 10, 2025 at 02:29:19PM -0500, Serge E. Hallyn wrote:
> > Kees, do you have any input?
>
> I'd like to move this back to the list. I don't think there is anything
> secret here?

I don't think there ever was anything secret, and I don't understand
why it was ever taken off-list. My initial post was public and it
already contained everything.
(My reply will go back to LKML and a full quote of your email to give
context to those who were excluded from this private thread. I assume
you're fine with that?)

>
> Regardless, I apologize for being so late to the discussion. Last week
> was weird. Anyway, random thoughts from the thread and refreshing myself
> on the various code flows:
>
> - musl doesn't clear LD_PRELOAD under AT_SECURE: Ew. That's a bug, IMO.

Why would it be a bug? If a program doesn't use or care for
LD_PRELOAD, why would it need to clear environment variables that may
or may be used by other arbitrary programs? Is every program
responsible for clearing sensitive environment variables for every
other program on the planet? (I'm stretching this for the sake of the
argument, but I hope you get my basic idea.)

> - We care about non-ELF, yes, but currently all realistic paths lead back
>   to ELF (e.g. creds are calculated from the interpreter and not
>   the script for binfmt_script).
>
> - "classic elevated privilege check" in userspace is just wrong. It's
>   been insufficient to check for euid!=uid for decades. Looking at
>   AT_SECURE is, afaict, sufficient, though.

We agree that AT_SECURE has been the canonical Linux-specific way for
this check for at least 20 years.

Yes, everybody should have an #ifdef __linux__ with this check instead
of geteuid()!=getuid(). But that's not the reality we're living in.
Did you know that not even util-linux and e2fsprogs get this right?
Look at:

- https://github.com/util-linux/util-linux/pull/3566
- https://github.com/tytso/e2fsprogs/pull/226

(I posted patches to a bunch of projects to get userspace fixed.
Others that don't get it right include: cups, libevent, Firefox, Qt,
Chromium, GnuPG, krb5, Postfix, Mono, iptables and many many others.)

The reality is that knowledge of the existence of AT_SECURE has
propagated only to a tiny bubble, and the majority of people have
never heard of it.

That alone would not justify fixing the NO_NEW_PRIVS behavior; but it
looks like the NO_NEW_PRIVS behavior was initially an undesired side
effect of a broken setuid check in the kernel. What the kernel does
here is not documented, and neither does it make sense. This is really
why I argue that the kernel should be fixed. If it doesn't make sense
AND breaks userspace, there is really only one way to fix it. But
that's just my opinion.

> - Linux has a kind of messy separation of logic:
>         - fs/exec.c is looking at DAC, but sets up checks for ptrace,
>           nnp, shared fs
>         - security/commoncap.c is evaluating DAC, caps, and ptrace.
>
> - in the kernel, having euid != uid is _defined_ as being setuid.
>   Whether or not this is "more privileged" isn't part of the definition.
>
> - NNP applies only in two place:
>         - stopping gain of euid/egid from setuid/setgid of a file (same
>           as nosuid mount option) in bprm_fill_uid().
>         - unconditionally removing euid when lacking CAP_SETUID,
>           in cap_bprm_creds_from_file() (same as shared fs, ptracing).
>           (The explicit check for NNP is not redundant here: it's forcing
>           NNP even with CAP_SETUID, where as the other LSM_UNSAFE flags
>           allow CAP_SETUID to bypass them.)
>
> I've read through the thread and I'm convinced the original patch should
> not be applied (it changes the definition of setuid and setgid). Having
> the _purpose_ of the patch explain in the commit log next time would be
> good, but that came out in the thread later: there is a removal of euid
> under the described construction of apache/php/etc.
>
> In CAKPOu+8+1uVrDJHwmHJd2d46-N6AwjR4_bbtoSJS+sx6J=rkjg@...l.gmail.com:
> > Without my patch:
> >
> >  $ /reexec
> >  ruid=1000 euid=0
>
> I'm so confused. Is this "reexec" marked setuid=0?
>
> >  re-exec
> >  ruid=1000 euid=0
> >  $ /reexec 1
> >  ruid=1000 euid=0
> >  setting NO_NEW_PRIVS
> >  re-exec
> >  ruid=1000 euid=1000
>
> I assume so, since this is the behavior I'd expect.
>
> > Without NO_NEW_PRIVS, the re-exec keeps the real/effective, but also
> > gains setuid again, but the suid is no-op; the euid is already 0.
> > With NO_NEW_PRIVS, the re-exec drops the euid=0, and doesn't regain it
> > - the setuid bit is ignored.
> >
> > With my patch:
> >
> >  $ /reexec
> >  ruid=1000 euid=0
> >  re-exec
> >  ruid=1000 euid=0
> >  $ /reexec 1
> >  ruid=1000 euid=0
> >  setting NO_NEW_PRIVS
> >  re-exec
> >  ruid=1000 euid=0
>
> I see how this looks inconsistent, but it is correct: any LSM_UNSAFE_*
> flag will trigger the stripping of euid. It could be argued it should
> strip _more_, IMO.

One can define NO_NEW_PRIVS this way (though I don't agree that it
should be), but that is not how it is documented currently. And the
name NO_NEW_PRIVS doesn't even remotely imply this behavior; you argue
for taking away privileges, but the name says no new privileges shall
be granted; keeping existing privileges is that.

For better NO_NEW_PRIVS documentation, please consider merging my
patch: https://lore.kernel.org/lkml/20250509184105.840928-1-max.kellermann@ionos.com/

In any case, I will keep my "ids identical" patch (with "secureexec"
bug fix) in our private kernel fork, because we need this behavior,
one way or another, but there is only this way. Thanks to everybody
who participated in the discussion - I really learned something here,
and that helped me understand the "secureexec" mistake in my patch.


Max

Powered by blists - more mailing lists