Received-SPF: pass (google.com: domain of linux-kernel+bounces-43297-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: "Eric W. Biederman" <ebiederm@xmission.com>
To: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,  Kees Cook
 <keescook@chromium.org>,  Christian Brauner <brauner@kernel.org>,  Jan
 Kara <jack@suse.cz>,  Paul Moore <paul@paul-moore.com>,  James Morris
 <jmorris@namei.org>,  "Serge E. Hallyn" <serge@hallyn.com>,
  linux-security-module@vger.kernel.org,  linux-fsdevel@vger.kernel.org,
  LKML <linux-kernel@vger.kernel.org>
References: <e938c37b-d615-4be4-a2da-02b904b7072f@I-love.SAKURA.ne.jp>
	<613a54d2-9508-4f87-a163-a25a77a101cd@I-love.SAKURA.ne.jp>
	<87frygbx04.fsf@email.froward.int.ebiederm.org>
	<dbf0ef61-355b-4dcb-8e51-9298cf847367@I-love.SAKURA.ne.jp>
Date: Mon, 29 Jan 2024 12:15:02 -0600
In-Reply-To: <dbf0ef61-355b-4dcb-8e51-9298cf847367@I-love.SAKURA.ne.jp>
	(Tetsuo Handa's message of "Mon, 29 Jan 2024 13:46:28 +0900")
Message-ID: <8734ug9fbt.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH 1/3] LSM: add security_bprm_aborting_creds() hook

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> On 2024/01/29 13:10, Eric W. Biederman wrote:
>>> @@ -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?
>
> Because security_cred_free() from put_cred_rcu() is called from RCU callback
> rather than from current thread doing execve().
> TOMOYO wants to restore attributes of current thread doing execve().

>
>> 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.
>
> TOMOYO does not use "struct cred"->security.
> TOMOYO uses only "struct task_struct"->security.
>
>   struct lsm_blob_sizes tomoyo_blob_sizes __ro_after_init = {
>       .lbs_task = sizeof(struct tomoyo_task),
>   };
>
> TOMOYO uses security_task_alloc() for allocating "struct task_struct"->security,
> security_task_free() for releasing "struct task_struct"->security,
> security_bprm_check() for updating "struct task_struct"->security,
> security_bprm_committed_creds() for erasing old "struct task_struct"->security,
> security_bprm_aborting_creds() for restoring old "struct task_struct"->security.
>
> Commit a6f76f23d297 ("CRED: Make execve() take advantage of copy-on-write
> credentials") made TOMOYO impossible to do above. current->in_execve flag was a
> hack for emulating security_bprm_aborting_creds() using security_prepare_creds().
>
>> 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?
>
> No. That does not match TOMOYO's design.
>
> allow_execve keyword does not imply "allow opening that file for non-execve() purpose".

Huh?  I was proposing using the allow_execve credential to allow opening
and reading the file for execve purpose.  So you don't need to perform
a domain transition early.

> Also, performing a domain transition before execve() reaches point of no return is
> the TOMOYO's design, but COW credentials does not allow such behavior.

My question is simple.  Why can't TOMOYO use the changing of credentials
in task->cred to perform the domain transition?  Why can't TOMOYO work
with the code in execve?

I don't see anything that fundamentally requires you to have the domain
transition early.  All I have seen so far is an assertion that you
are using task->security.  Is there anything except for the reading
of the executable that having the domain transition early allows?

My primary concern with TOMOYO being the odd man out, and using hooks
for purposes arbitrary purposes instead of purposes they are logically
designed to be used for?

If you aren't going to change your design your new hook should be:
	security_execve_revert(current);
Or maybe:
	security_execve_abort(current);

At least then it is based upon the reality that you plan to revert
changes to current->security.  Saying anything about creds or bprm when
you don't touch them, makes no sense at all.  Causing people to
completely misunderstand what is going on, and making it more likely
they will change the code in ways that will break TOMOYO.


What I understand from the documentation you provided about TOMOYO is:
- TOMOYO provides the domain transition early so that the executable
  can be read.
- TOMOYO did that because it could not detect reliably when a file
  was opened for execve and read for execve.

Am I wrong in my understanding?

If that understanding is correct, now that (file->f_mode & __FMODE_EXEC)
is a reliable indication of a file used exclusively for exec then it
should be possible to take advantage of the new information and get
TOMOYO and the rest of the execve playing nicely with each other without
having to add new hooks.


If the maintenance concerns are not enough please consider the situation
from an attack surface concern.  Any hacked together poorly maintained
surface is ripe bugs, and the exploitation of those bugs.  Sometimes
those bugs will break you in obvious ways, other times those bugs
will break you in overlooked and exploitable ways.

Eric