On Tue, Oct 5, 2021 at 6:59 PM Casey Schaufler [off-list ref] wrote:

On 10/5/2021 8:21 AM, Stephen Smalley wrote:

quoted

On Mon, Oct 4, 2021 at 8:27 PM Jann Horn [off-list ref] wrote:

quoted

On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler [off-list ref] wrote:

quoted

On 10/4/2021 3:28 PM, Jann Horn wrote:

quoted

You can't really attribute binder transactions to specific tasks that
are actually involved in the specific transaction, neither on the
sending side nor on the receiving side, because binder is built around
passing data through memory mappings. Memory mappings can be accessed
by multiple tasks, and even a task that does not currently have it
mapped could e.g. map it at a later time. And on top of that you have
the problem that the receiving task might also go through privileged
execve() transitions.

OK. I'm curious now as to why the task_struct was being passed to the
hook in the first place.

Probably because that's what most other LSM hooks looked like and the
authors/reviewers of the patch didn't realize that this model doesn't
really work for binder? FWIW, these hooks were added in commit
79af73079d75 ("Add security hooks to binder and implement the hooks
for SELinux."). The commit message also just talks about "processes".

Note that in the same code path (binder_transaction), sender_euid is
set from proc->tsk and security_ctx is based on proc->tsk. If we are
changing the hooks to operate on the opener cred, then presumably we
should be doing that for sender_euid and replace the
security_task_getsecid_obj() call with security_cred_getsecid()?

NB Mandatory Access Control doesn't allow uncontrolled delegation,
hence typically checks against the subject credential either at
delegation/transfer or use or both. That's true in other places too,
e.g. file_permission, socket_sendmsg.

Terrific. Now I'm even less convinced that either the proposed change
or the existing code make sense. It's also disturbing that the change
log claims that the reason for the change is fix a race condition when
in fact it changes the data being sent to the hook completely.

The race it's referring to is the one between
security_binder_transaction() (which checks for permission to send a
transaction and checks for delegation) and
security_task_getsecid_obj() (which tells the recipient what the
sender's security context is). (It's a good thing Paul noticed that
the v1 patch didn't actually change the security_task_getsecid_obj()
call... somehow I missed that.)