Re: [PATCH 1/3] LSM: add security_bprm_aborting_creds() hook

[Eric W. Biederman]

Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> writes:

> A regression caused by commit 978ffcbf00d8 ("execve: open the executable
> file before doing anything else") has been fixed by commit 4759ff71f23e
> ("exec: Check __FMODE_EXEC instead of in_execve for LSMs") and commit
> 3eab830189d9 ("uselib: remove use of __FMODE_EXEC"). While fixing this
> regression, Linus commented that we want to remove current->in_execve flag.
>
> The current->in_execve flag was introduced by commit f9ce1f1cda8b ("Add
> in_execve flag into task_struct.") when TOMOYO LSM was merged, and the
> reason was explained in commit f7433243770c ("LSM adapter functions.").
>
> In short, TOMOYO's design is not compatible with COW credential model
> introduced in Linux 2.6.29, and the current->in_execve flag was added for
> emulating security_bprm_free() hook which has been removed by introduction
> of COW credential model.

How is it not compatible?  Especially how is TOMOYO's design not
compatible with how things are today?

The discussion talks about not allowing reading of executables by programs
that can exec them.

At this point with __FMODE_EXEC being placed on the files for exec,
and with only execve using that mode all of your considerations should
be resolved.

So it appears to me that Tomoyo is currently compatible with COW
credentials even if it was not historically.

As such can we get a cleanup to actually make Tomoyo compatible.
Otherwise because Tomoyo is the only use of whatever you are doing
it will continue to be very easy to break Tomoyo.

The fact that somewhere Tomoyo is modifying a credential that the rest
of the kernel sees as read-only, and making it impossible to just
restore that credential is very concerning from a maintenance
perspective.

Can't Tomoyo simply allow reading of files that have __FMODE_EXEC
set when allow_execve is set, without needing to perform a domain
transition, and later back out that domain transition?


>  include/linux/security.h      |  5 +++++
>  security/security.c           | 14 ++++++++++++++
>  4 files changed, 21 insertions(+)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index af4fbb61cd53..9d198cd9a75c 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1519,6 +1519,7 @@ static void free_bprm(struct linux_binprm *bprm)
>  	}
>  	free_arg_pages(bprm);
>  	if (bprm->cred) {
> +		security_bprm_aborting_creds(bprm);
>  		mutex_unlock(&current->signal->cred_guard_mutex);
>  		abort_creds(bprm->cred);

Why isn't abort_creds calling security_free_cred enough here?
>  	}

Eric