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

[Kees Cook <kees@kernel.org>]

On June 13, 2025 8:28:46 AM PDT, Paul Moore <paul@paul-moore.com> wrote:
>On Thu, Jun 12, 2025 at 9:48 PM Kees Cook <kees@kernel.org> wrote:
>> On June 12, 2025 2:26:26 PM PDT, "Serge E. Hallyn" <serge@hallyn.com> 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@google.com> wrote:
>> >> > On Wed, May 21, 2025 at 5:27 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>> >> > > 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.
>> >> >
>> >> > 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. :)
>
>I'm not going to argue either way on this, that's between you and
>Serge, but as the entire patch is located within commoncap.c and that
>is part of the capabilities code which Serge maintains, can you
>explain why this should go via the execve/binfmt tree and not the
>capabilities tree?


Well, it's code exercised exclusively by the execve syscall (even the subject prefix says "exec"), but really I just want to be aware of changes in this area. How about we do what we're doing in vma, which is to list specific files in both entries in the MAINTAINERS file?

Anyway, sure, Serge, go ahead and take it. :)

Acked-by: Kees Cook <kees@kernel.org>


-- 
Kees Cook