[PATCH] exec: Correct the permission check for unsafe exec

[Jann Horn]

On Fri, May 16, 2025 at 5:26 PM Eric W. Biederman <ebiederm at xmission.com> wrote:
> Kees Cook <kees at kernel.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?