On Mon, May 18, 2020 at 07:33:46PM -0500, Eric W. Biederman wrote:
>
> Most of the support for passing the file descriptor of an executable
> to an interpreter already lives in the generic code and in binfmt_elf.
> Rework the fields in binfmt_elf that deal with executable file
> descriptor passing to make executable file descriptor passing a first
> class concept.
>
> Move the fd_install from binfmt_misc into begin_new_exec after the new
> creds have been installed. This means that accessing the file through
> /proc/<pid>/fd/N is able to see the creds for the new executable
> before allowing access to the new executables files.
>
> Performing the install of the executables file descriptor after
> the point of no return also means that nothing special needs to
> be done on error. The exiting of the process will close all
> of it's open files.
>
> Move the would_dump from binfmt_misc into begin_new_exec right
> after would_dump is called on the bprm->file. This makes it
> obvious this case exists and that no nesting of bprm->file is
> currently supported.
>
> In binfmt_misc the movement of fd_install into generic code means
> that it's special error exit path is no longer needed.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
Yes, this is so much nicer. :) My head did spin a little between changing
the management of bprm->executable between this patch and the next,
but I'm okay now. ;)
Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
nits/thoughts below...
> [...]
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 8c7779d6bf19..653508b25815 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> [...]
> @@ -48,6 +51,7 @@ struct linux_binprm {
> unsigned int taso:1;
> #endif
> unsigned int recursion_depth; /* only for search_binary_handler() */
> + struct file * executable; /* Executable to pass to the interpreter */
> struct file * file;
> struct cred *cred; /* new credentials */
nit: can we fix the "* " stuff here? This should be *file and *executable.
> [...]
> @@ -69,10 +73,6 @@ struct linux_binprm {
> #define BINPRM_FLAGS_ENFORCE_NONDUMP_BIT 0
> #define BINPRM_FLAGS_ENFORCE_NONDUMP (1 << BINPRM_FLAGS_ENFORCE_NONDUMP_BIT)
>
> -/* fd of the binary should be passed to the interpreter */
> -#define BINPRM_FLAGS_EXECFD_BIT 1
> -#define BINPRM_FLAGS_EXECFD (1 << BINPRM_FLAGS_EXECFD_BIT)
> -
> /* filename of the binary will be inaccessible after exec */
> #define BINPRM_FLAGS_PATH_INACCESSIBLE_BIT 2
> #define BINPRM_FLAGS_PATH_INACCESSIBLE (1 << BINPRM_FLAGS_PATH_INACCESSIBLE_BIT)
nit: may as well renumber BINPRM_FLAGS_PATH_INACCESSIBLE_BIT to 1,
they're not UAPI. And, actually, nothing uses the *_BIT defines, so
probably the entire chunk of code could just be reduced to:
/* either interpreter or executable was unreadable */
#define BINPRM_FLAGS_ENFORCE_NONDUMP BIT(0)
/* filename of the binary will be inaccessible after exec */
#define BINPRM_FLAGS_PATH_INACCESSIBLE BIT(1)
Though frankly, I wonder if interp_flags could just be removed in favor
of two new bit members, especially since interp_data is gone:
+ /* Either interpreter or executable was unreadable. */
+ nondumpable:1;
+ /* Filename of the binary will be inaccessible after exec. */
+ path_inaccessible:1;
...
- unsigned interp_flags;
...etc
--
Kees Cook
