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

[Kees Cook]

On June 12, 2025 2:26:26 PM PDT, "Serge E. Hallyn" <serge@xxxxxxxxxx> wrote:
>On Tue, Jun 10, 2025 at 08:18:56PM -0400, Paul Moore wrote:
>> On Wed, May 21, 2025 at 11:36 AM Jann Horn <jannh@xxxxxxxxxx> wrote:
>> > On Wed, May 21, 2025 at 5:27 PM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>> > > Jann Horn <jannh@xxxxxxxxxx> writes:
>> > >
>> > > > On Wed, May 21, 2025 at 12:13 AM Eric W. Biederman
>> > > > <ebiederm@xxxxxxxxxxxx> 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@xxxxxxxxxx>
>> > > >
>> > > >> @@ -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.
>> 
>> Aside from a tested-by verification from Max, it looks like everyone
>> is satisfied with the v2 patch, yes?
>> 
>> Serge, I see you've reviewed this patch, can I assume that now you
>> have a capabilities tree up and running you'll take this patch?
>
>I can take another look and consider taking it on Monday, but until
>then I'm effectively afk.

I'd rather this go via the execve/binfmt tree. I was waiting for -rc2 before putting it into -next. I can do Sunday night after it's out. :)

-- 
Kees Cook