Re: [PATCH 1/2] fs/exec: Explicitly unshare fs_struct on exec

["Eric W. Biederman" <ebiederm@xmission.com>]

Kees Cook <kees@kernel.org> writes:

> On Wed, May 14, 2025 at 02:03:31AM +0200, Mateusz Guzik wrote:
>> On Wed, May 14, 2025 at 12:17 AM Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>> >
>> > Jann Horn <jannh@google.com> writes:
>> >
>> > > On Tue, May 13, 2025 at 10:57 PM Kees Cook <kees@kernel.org> wrote:
>> > >> On May 13, 2025 6:05:45 AM PDT, Mateusz Guzik <mjguzik@gmail.com> wrote:
>> > >> >Here is my proposal: *deny* exec of suid/sgid binaries if fs_struct is
>> > >> >shared. This will have to be checked for after the execing proc becomes
>> > >> >single-threaded ofc.
>> > >>
>> > >> Unfortunately the above Chrome helper is setuid and uses CLONE_FS.
>> > >
>> > > Chrome first launches a setuid helper, and then the setuid helper does
>> > > CLONE_FS. Mateusz's proposal would not impact this usecase.
>> > >
>> > > Mateusz is proposing to block the case where a process first does
>> > > CLONE_FS, and *then* one of the processes sharing the fs_struct does a
>> > > setuid execve(). Linux already downgrades such an execve() to be
>> > > non-setuid, which probably means anyone trying to do this will get
>> > > hard-to-understand problems. Mateusz' proposal would just turn this
>> > > hard-to-debug edgecase, which already doesn't really work, into a
>> > > clean error; I think that is a nice improvement even just from the
>> > > UAPI standpoint.
>> > >
>> > > If this change makes it possible to clean up the kernel code a bit, even better.
>> >
>> > What has brought this to everyone's attention just now?  This is
>> > the second mention of this code path I have seen this week.
>> >
>> 
>> There is a syzkaller report concerning ->in_exec handling, for example:
>> https://lore.kernel.org/all/67dc67f0.050a0220.25ae54.001f.GAE@google.com/#t
>>
>> [...]
>> > It looks like most of the lsm's also test bprm->unsafe.
>> >
>> > So I imagine someone could very carefully separate the non-ptrace case
>> > from the ptrace case but *shrug*.
>> >
>> > Perhaps:
>> >
>> >         if ((is_setid || __cap_gained(permitted, new_old)) &&
>> >             ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
>> >              !ptracer_capable(current, new->user_ns))) {
>> > +               if (!(bprm->unsafe & LSM_UNSAFE_PTRACE)) {
>> > +                       return -EPERM;
>> > +               }
>> >                 /* downgrade; they get no more than they had, and maybe less */
>> >                 if (!ns_capable(new->user_ns, CAP_SETUID) ||
>> >                     (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)) {
>> >                         new->euid = new->uid;
>> >                         new->egid = new->gid;
>> >                 }
>> >                 new->cap_permitted = cap_intersect(new->cap_permitted,
>> >                                                    old->cap_permitted);
>> >          }
>> >
>> > If that is what you want that doesn't look to scary.  I don't think
>> > it simplifies anything about fs->in_exec.  As fs->in_exec is set when
>> > the processing calling exec is the only process that owns the fs_struct.
>> > With fs->in_exec just being a flag that doesn't allow another thread
>> > to call fork and start sharing the fs_struct during exec.
>> >
>> > *Shrug*
>> >
>> > I don't see why anyone would care.  It is just a very silly corner case.
>> 
>> Well I don't see how ptrace factors into any of this, apart from being
>> a different case of ignoring suid/sgid.
>
> I actually think we might want to expand the above bit of logic to use
> an explicit tests of each LSM_UNSAFE case -- the merged
> logic is very difficult to read currently. Totally untested expansion,
> if I'm reading everything correctly:
>
> 	if (bprm->unsafe &&
> 	    (is_setid || __cap_gained(permitted, new_old))) {
> 		bool limit_caps = false;
> 		bool strip_eid = false;
> 		unsigned int unsafe = bprm->unsafe;
> 		/* Check each bit */
>
> 		if (unsafe & LSM_UNSAFE_PTRACE) {
> 			if (!ptracer_capable(current, new->user_ns))
> 				limit_caps = true;
                                strip_eid  = true;
You missed the euid stripping there.
> 			unsafe &= ~LSM_UNSAFE_PTRACE;
> 		}
> 		if (unsafe & LSM_UNSAFE_SHARE) {
> 			limit_caps = true;
> 			if (!ns_capable(new->user_ns, CAP_SETUID))
> 				strip_eid = true;
> 			unsafe &= ~LSM_UNSAFE_SHARE;
> 		}
> 		if (unsafe & LSM_UNSAFE_NO_NEW_PRIVS) {
> 			limit_caps = true;
> 			if (!ns_capable(new->user_ns, CAP_SETUID))
> 				strip_eid = true;
> 			unsafe &= ~LSM_UNSAFE_NO_NEW_PRIVS;
> 		}
>
> 		if (WARN(unsafe, "Unhandled LSM_UNSAFE flag: %u?!\n", unsafe))
> 			return -EINVAL;
>
> 		if (limit_caps) {
> 			new->cap_permitted = cap_intersect(new->cap_permitted,
> 							   old->cap_permitted);
> 		}
> 		if (strip_eid) {
> 			new->euid = new->uid;
> 			new->egid = new->gid;
> 		}
> 	}


I think I would simplify this all to:

	if ((id_changed || __cap_gained(permitted, new, old)) &&
            !ptracer_capable(current->new_user_ns)) {
        	if (!ns_capable(new->user_ns, CAP_SETUID)) {
                	new->euid = old->euid;
                        new->egid = old->egid;
                }
                new->cap_permitted = cap_intersect(new->cap_permitted,
                				   old->cap_permitted);
        }
        if ((id_changed || __cap_gained(permitted, new, old)) &&
            (bprm->unsafe & ~LSM_UNSAFE_PTRACE)) {
        	return -EPERM;
        }

The code of no_new_privs doesn't prevent capset so ignoring the results
of ns_capable when NO_NEW_PRIVS is set doesn't make sense.

If we are going to do anything please let's find ways to understand
what is happening and simplify this code, not add to it's complexity.

Eric