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
