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

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

Jann Horn <jannh@google.com> writes:

> On Wed, May 21, 2025 at 12:13 AM Eric W. Biederman
> <ebiederm@xmission.com> wrote:

> Looks good to me overall, thanks for figuring out the history of this
> not-particularly-easy-to-understand code and figuring out the right
> fix.
>
> Reviewed-by: Jann Horn <jannh@google.com>
>
>> @@ -917,7 +911,7 @@ int cap_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *file)
>>         /* Process setpcap binaries and capabilities for uid 0 */
>>         const struct cred *old = current_cred();
>>         struct cred *new = bprm->cred;
>> -       bool effective = false, has_fcap = false, is_setid;
>> +       bool effective = false, has_fcap = false, id_changed;
>>         int ret;
>>         kuid_t root_uid;
>>
>> @@ -941,9 +935,9 @@ int cap_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *file)
>>          *
>>          * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
>>          */
>> -       is_setid = __is_setuid(new, old) || __is_setgid(new, old);
>> +       id_changed = !uid_eq(new->euid, old->euid) || !in_group_p(new->egid);
>
> Hm, so when we change from one EGID to another EGID which was already
> in our groups list, we don't treat it as a privileged exec? Which is
> okay because, while an unprivileged user would not just be allowed to
> change their EGID to a GID from their groups list themselves through
> __sys_setregid(), they would be allowed to create a new setgid binary
> owned by a group from their groups list and then execute that?
>
> That's fine with me, though it seems a little weird to me. setgid exec
> is changing our creds and yet we're not treating it as a "real" setgid
> execution because the execution is only granting privileges that
> userspace could have gotten anyway.

More than could have gotten.  From permission checking point of view
permission that the application already had.  In general group based
permission checks just check in_group_p, which looks at cred->fsgid and
the group.

The logic is since the effective permissions of the running executable
have not changed, there is nothing to special case.

Arguably a setgid exec can drop what was egid, and if people have
configured their permissions to deny people access based upon a group
they are in that could change the result of the permission checks.  If
changing egid winds up dropping a group from the list of the process's
groups, the process could also have dropped that group with setresgid.
So I don't think we need to be concerned about the combination of
dropping egid and brpm->unsafe.

If anyone sees a hole in that logic I am happy to change the check
to !gid_eq(new->egid, old->egid), but I just can't see a way changing
egid/fsgid to a group the process already has is a problem.

>> -       if ((is_setid || __cap_gained(permitted, new, old)) &&
>> +       if ((id_changed || __cap_gained(permitted, new, old)) &&
>>             ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
>>              !ptracer_capable(current, new->user_ns))) {
>>                 /* downgrade; they get no more than they had, and maybe less */

Eric