Re: [PATCH v3 1/3] LSM: add security_execve_abort() hook

[Paul Moore <paul@paul-moore.com>]

On Wed, Feb 7, 2024 at 9:34 AM Kees Cook <keescook@chromium.org> wrote:
> On Wed, Feb 07, 2024 at 08:10:55PM +0900, Tetsuo Handa wrote:
> > On 2024/02/07 9:00, Paul Moore wrote:
> > >> @@ -1223,6 +1223,17 @@ void security_bprm_committed_creds(const struct linux_binprm *bprm)
> > >>    call_void_hook(bprm_committed_creds, bprm);
> > >>  }
> > >>
> > >> +/**
> > >> + * security_execve_abort() - Notify that exec() has failed
> > >> + *
> > >> + * This hook is for undoing changes which cannot be discarded by
> > >> + * abort_creds().
> > >> + */
> > >> +void security_execve_abort(void)
> > >> +{
> > >> +  call_void_hook(execve_abort);
> > >> +}
> > >
> > > I don't have a problem with reinstating something like
> > > security_bprm_free(), but I don't like the name security_execve_abort(),
> > > especially given that it is being called from alloc_bprm() as well as
> > > all of the execve code.  At the risk of bikeshedding this, I'd be much
> > > happier if this hook were renamed to security_bprm_free() and the
> > > hook's description explained that this hook is called when a linux_bprm
> > > instance is being destroyed, after the bprm creds have been released,
> > > and is intended to cleanup any internal LSM state associated with the
> > > linux_bprm instance.
> > >
> > > Are you okay with that?
> >
> > Hmm, that will bring us back to v1 of this series.
> >
> > v3 was based on Eric W. Biederman's suggestion
> >
> >   If you aren't going to change your design your new hook should be:
> >           security_execve_revert(current);
> >   Or maybe:
> >           security_execve_abort(current);
> >
> >   At least then it is based upon the reality that you plan to revert
> >   changes to current->security.  Saying anything about creds or bprm when
> >   you don't touch them, makes no sense at all.  Causing people to
> >   completely misunderstand what is going on, and making it more likely
> >   they will change the code in ways that will break TOMOYO.
> >
> > at https://lkml.kernel.org/r/8734ug9fbt.fsf@email.froward.int.ebiederm.org .
>
> Yeah, I'd agree with Eric on this: it's not about bprm freeing, it's
> catching the execve failure. I think the name is accurate -- it mirrors
> the abort_creds() call.

I'm sorry, but I would still much rather prefer security_bprm_free()
both to reflect the nature of the caller as well as to abstract away a
particular use case; this is also why I suggested a different hook
description for the function header block.

If you really want this to be focused on reverting the execvc changes
(I do agree with Eric about "revert" over "abort") then please move
this hook out of free_bprm() and into do_execveat_common() and
kernel_execve().

To quickly summarize, there are two paths forward that I believe are
acceptable from a LSM perspective, pick either one and send me an
updated patchset.

1. Rename the hook to security_bprm_free() and update the LSM hook
description as I mentioned earlier in this thread.

2. Rename the hook to security_execve_revert(), move it into the
execve related functions, and update the LSM hook description to
reflect that this hook is for reverting execve related changes to the
current task's internal LSM state beyond what is possible via the
credential hooks.

-- 
paul-moore.com