On Wed, May 21, 2025 at 5:27 PM Eric W. Biederman [off-list ref] wrote:

Jann Horn [off-list ref] writes:

quoted

On Wed, May 21, 2025 at 12:13 AM Eric W. Biederman
[off-list ref] wrote:

quoted

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>

quoted

@@ -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.