From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
To: "Eric W. Biederman" <ebiederm@xmission.com>
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>
Subject: Re: [PATCH 1/3] LSM: add security_bprm_aborting_creds() hook
Date: Tue, 30 Jan 2024 19:42:59 +0900 [thread overview]
Message-ID: <56432241-1947-4701-a3d1-febd57fb3096@I-love.SAKURA.ne.jp> (raw)
In-Reply-To: <8734ug9fbt.fsf@email.froward.int.ebiederm.org>
On 2024/01/30 3:15, Eric W. Biederman wrote:
> 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.
Fine for me. The current argument is redundant, for nobody will try to
call security_execve_abort() on a remote thread.
>
>
> 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.
current->in_execve flag has two purposes: "whether to check permission" and
"what domain is used for checking permission (if need to check permission)".
One is to distinguish "open from execve()" and "open from uselib()".
This was replaced by the (file->f_mode & __FMODE_EXEC) change, for
__FMODE_EXEC was now removed from uselib(). But this is after all about
"whether to check permission".
The other is to emulate security_execve_abort(). security_execve_abort() is
needed because TOMOYO checks permission for opening interpreter file from
execve() using a domain which the current thread will belong to if execve()
succeeds (whereas DAC checks permission for opening interpreter file from
execve() using credentials which the current thread is currently using).
This is about "what domain is used for checking permission".
Since security_file_open() hook cannot see bprm->cred, TOMOYO cannot know
"what domain is used for checking permission" from security_file_open().
TOMOYO can know only "whether to check permission" from security_file_open().
Since TOMOYO cannot pass bprm->cred to security_file_open() hook using
override_creds()/revert_creds(), TOMOYO is passing "what domain is used for
checking permission" to security_file_open() via "struct task_struct"->security.
"struct task_struct"->security is updated _before_ security_file_open() for the
interpreter file is called.
Since security_execve_abort() was missing, when execve() failed, TOMOYO had
to keep the domain which the current thread would belong to if execve() succeeded.
The kept domain is cleared when TOMOYO finds that previous execve() was finished
(indicated by current->in_execve == 0) or when TOMOYO finds that new execve() is
in progress (indicated by current->in_execve == 0 when security_cred_prepare() is
called).
It is not possible to extract "what domain is used for checking permission" from
"whether file->f_mode includes __FMODE_EXEC". Talking about the
(file->f_mode & __FMODE_EXEC) change (i.e. "whether to check permission") is
pointless when talking about "what domain is used for checking permission".
next prev parent reply other threads:[~2024-01-30 10:44 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-28 14:16 [PATCH 0/3] fs/exec: remove current->in_execve flag Tetsuo Handa
2024-01-28 14:16 ` [PATCH 1/3] LSM: add security_bprm_aborting_creds() hook Tetsuo Handa
2024-01-29 4:10 ` Eric W. Biederman
2024-01-29 4:46 ` Tetsuo Handa
2024-01-29 18:15 ` Eric W. Biederman
2024-01-30 10:42 ` Tetsuo Handa [this message]
2024-01-28 14:17 ` [PATCH 2/3] tomoyo: replace current->in_execve flag with " Tetsuo Handa
2024-01-28 14:17 ` [PATCH 3/3] fs/exec: remove current->in_execve flag Tetsuo Handa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56432241-1947-4701-a3d1-febd57fb3096@I-love.SAKURA.ne.jp \
--to=penguin-kernel@i-love.sakura.ne.jp \
--cc=brauner@kernel.org \
--cc=ebiederm@xmission.com \
--cc=jack@suse.cz \
--cc=jmorris@namei.org \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=serge@hallyn.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).