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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHC9VhRPUXwqLvo4rbxL0++5zqHXfD8_tr-sirTJXdF_Aba_UQ@mail.gmail.com>
Date: Tue, 10 Jun 2025 20:18:56 -0400
From: Paul Moore <paul@...l-moore.com>
To: Jann Horn <jannh@...gle.com>
Cc: "Eric W. Biederman" <ebiederm@...ssion.com>, Richard Guy Briggs <rgb@...hat.com>, 
	"Serge E. Hallyn" <serge@...lyn.com>, Kees Cook <kees@...nel.org>, 
	Max Kellermann <max.kellermann@...os.com>, jmorris@...ei.org, 
	Andy Lutomirski <luto@...nel.org>, morgan@...nel.org, 
	Christian Brauner <christian@...uner.io>, linux-security-module@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] exec: Correct the permission check for unsafe exec

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

-- 
paul-moore.com