lkml.org 
[lkml]   [2020]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 3/8] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds
    On Mon, May 18, 2020 at 07:31:14PM -0500, Eric W. Biederman wrote:
    >
    > Rename bprm->cap_elevated to bprm->active_secureexec and initialize it
    > in prepare_binprm instead of in cap_bprm_set_creds. Initializing
    > bprm->active_secureexec in prepare_binprm allows multiple
    > implementations of security_bprm_repopulate_creds to play nicely with
    > each other.
    >
    > Rename security_bprm_set_creds to security_bprm_reopulate_creds to
    > emphasize that this path recomputes part of bprm->cred. This
    > recomputation avoids the time of check vs time of use problems that
    > are inherent in unix #! interpreters.
    >
    > In short two renames and a move in the location of initializing
    > bprm->active_secureexec.

    I like this much better than the direct call to the capabilities hook.
    Thanks!

    Reviewed-by: Kees Cook <keescook@chromium.org>

    One nit is a bikeshed on the name "active_secureexec", since
    the word "active" isn't really associated with any other part of the
    binfmt logic. It's supposed to be "latest state from the binfmt loop",
    so instead of "active", I considered these words that I also didn't
    like: "current", "this", "recent", and "now". Is "latest" better than
    "active"? Probably not.

    > [...]
    > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
    > index d1217fcdedea..8605ab4a0f89 100644
    > --- a/include/linux/binfmts.h
    > +++ b/include/linux/binfmts.h
    > @@ -27,10 +27,10 @@ struct linux_binprm {
    > unsigned long argmin; /* rlimit marker for copy_strings() */
    > unsigned int
    > /*
    > - * True if most recent call to cap_bprm_set_creds
    > + * True if most recent call to security_bprm_set_creds
    > * resulted in elevated privileges.
    > */
    > - cap_elevated:1,
    > + active_secureexec:1,

    Also, I'd like it if this comment could be made more verbose as well, for
    anyone trying to understand the binfmt execution flow for the first time.
    Perhaps:

    /*
    * Must be set True during the any call to
    * bprm_set_creds hook where the execution would
    * reuslt in elevated privileges. (The hook can be
    * called multiple times during nested interpreter
    * resolution across binfmt_script, binfmt_misc, etc).
    */


    --
    Kees Cook

    \
     
     \ /
      Last update: 2020-05-19 20:22    [W:4.020 / U:0.380 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site