Re: [PATCH v2 3/8] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds

Kees Cook [off-list ref] writes:

On Tue, May 19, 2020 at 02:03:23PM -0500, Eric W. Biederman wrote:

quoted

Kees Cook [off-list ref] writes:

quoted

On Mon, May 18, 2020 at 07:31:14PM -0500, Eric W. Biederman wrote:

quoted

[...]

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).
		 */

Well it is not during but after the call that it becomes true.
I think most recent covers the case of multiple calls.

I'm thinking of an LSM writing reading these comments to decide what
they need to do to the flags, so it's a direction to them to set it to
true if they have determined that privilege was gained. (Though in
theory, this is all moot since only the commoncap hook cares.)

The comments for an LSM writer are in include/linux/lsm_hooks.h

 * @bprm_repopulate_creds:
 *	Assuming that the relevant bits of @bprm->cred->security have been
 *	previously set, examine @bprm->file and regenerate them.  This is
 *	so that the credentials derived from the interpreter the code is
 *	actually going to run are used rather than credentials derived
 *	from a script.  This done because the interpreter binary needs to
 *	reopen script, and may end up opening something completely different.
 *	This hook may also optionally check permissions (e.g. for
 *	transitions between security domains).
 *	The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be set to
 *	request libc enable secure mode.
 *	@bprm contains the linux_binprm structure.
 *	Return 0 if the hook is successful and permission is granted.

I hope that is detailed enough.

I will leave the rest of the comments for the maintainer of the code.

I really don't think we should duplicate the prescriptive comments in
multiple locations.

Eric