linux-kernel - Re: [PATCH] exec: Correct the permission check for unsafe exec

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202505201542.B8F7E7D0B@keescook>
Date: Tue, 20 May 2025 15:43:15 -0700
From: Kees Cook <kees@...nel.org>
To: Jann Horn <jannh@...gle.com>
Cc: sergeh@...nel.org, "Eric W. Biederman" <ebiederm@...ssion.com>,
	Max Kellermann <max.kellermann@...os.com>,
	"Serge E. Hallyn" <serge@...lyn.com>, paul@...l-moore.com,
	jmorris@...ei.org, Andy Lutomirski <luto@...nel.org>,
	morgan@...nel.org, Christian Brauner <christian@...uner.io>,
	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] exec: Correct the permission check for unsafe exec

On Wed, May 21, 2025 at 12:38:33AM +0200, Jann Horn wrote:
> On Fri, May 16, 2025 at 11:46 PM <sergeh@...nel.org> wrote:
> > On Fri, May 16, 2025 at 08:06:15PM +0200, Jann Horn wrote:
> > > On Fri, May 16, 2025 at 5:26 PM Eric W. Biederman <ebiederm@...ssion.com> wrote:
> > > > Kees Cook <kees@...nel.org> writes:
> > > >
> > > > > On Thu, May 15, 2025 at 11:24:47AM -0500, Eric W. Biederman wrote:
> > > > >> I have condensed the logic from Linux-2.4.0-test12 to just:
> > > > >>      id_changed = !uid_eq(new->euid, old->euid) || !in_group_p(new->egid);
> > > > >>
> > > > >> This change is userspace visible, but I don't expect anyone to care.
> > > > >> [...]
> > > > >> -static inline bool __is_setuid(struct cred *new, const struct cred *old)
> > > > >> -{ return !uid_eq(new->euid, old->uid); }
> > > > >> -
> > > > >> -static inline bool __is_setgid(struct cred *new, const struct cred *old)
> > > > >> -{ return !gid_eq(new->egid, old->gid); }
> > > > >> -
> > > > >> [...]
> > > > >> -    is_setid = __is_setuid(new, old) || __is_setgid(new, old);
> > > > >> +    id_changed = !uid_eq(new->euid, old->euid) || !in_group_p(new->egid);
> > > > >
> > > > > The core change here is testing for differing euid rather than
> > > > > mismatched uid/euid. (And checking for egid in the set of all groups.)
> > > >
> > > > Yes.
> > > >
> > > > For what the code is trying to do I can't fathom what was trying to
> > > > be accomplished by the "mismatched" uid/euid check.
> > >
> > > I remember that when I was looking at this code years ago, one case I
> > > was interested in was what happens when a setuid process (running with
> > > something like euid=1000,ruid=0) execve()'s a normal binary. Clearly
> > > the LSM_UNSAFE_* stuff is not so interesting there, because we're
> > > already coming from a privileged context; but the behavior of
> > > bprm->secureexec could be important.
> > >
> > > Like, I think currently a setuid binary like this is probably (?) not
> > > exploitable:
> > >
> > > int main(void) {
> > >   execl("/bin/echo", "echo", "hello world");
> > > }
> > >
> > > but after your proposed change, I think it might (?) become
> > > exploitable because "echo" would not have AT_SECURE set (I think?) and
> > > would therefore load libraries based on environment variables?
> > >
> > > To be clear, I think this would be a stupid thing for userspace to do
> > > - a setuid binary just should not be running other binaries with the
> > > caller-provided environment while having elevated privileges. But if
> > > userspace was doing something like that, this change might make it
> > > more exploitable, and I imagine that the check for mismatched uid/euid
> > > was intended to catch cases like this?
> >
> > If the original process became privileged by executing a setuid-root
> > file (and uses glibc), then LD_PRELOAD will already have been cleared,
> > right?  So it would either have to add the unsafe entries back to
> > LD_PRELOAD again, or it has to have been root all along, not a
> > setuid-root program.  I think at that point we have to say this is what
> > it intended, and possibly with good reason.
> 
> Oh, I see what you mean, glibc's loader code zaps that environment
> variable on secureexec for additional safety, I didn't know that.

It was pointed out that musl does _not_ zap it; it just ignores it but
leaves it set. (I have not verified this myself...)

-- 
Kees Cook