On Sat, May 09, 2020 at 02:42:23PM -0500, Eric W. Biederman wrote:
>
> Add a flag preserve_creds that binfmt_misc can set to prevent
> credentials from being updated. This allows binfmrt_misc to always
> call prepare_binfmt. Allowing the credential computation logic to be
> consolidated.
>
> Ref: c407c033de84 ("[PATCH] binfmt_misc: improve calculation of interpreter's
> credentials")
> Signed-off-by: "Eric W. Biederman" <ebied...@xmission.com>
> ---
> fs/binfmt_misc.c | 15 +++------------
> fs/exec.c | 14 +++++++++-----
> include/linux/binfmts.h | 2 ++
> 3 files changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index 127fae9c21ab..16bfafd2671d 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -218,19 +218,10 @@ static int load_misc_binary(struct linux_binprm *bprm)
> goto error;
>
> bprm->file = interp_file;
> - if (fmt->flags & MISC_FMT_CREDENTIALS) {
> - loff_t pos = 0;
> -
> - /*
> - * No need to call prepare_binprm(), it's already been
> - * done. bprm->buf is stale, update from interp_file.
> - */
> - memset(bprm->buf, 0, BINPRM_BUF_SIZE);
> - retval = kernel_read(bprm->file, bprm->buf, BINPRM_BUF_SIZE,
> - &pos);
> - } else
> - retval = prepare_binprm(bprm);
> + if (fmt->flags & MISC_FMT_CREDENTIALS)
> + bprm->preserve_creds = 1;
>
> + retval = prepare_binprm(bprm);
> if (retval < 0)
> goto error;
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 8bbf5fa785a6..01dbeb025c46 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1630,14 +1630,18 @@ static void bprm_fill_uid(struct linux_binprm *bprm)
> */
> int prepare_binprm(struct linux_binprm *bprm)
> {
> - int retval;
> loff_t pos = 0;
>
> - bprm_fill_uid(bprm);
> + if (!bprm->preserve_creds) {
nit: hint this to the common execution path:
if (likely(!bprm->preserve_creds) {
> + int retval;
>
> - retval = cap_bprm_set_creds(bprm);
> - if (retval)
> - return retval;
> + bprm_fill_uid(bprm);
> +
> + retval = cap_bprm_set_creds(bprm);
> + if (retval)
> + return retval;
> + }
> + bprm->preserve_creds = 0;
>
> memset(bprm->buf, 0, BINPRM_BUF_SIZE);
> return kernel_read(bprm->file, bprm->buf, BINPRM_BUF_SIZE, &pos);
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 89f1135dcb75..cb016f001e7a 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -26,6 +26,8 @@ struct linux_binprm {
> unsigned long p; /* current top of mem */
> unsigned long argmin; /* rlimit marker for copy_strings() */
> unsigned int
> + /* Don't update the creds for an interpreter (see binfmt_misc)
> */
I'd like a much more verbose comment here. How about this:
/*
* Skip setting new privileges for an interpreter (see
* binfmt_misc) on the next call to prepare_binprm().
*/
> + preserve_creds:1,
Nit pick: we've seen there is a logical difference here between "creds"
(which mean "the creds struct itself") and "privileges" (which are
stored in the cred struct). I think we should reinforce this distinction
here and name this:
preserve_privileges:1,
> /*
> * True if most recent call to the commoncaps bprm_set_creds
> * hook (due to multiple prepare_binprm() calls from the
> --
> 2.25.0
>
Otherwise, yeah, this seems okay to me.
--
Kees Cook
