[PATCH 14/27] LSM: Specify which LSM to display

[Kees Cook]

On Fri, Jul 26, 2019 at 04:39:10PM -0700, Casey Schaufler wrote:
> When a program is executed in a way that changes its privilege
> the display is reset to the initial state to prevent unprivileged
> programs from tricking it into setting an unexpected display.
> [...]
> diff --git a/security/security.c b/security/security.c
> index 8927508b2142..4dd4ebeda18d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -835,7 +857,18 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
>  
>  int security_bprm_set_creds(struct linux_binprm *bprm)
>  {
> -	return call_int_hook(bprm_set_creds, 0, bprm);
> +	int *disp = current->security;
> +	int rc;
> +
> +	rc = call_int_hook(bprm_set_creds, 0, bprm);
> +
> +	/*
> +	 * Reset the "display" LSM if privilege has been elevated.
> +	 */
> +	if (bprm->cap_elevated && disp)
> +		*disp = LSMBLOB_INVALID;
> +
> +	return rc;
>  }

I think this is the wrong place to check this. This is called in
prepare_binprm(), which is very early in the execve() process. By my
reading this will change the forked process's display first before the
exec happens (which may potentially fail) -- this needs to be changing
the final state once exec is under way (past the "point of no return" in
flush_old_exec()).

Also, the consolidation of privilege information happens into
bprm->secureexec in setup_new_exec(), so I think you want to test
secureexec not just cap_elevated.

So the test/clear should likely happen in finalize_exec() since it's a
runtime state, not a memory layout-changing state (which would need to
happen earlier).

-- 
Kees Cook