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

["Eric W. Biederman" <ebiederm@xmission.com>]

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.

> 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?

The patch that made the change doesn't show up on lore.kernel.org so I
believe any record of the rational is lost.

To me it looks like someone was right up against the deadline to get
their code change into 2.4.0, was used to uid != euid in userspace, and
talked Linus into a last minute merge before 2.4.0 was released.

Eric