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

[Kees Cook <kees@kernel.org>]

On Fri, May 16, 2025 at 06:29:21PM -0500, Eric W. Biederman wrote:
> Jann Horn <jannh@google.com> writes:
> 
> > On Fri, May 16, 2025 at 5:26 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> >> Kees Cook <kees@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?
> 
> Yes.  bprm->secureexec controls AT_SECURE.
> 
> I am fine if we want to set secureexec and AT_SECURE in this situation.
> It is a bit odd, but I don't see a problem with that.

So the idea would be that uid/euid mismatch would still induce AT_SECURE?

That seems reasonable.

I was already waiting for the after the coming merge window to put this
into -next, so if you cant update it for the AT_SECURE logic, we can
give that a try and see what we get.

Thanks!

-Kees

-- 
Kees Cook