[v2,4/8] exec: Allow load_misc_binary to call prepare_binfmt unconditionally
diff mbox series

Message ID 87imgszrwo.fsf_-_@x220.int.ebiederm.org
State New
Headers show
Series
  • exec: Control flow simplifications
Related show

Commit Message

Eric W. Biederman May 19, 2020, 12:31 a.m. UTC
Add a flag preserve_creds that binfmt_misc can set to prevent
credentials from being updated.  This allows binfmt_misc to always
call prepare_binfmt.  Allowing the credential computation logic to be
consolidated.

Not replacing the credentials with the interpreters credentials is
safe because because an open file descriptor to the executable is
passed to the interpreter.   As the interpreter does not need to
reopen the executable it is guaranteed to see the same file that
exec sees.

Ref: c407c033de84 ("[PATCH] binfmt_misc: improve calculation of interpreter's credentials")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/binfmt_misc.c        | 15 +++------------
 fs/exec.c               | 19 ++++++++++++-------
 include/linux/binfmts.h |  2 ++
 3 files changed, 17 insertions(+), 19 deletions(-)

Comments

Kees Cook May 19, 2020, 6:27 p.m. UTC | #1
On Mon, May 18, 2020 at 07:31:51PM -0500, Eric W. Biederman wrote:
> 
> Add a flag preserve_creds that binfmt_misc can set to prevent
> credentials from being updated.  This allows binfmt_misc to always
> call prepare_binfmt.  Allowing the credential computation logic to be

typo: prepare_binprm()

> consolidated.
> 
> Not replacing the credentials with the interpreters credentials is
> safe because because an open file descriptor to the executable is
> passed to the interpreter.   As the interpreter does not need to
> reopen the executable it is guaranteed to see the same file that
> exec sees.

Yup, looks good. Note below on comment.

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

> [...]
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 8605ab4a0f89..dbb5614d62a2 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
> +		/* It is safe to use the creds of a script (see binfmt_misc) */
> +		preserve_creds:1,

How about:

		/*
		 * A binfmt handler will set this to True before calling
		 * prepare_binprm() if it is safe to reuse the previous
		 * credentials, based on bprm->file (see binfmt_misc).
		 */
Eric W. Biederman May 19, 2020, 7:08 p.m. UTC | #2
Kees Cook <keescook@chromium.org> writes:

> On Mon, May 18, 2020 at 07:31:51PM -0500, Eric W. Biederman wrote:
>> 
>> Add a flag preserve_creds that binfmt_misc can set to prevent
>> credentials from being updated.  This allows binfmt_misc to always
>> call prepare_binfmt.  Allowing the credential computation logic to be
>
> typo: prepare_binprm()

Thank you.

>> consolidated.
>> 
>> Not replacing the credentials with the interpreters credentials is
>> safe because because an open file descriptor to the executable is
>> passed to the interpreter.   As the interpreter does not need to
>> reopen the executable it is guaranteed to see the same file that
>> exec sees.
>
> Yup, looks good. Note below on comment.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
>> [...]
>> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
>> index 8605ab4a0f89..dbb5614d62a2 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
>> +		/* It is safe to use the creds of a script (see binfmt_misc) */
>> +		preserve_creds:1,
>
> How about:
>
> 		/*
> 		 * A binfmt handler will set this to True before calling
> 		 * prepare_binprm() if it is safe to reuse the previous
> 		 * credentials, based on bprm->file (see binfmt_misc).
> 		 */

I think that is more words saying less.

While I agree it might be better.  I don't see what your comment adds to
the understanding.  What do you see my comment not saying that is important?

Eric
Kees Cook May 19, 2020, 7:17 p.m. UTC | #3
On Tue, May 19, 2020 at 02:08:34PM -0500, Eric W. Biederman wrote:
> Kees Cook <keescook@chromium.org> writes:
> 
> > On Mon, May 18, 2020 at 07:31:51PM -0500, Eric W. Biederman wrote:
> >> [...]
> >> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> >> index 8605ab4a0f89..dbb5614d62a2 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
> >> +		/* It is safe to use the creds of a script (see binfmt_misc) */
> >> +		preserve_creds:1,
> >
> > How about:
> >
> > 		/*
> > 		 * A binfmt handler will set this to True before calling
> > 		 * prepare_binprm() if it is safe to reuse the previous
> > 		 * credentials, based on bprm->file (see binfmt_misc).
> > 		 */
> 
> I think that is more words saying less.
> 
> While I agree it might be better.  I don't see what your comment adds to
> the understanding.  What do you see my comment not saying that is important?

I think your comment is aimed at the consumer of preserve_creds (i.e.
the fs/exec.c code), whereas I think the comment should be directed at
a binfmt author, who wants to answer the question "why would I set this
flag?" Though I strongly hope we never have new binfmts. ;)

Patch
diff mbox series

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index cdb45829354d..264829745d6f 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 8e3b93d51d31..028e0e323af5 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1631,15 +1631,20 @@  static void bprm_fill_uid(struct linux_binprm *bprm)
  */
 int prepare_binprm(struct linux_binprm *bprm)
 {
-	int retval;
 	loff_t pos = 0;
 
-	/* Recompute parts of bprm->cred based on bprm->file */
-	bprm->active_secureexec = 0;
-	bprm_fill_uid(bprm);
-	retval = security_bprm_repopulate_creds(bprm);
-	if (retval)
-		return retval;
+	/* Can the interpreter get to the executable without races? */
+	if (!bprm->preserve_creds) {
+		int retval;
+
+		/* Recompute parts of bprm->cred based on bprm->file */
+		bprm->active_secureexec = 0;
+		bprm_fill_uid(bprm);
+		retval = security_bprm_repopulate_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 8605ab4a0f89..dbb5614d62a2 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
+		/* It is safe to use the creds of a script (see binfmt_misc) */
+		preserve_creds:1,
 		/*
 		 * True if most recent call to security_bprm_set_creds
 		 * resulted in elevated privileges.