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

[Jann Horn]

On Wed, May 21, 2025 at 5:27 PM Eric W. Biederman <ebiederm at xmission.com> wrote:
> Jann Horn <jannh at google.com> writes:
>
> > On Wed, May 21, 2025 at 12:13 AM Eric W. Biederman
> > <ebiederm at 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 at 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.

I'm fine with leaving your patch as-is.