[PATCH v2] binder: use cred instead of task for selinux checks

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

Fix by saving the 'struct cred' during binder_open and pass
it to the selinux subsystem.

Fixes: 79af73079d75 ("Add security hooks to binder and implement the
hooks for SELinux.")
Signed-off-by: Todd Kjos <redacted>
Cc: stable@vger.kernel.org # 5.14+ (need backport for earlier stables)
---
v2: updated comments as suggested by Paul Moore

 drivers/android/binder.c          | 14 +++++----
 drivers/android/binder_internal.h |  4 +++
 include/linux/lsm_hook_defs.h     | 14 ++++-----
 include/linux/lsm_hooks.h         | 14 ++++-----
 include/linux/security.h          | 28 +++++++++---------
 security/security.c               | 14 ++++-----
 security/selinux/hooks.c          | 48 +++++++++----------------------
 7 files changed, 60 insertions(+), 76 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 9edacc8b9768..ca599ebdea4a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c

@@ -2056,7 +2056,7 @@ static int binder_translate_binder(struct flat_binder_object *fp,
 		ret = -EINVAL;
 		goto done;
 	}
-	if (security_binder_transfer_binder(proc->tsk, target_proc->tsk)) {
+	if (security_binder_transfer_binder(proc->cred, target_proc->cred)) {
 		ret = -EPERM;
 		goto done;
 	}

@@ -2102,7 +2102,7 @@ static int binder_translate_handle(struct flat_binder_object *fp,
 				  proc->pid, thread->pid, fp->handle);
 		return -EINVAL;
 	}
-	if (security_binder_transfer_binder(proc->tsk, target_proc->tsk)) {
+	if (security_binder_transfer_binder(proc->cred, target_proc->cred)) {
 		ret = -EPERM;
 		goto done;
 	}

@@ -2190,7 +2190,7 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
 		ret = -EBADF;
 		goto err_fget;
 	}
-	ret = security_binder_transfer_file(proc->tsk, target_proc->tsk, file);
+	ret = security_binder_transfer_file(proc->cred, target_proc->cred, file);
 	if (ret < 0) {
 		ret = -EPERM;
 		goto err_security;

@@ -2595,8 +2595,8 @@ static void binder_transaction(struct binder_proc *proc,
 			return_error_line = __LINE__;
 			goto err_invalid_target_handle;
 		}
-		if (security_binder_transaction(proc->tsk,
-						target_proc->tsk) < 0) {
+		if (security_binder_transaction(proc->cred,
+						target_proc->cred) < 0) {
 			return_error = BR_FAILED_REPLY;
 			return_error_param = -EPERM;
 			return_error_line = __LINE__;

@@ -4353,6 +4353,7 @@ static void binder_free_proc(struct binder_proc *proc)
 	}
 	binder_alloc_deferred_release(&proc->alloc);
 	put_task_struct(proc->tsk);
+	put_cred(proc->cred);
 	binder_stats_deleted(BINDER_STAT_PROC);
 	kfree(proc);
 }

@@ -4564,7 +4565,7 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp,
 		ret = -EBUSY;
 		goto out;
 	}
-	ret = security_binder_set_context_mgr(proc->tsk);
+	ret = security_binder_set_context_mgr(proc->cred);
 	if (ret < 0)
 		goto out;
 	if (uid_valid(context->binder_context_mgr_uid)) {

@@ -5055,6 +5056,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
 	spin_lock_init(&proc->outer_lock);
 	get_task_struct(current->group_leader);
 	proc->tsk = current->group_leader;
+	proc->cred = get_cred(filp->f_cred);
 	INIT_LIST_HEAD(&proc->todo);
 	init_waitqueue_head(&proc->freeze_wait);
 	proc->default_priority = task_nice(current);

diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 402c4d4362a8..d6b6b8cb7346 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h

@@ -364,6 +364,9 @@ struct binder_ref {
  *                        (invariant after initialized)
  * @tsk                   task_struct for group_leader of process
  *                        (invariant after initialized)
+ * @cred                  struct cred associated with the `struct file`
+ *                        in binder_open()
+ *                        (invariant after initialized)
  * @deferred_work_node:   element for binder_deferred_list
  *                        (protected by binder_deferred_lock)
  * @deferred_work:        bitmap of deferred work to perform

@@ -426,6 +429,7 @@ struct binder_proc {
 	struct list_head waiting_threads;
 	int pid;
 	struct task_struct *tsk;
+	const struct cred *cred;
 	struct hlist_node deferred_work_node;
 	int deferred_work;
 	int outstanding_txns;

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 2adeea44c0d5..61590c1f2d33 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h

@@ -26,13 +26,13 @@
  *   #undef LSM_HOOK
  * };
  */
-LSM_HOOK(int, 0, binder_set_context_mgr, struct task_struct *mgr)
-LSM_HOOK(int, 0, binder_transaction, struct task_struct *from,
-	 struct task_struct *to)
-LSM_HOOK(int, 0, binder_transfer_binder, struct task_struct *from,
-	 struct task_struct *to)
-LSM_HOOK(int, 0, binder_transfer_file, struct task_struct *from,
-	 struct task_struct *to, struct file *file)
+LSM_HOOK(int, 0, binder_set_context_mgr, const struct cred *mgr)
+LSM_HOOK(int, 0, binder_transaction, const struct cred *from,
+	 const struct cred *to)
+LSM_HOOK(int, 0, binder_transfer_binder, const struct cred *from,
+	 const struct cred *to)
+LSM_HOOK(int, 0, binder_transfer_file, const struct cred *from,
+	 const struct cred *to, struct file *file)
 LSM_HOOK(int, 0, ptrace_access_check, struct task_struct *child,
 	 unsigned int mode)
 LSM_HOOK(int, 0, ptrace_traceme, struct task_struct *parent)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 5c4c5c0602cb..59024618554e 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h

@@ -1313,22 +1313,22 @@
  *
  * @binder_set_context_mgr:
  *	Check whether @mgr is allowed to be the binder context manager.
- *	@mgr contains the task_struct for the task being registered.
+ *	@mgr contains the struct cred for the current binder process.
  *	Return 0 if permission is granted.
  * @binder_transaction:
  *	Check whether @from is allowed to invoke a binder transaction call
  *	to @to.
- *	@from contains the task_struct for the sending task.
- *	@to contains the task_struct for the receiving task.
+ *	@from contains the struct cred for the sending process.
+ *	@to contains the struct cred for the receiving process.
  * @binder_transfer_binder:
  *	Check whether @from is allowed to transfer a binder reference to @to.
- *	@from contains the task_struct for the sending task.
- *	@to contains the task_struct for the receiving task.
+ *	@from contains the struct cred for the sending process.
+ *	@to contains the struct cred for the receiving process.
  * @binder_transfer_file:
  *	Check whether @from is allowed to transfer @file to @to.
- *	@from contains the task_struct for the sending task.
+ *	@from contains the struct cred for the sending process.
  *	@file contains the struct file being transferred.
- *	@to contains the task_struct for the receiving task.
+ *	@to contains the struct cred for the receiving process.
  *
  * @ptrace_access_check:
  *	Check permission before allowing the current process to trace the

diff --git a/include/linux/security.h b/include/linux/security.h
index 5b7288521300..6344d3362df7 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h

@@ -258,13 +258,13 @@ extern int security_init(void);
 extern int early_security_init(void);
 
 /* Security operations */
-int security_binder_set_context_mgr(struct task_struct *mgr);
-int security_binder_transaction(struct task_struct *from,
-				struct task_struct *to);
-int security_binder_transfer_binder(struct task_struct *from,
-				    struct task_struct *to);
-int security_binder_transfer_file(struct task_struct *from,
-				  struct task_struct *to, struct file *file);
+int security_binder_set_context_mgr(const struct cred *mgr);
+int security_binder_transaction(const struct cred *from,
+				const struct cred *to);
+int security_binder_transfer_binder(const struct cred *from,
+				    const struct cred *to);
+int security_binder_transfer_file(const struct cred *from,
+				  const struct cred *to, struct file *file);
 int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
 int security_ptrace_traceme(struct task_struct *parent);
 int security_capget(struct task_struct *target,

@@ -508,25 +508,25 @@ static inline int early_security_init(void)
 	return 0;
 }
 
-static inline int security_binder_set_context_mgr(struct task_struct *mgr)
+static inline int security_binder_set_context_mgr(const struct cred *mgr)
 {
 	return 0;
 }
 
-static inline int security_binder_transaction(struct task_struct *from,
-					      struct task_struct *to)
+static inline int security_binder_transaction(const struct cred *from,
+					      const struct cred *to)
 {
 	return 0;
 }
 
-static inline int security_binder_transfer_binder(struct task_struct *from,
-						  struct task_struct *to)
+static inline int security_binder_transfer_binder(const struct cred *from,
+						  const struct cred *to)
 {
 	return 0;
 }
 
-static inline int security_binder_transfer_file(struct task_struct *from,
-						struct task_struct *to,
+static inline int security_binder_transfer_file(const struct cred *from,
+						const struct cred *to,
 						struct file *file)
 {
 	return 0;

diff --git a/security/security.c b/security/security.c
index 9ffa9e9c5c55..67264cb08fb3 100644
--- a/security/security.c
+++ b/security/security.c

@@ -747,25 +747,25 @@ static int lsm_superblock_alloc(struct super_block *sb)
 
 /* Security operations */
 
-int security_binder_set_context_mgr(struct task_struct *mgr)
+int security_binder_set_context_mgr(const struct cred *mgr)
 {
 	return call_int_hook(binder_set_context_mgr, 0, mgr);
 }
 
-int security_binder_transaction(struct task_struct *from,
-				struct task_struct *to)
+int security_binder_transaction(const struct cred *from,
+				const struct cred *to)
 {
 	return call_int_hook(binder_transaction, 0, from, to);
 }
 
-int security_binder_transfer_binder(struct task_struct *from,
-				    struct task_struct *to)
+int security_binder_transfer_binder(const struct cred *from,
+				    const struct cred *to)
 {
 	return call_int_hook(binder_transfer_binder, 0, from, to);
 }
 
-int security_binder_transfer_file(struct task_struct *from,
-				  struct task_struct *to, struct file *file)
+int security_binder_transfer_file(const struct cred *from,
+				  const struct cred *to, struct file *file)
 {
 	return call_int_hook(binder_transfer_file, 0, from, to, file);
 }

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e7ebd45ca345..c8bf3db90c8b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c

@@ -255,29 +255,6 @@ static inline u32 task_sid_obj(const struct task_struct *task)
 	return sid;
 }
 
-/*
- * get the security ID of a task for use with binder
- */
-static inline u32 task_sid_binder(const struct task_struct *task)
-{
-	/*
-	 * In many case where this function is used we should be using the
-	 * task's subjective SID, but we can't reliably access the subjective
-	 * creds of a task other than our own so we must use the objective
-	 * creds/SID, which are safe to access.  The downside is that if a task
-	 * is temporarily overriding it's creds it will not be reflected here;
-	 * however, it isn't clear that binder would handle that case well
-	 * anyway.
-	 *
-	 * If this ever changes and we can safely reference the subjective
-	 * creds/SID of another task, this function will make it easier to
-	 * identify the various places where we make use of the task SIDs in
-	 * the binder code.  It is also likely that we will need to adjust
-	 * the main drivers/android binder code as well.
-	 */
-	return task_sid_obj(task);
-}
-
 static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dentry);
 
 /*

@@ -2066,18 +2043,19 @@ static inline u32 open_file_to_av(struct file *file)
 
 /* Hook functions begin here. */
 
-static int selinux_binder_set_context_mgr(struct task_struct *mgr)
+static int selinux_binder_set_context_mgr(const struct cred *mgr)
 {
 	return avc_has_perm(&selinux_state,
-			    current_sid(), task_sid_binder(mgr), SECCLASS_BINDER,
+			    current_sid(), cred_sid(mgr), SECCLASS_BINDER,
 			    BINDER__SET_CONTEXT_MGR, NULL);
 }
 
-static int selinux_binder_transaction(struct task_struct *from,
-				      struct task_struct *to)
+static int selinux_binder_transaction(const struct cred *from,
+				      const struct cred *to)
 {
 	u32 mysid = current_sid();
-	u32 fromsid = task_sid_binder(from);
+	u32 fromsid = cred_sid(from);
+	u32 tosid = cred_sid(to);
 	int rc;
 
 	if (mysid != fromsid) {

@@ -2088,24 +2066,24 @@ static int selinux_binder_transaction(struct task_struct *from,
 			return rc;
 	}
 
-	return avc_has_perm(&selinux_state, fromsid, task_sid_binder(to),
+	return avc_has_perm(&selinux_state, fromsid, tosid,
 			    SECCLASS_BINDER, BINDER__CALL, NULL);
 }
 
-static int selinux_binder_transfer_binder(struct task_struct *from,
-					  struct task_struct *to)
+static int selinux_binder_transfer_binder(const struct cred *from,
+					  const struct cred *to)
 {
 	return avc_has_perm(&selinux_state,
-			    task_sid_binder(from), task_sid_binder(to),
+			    cred_sid(from), cred_sid(to),
 			    SECCLASS_BINDER, BINDER__TRANSFER,
 			    NULL);
 }
 
-static int selinux_binder_transfer_file(struct task_struct *from,
-					struct task_struct *to,
+static int selinux_binder_transfer_file(const struct cred *from,
+					const struct cred *to,
 					struct file *file)
 {
-	u32 sid = task_sid_binder(to);
+	u32 sid = cred_sid(to);
 	struct file_security_struct *fsec = selinux_file(file);
 	struct dentry *dentry = file->f_path.dentry;
 	struct inode_security_struct *isec;

-- 
2.33.0.800.g4c38ced690-goog

On 10/1/2021 10:55 AM, Todd Kjos wrote:

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

I understand that there are no users of the binder driver
other than SELinux in Android upstream today. That's not
the issue. Two processes on a system with SELinux and AppArmor
together may be required to provide incompatible results
from security_secid_to_secctx()/security_secctx_to_secid().
If it's impossible to detect this incompatibility it's
impossible to prevent serious confusion.

The LSM stacking isn't upstream yet. But I hope to have it
there Real Soon Now. If there's another way to fix this that
doesn't remove the task_struct it would avoid my having to
put it back.

Thank you.

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

Why not? Why can't you put the security identity of the task into the creds?

SELinux already identifies tasks through their creds (see e.g.
task_sid_obj()), and doesn't use the task security blob at all.
Apparmor also identifies tasks through their creds (see
aa_current_raw_label() and __aa_task_raw_label()), and just uses the
task blob to store information about other labels that the process may
transition from or to.

From what I can tell, the only LSM that actually identifies the
caller's security context through the task security blob is Tomoyo. As
far as I know, that means Tomoyo is kinda broken. (But does anyone
even use Tomoyo?)

I understand that there are no users of the binder driver
other than SELinux in Android upstream today. That's not
the issue. Two processes on a system with SELinux and AppArmor
together may be required to provide incompatible results
from security_secid_to_secctx()/security_secctx_to_secid().
If it's impossible to detect this incompatibility it's
impossible to prevent serious confusion.

The LSM stacking isn't upstream yet. But I hope to have it
there Real Soon Now. If there's another way to fix this that
doesn't remove the task_struct it would avoid my having to
put it back.

You fundamentally can't identify the recipient of a binder transaction
through its task_struct, because the recipient might have given the
binder FD to a child process and executed a setuid binary since it
opened /dev/binder. If you look at the credentials of the task on the
other side, you'll just see the setuid binary that doesn't even know
it has an open binder FD, and won't see the child process that is
actually going to receive the transaction.

You can't even usefully identify the opener of a file through its
task_struct - especially with io_uring, any userspace process can
cause kernel threads to open files and perform I/O on them *on behalf
of userspace* - and this "on behalf of" relationship is only visible
in the overridden credentials. (And yes, I do think that means Tomoyo
doesn't work properly on systems with io_uring.)

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn [off-list ref] wrote:

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

Why not? Why can't you put the security identity of the task into the creds?

Ah, I get it now, you're concerned about different processes wanting
to see security contexts formatted differently (e.g. printing the
SELinux label vs printing the AppArmor label), right?

But still, I don't think you can pull that information from the
receiving task. Maybe the easiest solution would be to also store that
in the creds? Or you'd have to manually grab that information when
/dev/binder is opened.

On 10/1/2021 12:50 PM, Jann Horn wrote:

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn [off-list ref] wrote:

quoted

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

Why not? Why can't you put the security identity of the task into the creds?

Ah, I get it now, you're concerned about different processes wanting
to see security contexts formatted differently (e.g. printing the
SELinux label vs printing the AppArmor label), right?

That is correct.

But still, I don't think you can pull that information from the
receiving task. Maybe the easiest solution would be to also store that
in the creds? Or you'd have to manually grab that information when
/dev/binder is opened.

I'm storing the information in the task security blob because that's
the appropriate scope. Today the LSM hook is given both task_struct's.
It's easy to compare to make sure the tasks are compatible. Adding the
information to the cred would be yet another case where the scope of
security information is wrong. 

On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler [off-list ref] wrote:

On 10/1/2021 12:50 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn [off-list ref] wrote:

quoted

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

Why not? Why can't you put the security identity of the task into the creds?

Ah, I get it now, you're concerned about different processes wanting
to see security contexts formatted differently (e.g. printing the
SELinux label vs printing the AppArmor label), right?

That is correct.

quoted

But still, I don't think you can pull that information from the
receiving task. Maybe the easiest solution would be to also store that
in the creds? Or you'd have to manually grab that information when
/dev/binder is opened.

I'm storing the information in the task security blob because that's
the appropriate scope. Today the LSM hook is given both task_struct's.

Which is wrong, because you have no idea who the semantic "recipient
task" is - any task that has a mapping of the binder fd can
effectively receive transactions from it.

(And the current "sender task" is also wrong, because binder looks at
the task that opened the binder device, not the task currently
performing the action.)

It's easy to compare to make sure the tasks are compatible.

It would be, if you actually had a pair of tasks that accurately
represent the sender and the recipient.

Adding the
information to the cred would be yet another case where the scope of
security information is wrong.

Can you elaborate on why you think that?

On 10/1/2021 3:58 PM, Jann Horn wrote:

On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 12:50 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn [off-list ref] wrote:

quoted

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

Why not? Why can't you put the security identity of the task into the creds?

Ah, I get it now, you're concerned about different processes wanting
to see security contexts formatted differently (e.g. printing the
SELinux label vs printing the AppArmor label), right?

That is correct.

quoted

But still, I don't think you can pull that information from the
receiving task. Maybe the easiest solution would be to also store that
in the creds? Or you'd have to manually grab that information when
/dev/binder is opened.

I'm storing the information in the task security blob because that's
the appropriate scope. Today the LSM hook is given both task_struct's.

Which is wrong, because you have no idea who the semantic "recipient
task" is - any task that has a mapping of the binder fd can
effectively receive transactions from it.

(And the current "sender task" is also wrong, because binder looks at
the task that opened the binder device, not the task currently
performing the action.)

I'm confused. Are you saying that the existing binder code is
completely broken? Are you saying that neither "task" is correct?
How does passing the creds from the wrong tasks "fix" the problem?

quoted

It's easy to compare to make sure the tasks are compatible.

It would be, if you actually had a pair of tasks that accurately
represent the sender and the recipient.

quoted

Adding the
information to the cred would be yet another case where the scope of
security information is wrong.

Can you elaborate on why you think that?

The information identifies how the task is going to display
the security "context". It isn't used in access checks.

On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler [off-list ref] wrote:

On 10/1/2021 3:58 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 12:50 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn [off-list ref] wrote:

quoted

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

Why not? Why can't you put the security identity of the task into the creds?

Ah, I get it now, you're concerned about different processes wanting
to see security contexts formatted differently (e.g. printing the
SELinux label vs printing the AppArmor label), right?

That is correct.

quoted

But still, I don't think you can pull that information from the
receiving task. Maybe the easiest solution would be to also store that
in the creds? Or you'd have to manually grab that information when
/dev/binder is opened.

I'm storing the information in the task security blob because that's
the appropriate scope. Today the LSM hook is given both task_struct's.

Which is wrong, because you have no idea who the semantic "recipient
task" is - any task that has a mapping of the binder fd can
effectively receive transactions from it.

(And the current "sender task" is also wrong, because binder looks at
the task that opened the binder device, not the task currently
performing the action.)

I'm confused. Are you saying that the existing binder code is
completely broken? Are you saying that neither "task" is correct?

Yeah, basically - but luckily the actual impact this has is limited by
the transitions that SELinux permits. If domain1 has no way to
transition to domain2, then it can't abuse this bug to pretend to be
domain2. I do have a reproducer that lets Android's "shell" domain
send a binder transaction that appears to come from "runas", but
luckily "runas" has no interesting privileges with regards to binder,
so that's not exploitable.

How does passing the creds from the wrong tasks "fix" the problem?

This patch is not passing the creds from the "wrong" tasks at all. It
relies on the basic idea that when a security context opens a
resource, and then hands that resource to another context for
read/write operations, then you can effectively treat this as a
delegation of privileges from the original opener, and perform access
checks against the credentials using which the resource was opened.

In particular, we already have those semantics in the core kernel for
->read() and ->write() VFS operations - they are *not allowed* to look
at the credentials of the caller, and if they want to make security
checks, they have to instead check against file->f_cred, which are the
credentials using which the file was originally opened. (Yes, some
places still get that wrong.) Passing a file descriptor to another
task is a delegation of access, and the other task can then call
syscalls like read() / write() / mmap() on the file descriptor without
needing to have any access to the underlying file.

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.

quoted

quoted

It's easy to compare to make sure the tasks are compatible.

It would be, if you actually had a pair of tasks that accurately
represent the sender and the recipient.

quoted

Adding the
information to the cred would be yet another case where the scope of
security information is wrong.

Can you elaborate on why you think that?

The information identifies how the task is going to display
the security "context". It isn't used in access checks.

But it is data that AFAICS needs to be preserved in the same places
where the creds need to be preserved, and it is also related to
security labels, so isn't "struct cred" a logical place to stuff it
anyway?

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

On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 3:58 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 12:50 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn [off-list ref] wrote:

quoted

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

Why not? Why can't you put the security identity of the task into the creds?

Ah, I get it now, you're concerned about different processes wanting
to see security contexts formatted differently (e.g. printing the
SELinux label vs printing the AppArmor label), right?

That is correct.

quoted

But still, I don't think you can pull that information from the
receiving task. Maybe the easiest solution would be to also store that
in the creds? Or you'd have to manually grab that information when
/dev/binder is opened.

I'm storing the information in the task security blob because that's
the appropriate scope. Today the LSM hook is given both task_struct's.

Which is wrong, because you have no idea who the semantic "recipient
task" is - any task that has a mapping of the binder fd can
effectively receive transactions from it.

(And the current "sender task" is also wrong, because binder looks at
the task that opened the binder device, not the task currently
performing the action.)

I'm confused. Are you saying that the existing binder code is
completely broken? Are you saying that neither "task" is correct?

Yeah, basically

Well, hot biscuits and gravy!

 - but luckily the actual impact this has is limited by
the transitions that SELinux permits. If domain1 has no way to
transition to domain2, then it can't abuse this bug to pretend to be
domain2. I do have a reproducer that lets Android's "shell" domain
send a binder transaction that appears to come from "runas", but
luckily "runas" has no interesting privileges with regards to binder,
so that's not exploitable.

You're counting on the peculiarities of the SELinux policy you're
assuming is used to mask the fact that the hook isn't really doing
what it is supposed to?  Ouch.

quoted

How does passing the creds from the wrong tasks "fix" the problem?

This patch is not passing the creds from the "wrong" tasks at all. It
relies on the basic idea that when a security context opens a
resource, and then hands that resource to another context for
read/write operations, then you can effectively treat this as a
delegation of privileges from the original opener, and perform access
checks against the credentials using which the resource was opened.

OK. I can understand that without endorsing it.

In particular, we already have those semantics in the core kernel for
->read() and ->write() VFS operations - they are *not allowed* to look
at the credentials of the caller, and if they want to make security
checks, they have to instead check against file->f_cred, which are the
credentials using which the file was originally opened. (Yes, some
places still get that wrong.) Passing a file descriptor to another
task is a delegation of access, and the other task can then call
syscalls like read() / write() / mmap() on the file descriptor without
needing to have any access to the underlying file.

A mechanism sufficiently entrenched.

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. And about where you are getting the cred from
if not a task.

quoted

quoted

quoted

It's easy to compare to make sure the tasks are compatible.

It would be, if you actually had a pair of tasks that accurately
represent the sender and the recipient.

quoted

Adding the
information to the cred would be yet another case where the scope of
security information is wrong.

Can you elaborate on why you think that?

The information identifies how the task is going to display
the security "context". It isn't used in access checks.

But it is data that AFAICS needs to be preserved in the same places
where the creds need to be preserved, and it is also related to
security labels, so isn't "struct cred" a logical place to stuff it
anyway?

I am probably the only person on the planet who dislikes shared creds.
One of the things that made me happiest when I switched from UNIX
development to Linux was that it didn't have shared creds and all the
associated management. Oh well. Yes, it could go in the cred.

But that raises another question. Where are the creds coming from?
Is it even rational to make access decisions based on them? You've
explained how SELinux ends up with an Uncle Bob, but that's doesn't
leave me confident that another security module would be able to
come up with something sensible. 

At this point I'm really looking for something that I can put in
the change log explaining why creds work and task_structs don't.

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

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

quoted

On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 3:58 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 12:50 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn [off-list ref] wrote:

quoted

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

Why not? Why can't you put the security identity of the task into the creds?

Ah, I get it now, you're concerned about different processes wanting
to see security contexts formatted differently (e.g. printing the
SELinux label vs printing the AppArmor label), right?

That is correct.

quoted

But still, I don't think you can pull that information from the
receiving task. Maybe the easiest solution would be to also store that
in the creds? Or you'd have to manually grab that information when
/dev/binder is opened.

I'm storing the information in the task security blob because that's
the appropriate scope. Today the LSM hook is given both task_struct's.

Which is wrong, because you have no idea who the semantic "recipient
task" is - any task that has a mapping of the binder fd can
effectively receive transactions from it.

(And the current "sender task" is also wrong, because binder looks at
the task that opened the binder device, not the task currently
performing the action.)

I'm confused. Are you saying that the existing binder code is
completely broken? Are you saying that neither "task" is correct?

Yeah, basically

Well, hot biscuits and gravy!

quoted

 - but luckily the actual impact this has is limited by
the transitions that SELinux permits. If domain1 has no way to
transition to domain2, then it can't abuse this bug to pretend to be
domain2. I do have a reproducer that lets Android's "shell" domain
send a binder transaction that appears to come from "runas", but
luckily "runas" has no interesting privileges with regards to binder,
so that's not exploitable.

You're counting on the peculiarities of the SELinux policy you're
assuming is used to mask the fact that the hook isn't really doing
what it is supposed to?  Ouch.

I'm not saying I like the current situation - I do think that this
needs to change. I'm just saying it probably isn't *exploitable*, and
exploitability often hinges on these little circumstantial details.

quoted

quoted

How does passing the creds from the wrong tasks "fix" the problem?

This patch is not passing the creds from the "wrong" tasks at all. It
relies on the basic idea that when a security context opens a
resource, and then hands that resource to another context for
read/write operations, then you can effectively treat this as a
delegation of privileges from the original opener, and perform access
checks against the credentials using which the resource was opened.

OK. I can understand that without endorsing it.

quoted

In particular, we already have those semantics in the core kernel for
->read() and ->write() VFS operations - they are *not allowed* to look
at the credentials of the caller, and if they want to make security
checks, they have to instead check against file->f_cred, which are the
credentials using which the file was originally opened. (Yes, some
places still get that wrong.) Passing a file descriptor to another
task is a delegation of access, and the other task can then call
syscalls like read() / write() / mmap() on the file descriptor without
needing to have any access to the underlying file.

A mechanism sufficiently entrenched.

It's not just "entrenched", it is a fundamental requirement for being
able to use file descriptor passing with syscalls like write(). If
task A gives a file descriptor to task B, then task B must be able to
write() to that FD without having to worry that the FD actually refers
to some sort of special file that interprets the written data as some
type of command, or something like that, and that this leads to task B
unknowingly passing through access checks.

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".

And about where you are getting the cred from
if not a task.

This patch still ultimately gets the creds from a task. But it's not
looking at the *current* credentials of any task, but instead looks at
the credentials that the task that opened /dev/binder had at that
point.

quoted

quoted

quoted

quoted

It's easy to compare to make sure the tasks are compatible.

It would be, if you actually had a pair of tasks that accurately
represent the sender and the recipient.

quoted

Adding the
information to the cred would be yet another case where the scope of
security information is wrong.

Can you elaborate on why you think that?

The information identifies how the task is going to display
the security "context". It isn't used in access checks.

But it is data that AFAICS needs to be preserved in the same places
where the creds need to be preserved, and it is also related to
security labels, so isn't "struct cred" a logical place to stuff it
anyway?

I am probably the only person on the planet who dislikes shared creds.
One of the things that made me happiest when I switched from UNIX
development to Linux was that it didn't have shared creds and all the
associated management. Oh well. Yes, it could go in the cred.

But that raises another question. Where are the creds coming from?

They are the creds that the task that opened /dev/binder had.

Is it even rational to make access decisions based on them?

Yes, if you assume that handing file descriptors or memory mappings to
other security contexts constitutes delegation of privilege.

You've
explained how SELinux ends up with an Uncle Bob, but that's doesn't
leave me confident that another security module would be able to
come up with something sensible.

Just like SELinux, they can stuff information about a task's identity
into the cred security blob. See e.g. AppArmor, which AFAICS also does
that.

At this point I'm really looking for something that I can put in
the change log explaining why creds work and task_structs don't.

creds work because they can snapshot the privileges a task had at some
point in time for future security checks. task_structs don't work
because you can't use them to figure out which privileges a task had
in the past (or will have in the future).

On Fri, Oct 01, 2021 at 10:55:21AM -0700, Todd Kjos wrote:

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

Fix by saving the 'struct cred' during binder_open and pass
it to the selinux subsystem.

Fixes: 79af73079d75 ("Add security hooks to binder and implement the
hooks for SELinux.")
Signed-off-by: Todd Kjos <redacted>
Cc: stable@vger.kernel.org # 5.14+ (need backport for earlier stables)
---
v2: updated comments as suggested by Paul Moore

 drivers/android/binder.c          | 14 +++++----
 drivers/android/binder_internal.h |  4 +++
 include/linux/lsm_hook_defs.h     | 14 ++++-----
 include/linux/lsm_hooks.h         | 14 ++++-----
 include/linux/security.h          | 28 +++++++++---------
 security/security.c               | 14 ++++-----
 security/selinux/hooks.c          | 48 +++++++++----------------------
 7 files changed, 60 insertions(+), 76 deletions(-)

Ideally I could get an ack from the security developers before taking
this in my tree...

thanks,

greg k-h

On Tue, Oct 5, 2021 at 9:31 AM Greg KH [off-list ref] wrote:

On Fri, Oct 01, 2021 at 10:55:21AM -0700, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

Fix by saving the 'struct cred' during binder_open and pass
it to the selinux subsystem.

Fixes: 79af73079d75 ("Add security hooks to binder and implement the
hooks for SELinux.")
Signed-off-by: Todd Kjos <redacted>
Cc: stable@vger.kernel.org # 5.14+ (need backport for earlier stables)
---
v2: updated comments as suggested by Paul Moore

 drivers/android/binder.c          | 14 +++++----
 drivers/android/binder_internal.h |  4 +++
 include/linux/lsm_hook_defs.h     | 14 ++++-----
 include/linux/lsm_hooks.h         | 14 ++++-----
 include/linux/security.h          | 28 +++++++++---------
 security/security.c               | 14 ++++-----
 security/selinux/hooks.c          | 48 +++++++++----------------------
 7 files changed, 60 insertions(+), 76 deletions(-)

Ideally I could get an ack from the security developers before taking
this in my tree...

This should probably go in via one of the security trees, e.g. SELinux
or LSM, rather than the binder/driver tree.

-- 
paul moore
www.paul-moore.com

On Tue, Oct 05, 2021 at 09:53:31AM -0400, Paul Moore wrote:

On Tue, Oct 5, 2021 at 9:31 AM Greg KH [off-list ref] wrote:

quoted

On Fri, Oct 01, 2021 at 10:55:21AM -0700, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

Fix by saving the 'struct cred' during binder_open and pass
it to the selinux subsystem.

Fixes: 79af73079d75 ("Add security hooks to binder and implement the
hooks for SELinux.")
Signed-off-by: Todd Kjos <redacted>
Cc: stable@vger.kernel.org # 5.14+ (need backport for earlier stables)
---
v2: updated comments as suggested by Paul Moore

 drivers/android/binder.c          | 14 +++++----
 drivers/android/binder_internal.h |  4 +++
 include/linux/lsm_hook_defs.h     | 14 ++++-----
 include/linux/lsm_hooks.h         | 14 ++++-----
 include/linux/security.h          | 28 +++++++++---------
 security/security.c               | 14 ++++-----
 security/selinux/hooks.c          | 48 +++++++++----------------------
 7 files changed, 60 insertions(+), 76 deletions(-)

Ideally I could get an ack from the security developers before taking
this in my tree...

This should probably go in via one of the security trees, e.g. SELinux
or LSM, rather than the binder/driver tree.

Fine with me, take it away!

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

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

On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 3:58 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 12:50 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn [off-list ref] wrote:

quoted

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

Why not? Why can't you put the security identity of the task into the creds?

Ah, I get it now, you're concerned about different processes wanting
to see security contexts formatted differently (e.g. printing the
SELinux label vs printing the AppArmor label), right?

That is correct.

quoted

But still, I don't think you can pull that information from the
receiving task. Maybe the easiest solution would be to also store that
in the creds? Or you'd have to manually grab that information when
/dev/binder is opened.

I'm storing the information in the task security blob because that's
the appropriate scope. Today the LSM hook is given both task_struct's.

Which is wrong, because you have no idea who the semantic "recipient
task" is - any task that has a mapping of the binder fd can
effectively receive transactions from it.

(And the current "sender task" is also wrong, because binder looks at
the task that opened the binder device, not the task currently
performing the action.)

I'm confused. Are you saying that the existing binder code is
completely broken? Are you saying that neither "task" is correct?

Yeah, basically

Well, hot biscuits and gravy!

quoted

 - but luckily the actual impact this has is limited by
the transitions that SELinux permits. If domain1 has no way to
transition to domain2, then it can't abuse this bug to pretend to be
domain2. I do have a reproducer that lets Android's "shell" domain
send a binder transaction that appears to come from "runas", but
luckily "runas" has no interesting privileges with regards to binder,
so that's not exploitable.

You're counting on the peculiarities of the SELinux policy you're
assuming is used to mask the fact that the hook isn't really doing
what it is supposed to?  Ouch.

I'm not saying I like the current situation - I do think that this
needs to change. I'm just saying it probably isn't *exploitable*, and
exploitability often hinges on these little circumstantial details.

quoted

quoted

quoted

How does passing the creds from the wrong tasks "fix" the problem?

This patch is not passing the creds from the "wrong" tasks at all. It
relies on the basic idea that when a security context opens a
resource, and then hands that resource to another context for
read/write operations, then you can effectively treat this as a
delegation of privileges from the original opener, and perform access
checks against the credentials using which the resource was opened.

OK. I can understand that without endorsing it.

quoted

In particular, we already have those semantics in the core kernel for
->read() and ->write() VFS operations - they are *not allowed* to look
at the credentials of the caller, and if they want to make security
checks, they have to instead check against file->f_cred, which are the
credentials using which the file was originally opened. (Yes, some
places still get that wrong.) Passing a file descriptor to another
task is a delegation of access, and the other task can then call
syscalls like read() / write() / mmap() on the file descriptor without
needing to have any access to the underlying file.

A mechanism sufficiently entrenched.

It's not just "entrenched", it is a fundamental requirement for being
able to use file descriptor passing with syscalls like write(). If
task A gives a file descriptor to task B, then task B must be able to
write() to that FD without having to worry that the FD actually refers
to some sort of special file that interprets the written data as some
type of command, or something like that, and that this leads to task B
unknowingly passing through access checks.

quoted

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.

On Tue, Oct 5, 2021 at 8:21 AM Stephen Smalley
[off-list ref] wrote:

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

On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 3:58 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 12:50 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn [off-list ref] wrote:

quoted

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

Why not? Why can't you put the security identity of the task into the creds?

Ah, I get it now, you're concerned about different processes wanting
to see security contexts formatted differently (e.g. printing the
SELinux label vs printing the AppArmor label), right?

That is correct.

quoted

But still, I don't think you can pull that information from the
receiving task. Maybe the easiest solution would be to also store that
in the creds? Or you'd have to manually grab that information when
/dev/binder is opened.

I'm storing the information in the task security blob because that's
the appropriate scope. Today the LSM hook is given both task_struct's.

Which is wrong, because you have no idea who the semantic "recipient
task" is - any task that has a mapping of the binder fd can
effectively receive transactions from it.

(And the current "sender task" is also wrong, because binder looks at
the task that opened the binder device, not the task currently
performing the action.)

I'm confused. Are you saying that the existing binder code is
completely broken? Are you saying that neither "task" is correct?

Yeah, basically

Well, hot biscuits and gravy!

quoted

 - but luckily the actual impact this has is limited by
the transitions that SELinux permits. If domain1 has no way to
transition to domain2, then it can't abuse this bug to pretend to be
domain2. I do have a reproducer that lets Android's "shell" domain
send a binder transaction that appears to come from "runas", but
luckily "runas" has no interesting privileges with regards to binder,
so that's not exploitable.

You're counting on the peculiarities of the SELinux policy you're
assuming is used to mask the fact that the hook isn't really doing
what it is supposed to?  Ouch.

I'm not saying I like the current situation - I do think that this
needs to change. I'm just saying it probably isn't *exploitable*, and
exploitability often hinges on these little circumstantial details.

quoted

quoted

quoted

How does passing the creds from the wrong tasks "fix" the problem?

This patch is not passing the creds from the "wrong" tasks at all. It
relies on the basic idea that when a security context opens a
resource, and then hands that resource to another context for
read/write operations, then you can effectively treat this as a
delegation of privileges from the original opener, and perform access
checks against the credentials using which the resource was opened.

OK. I can understand that without endorsing it.

quoted

In particular, we already have those semantics in the core kernel for
->read() and ->write() VFS operations - they are *not allowed* to look
at the credentials of the caller, and if they want to make security
checks, they have to instead check against file->f_cred, which are the
credentials using which the file was originally opened. (Yes, some
places still get that wrong.) Passing a file descriptor to another
task is a delegation of access, and the other task can then call
syscalls like read() / write() / mmap() on the file descriptor without
needing to have any access to the underlying file.

A mechanism sufficiently entrenched.

It's not just "entrenched", it is a fundamental requirement for being
able to use file descriptor passing with syscalls like write(). If
task A gives a file descriptor to task B, then task B must be able to
write() to that FD without having to worry that the FD actually refers
to some sort of special file that interprets the written data as some
type of command, or something like that, and that this leads to task B
unknowingly passing through access checks.

quoted

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()?

Stephan, do you want that to be included in this patch? Or should I
follow this up with another patch for the sender_euid case?

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.

--
To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.

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

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

On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 3:58 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 12:50 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn [off-list ref] wrote:

quoted

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

Why not? Why can't you put the security identity of the task into the creds?

Ah, I get it now, you're concerned about different processes wanting
to see security contexts formatted differently (e.g. printing the
SELinux label vs printing the AppArmor label), right?

That is correct.

quoted

But still, I don't think you can pull that information from the
receiving task. Maybe the easiest solution would be to also store that
in the creds? Or you'd have to manually grab that information when
/dev/binder is opened.

I'm storing the information in the task security blob because that's
the appropriate scope. Today the LSM hook is given both task_struct's.

Which is wrong, because you have no idea who the semantic "recipient
task" is - any task that has a mapping of the binder fd can
effectively receive transactions from it.

(And the current "sender task" is also wrong, because binder looks at
the task that opened the binder device, not the task currently
performing the action.)

I'm confused. Are you saying that the existing binder code is
completely broken? Are you saying that neither "task" is correct?

Yeah, basically

Well, hot biscuits and gravy!

quoted

 - but luckily the actual impact this has is limited by
the transitions that SELinux permits. If domain1 has no way to
transition to domain2, then it can't abuse this bug to pretend to be
domain2. I do have a reproducer that lets Android's "shell" domain
send a binder transaction that appears to come from "runas", but
luckily "runas" has no interesting privileges with regards to binder,
so that's not exploitable.

You're counting on the peculiarities of the SELinux policy you're
assuming is used to mask the fact that the hook isn't really doing
what it is supposed to?  Ouch.

I'm not saying I like the current situation - I do think that this
needs to change. I'm just saying it probably isn't *exploitable*, and
exploitability often hinges on these little circumstantial details.

quoted

quoted

quoted

How does passing the creds from the wrong tasks "fix" the problem?

This patch is not passing the creds from the "wrong" tasks at all. It
relies on the basic idea that when a security context opens a
resource, and then hands that resource to another context for
read/write operations, then you can effectively treat this as a
delegation of privileges from the original opener, and perform access
checks against the credentials using which the resource was opened.

OK. I can understand that without endorsing it.

quoted

In particular, we already have those semantics in the core kernel for
->read() and ->write() VFS operations - they are *not allowed* to look
at the credentials of the caller, and if they want to make security
checks, they have to instead check against file->f_cred, which are the
credentials using which the file was originally opened. (Yes, some
places still get that wrong.) Passing a file descriptor to another
task is a delegation of access, and the other task can then call
syscalls like read() / write() / mmap() on the file descriptor without
needing to have any access to the underlying file.

A mechanism sufficiently entrenched.

It's not just "entrenched", it is a fundamental requirement for being
able to use file descriptor passing with syscalls like write(). If
task A gives a file descriptor to task B, then task B must be able to
write() to that FD without having to worry that the FD actually refers
to some sort of special file that interprets the written data as some
type of command, or something like that, and that this leads to task B
unknowingly passing through access checks.

quoted

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. I, for one,
had assumed that the cred being passed was the cred from the task. There
is certainly nothing in the description to make me think otherwise.

On Tue, Oct 5, 2021 at 12:49 PM Todd Kjos [off-list ref] wrote:

On Tue, Oct 5, 2021 at 8:21 AM Stephen Smalley
[off-list ref] 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

On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 3:58 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 12:50 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn [off-list ref] wrote:

quoted

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

Save the struct cred associated with a binder process
at initial open to avoid potential race conditions
when converting to a security ID.

Since binder was integrated with selinux, it has passed
'struct task_struct' associated with the binder_proc
to represent the source and target of transactions.
The conversion of task to SID was then done in the hook
implementations. It turns out that there are race conditions
which can result in an incorrect security context being used.

In the LSM stacking patch set I've been posting for a while
(on version 29 now) I use information from the task structure
to ensure that the security information passed via the binder
interface is agreeable to both sides. Passing the cred will
make it impossible to do this check. The task information
required is not appropriate to have in the cred.

Why not? Why can't you put the security identity of the task into the creds?

Ah, I get it now, you're concerned about different processes wanting
to see security contexts formatted differently (e.g. printing the
SELinux label vs printing the AppArmor label), right?

That is correct.

quoted

But still, I don't think you can pull that information from the
receiving task. Maybe the easiest solution would be to also store that
in the creds? Or you'd have to manually grab that information when
/dev/binder is opened.

I'm storing the information in the task security blob because that's
the appropriate scope. Today the LSM hook is given both task_struct's.

Which is wrong, because you have no idea who the semantic "recipient
task" is - any task that has a mapping of the binder fd can
effectively receive transactions from it.

(And the current "sender task" is also wrong, because binder looks at
the task that opened the binder device, not the task currently
performing the action.)

I'm confused. Are you saying that the existing binder code is
completely broken? Are you saying that neither "task" is correct?

Yeah, basically

Well, hot biscuits and gravy!

quoted

 - but luckily the actual impact this has is limited by
the transitions that SELinux permits. If domain1 has no way to
transition to domain2, then it can't abuse this bug to pretend to be
domain2. I do have a reproducer that lets Android's "shell" domain
send a binder transaction that appears to come from "runas", but
luckily "runas" has no interesting privileges with regards to binder,
so that's not exploitable.

You're counting on the peculiarities of the SELinux policy you're
assuming is used to mask the fact that the hook isn't really doing
what it is supposed to?  Ouch.

I'm not saying I like the current situation - I do think that this
needs to change. I'm just saying it probably isn't *exploitable*, and
exploitability often hinges on these little circumstantial details.

quoted

quoted

quoted

How does passing the creds from the wrong tasks "fix" the problem?

This patch is not passing the creds from the "wrong" tasks at all. It
relies on the basic idea that when a security context opens a
resource, and then hands that resource to another context for
read/write operations, then you can effectively treat this as a
delegation of privileges from the original opener, and perform access
checks against the credentials using which the resource was opened.

OK. I can understand that without endorsing it.

quoted

In particular, we already have those semantics in the core kernel for
->read() and ->write() VFS operations - they are *not allowed* to look
at the credentials of the caller, and if they want to make security
checks, they have to instead check against file->f_cred, which are the
credentials using which the file was originally opened. (Yes, some
places still get that wrong.) Passing a file descriptor to another
task is a delegation of access, and the other task can then call
syscalls like read() / write() / mmap() on the file descriptor without
needing to have any access to the underlying file.

A mechanism sufficiently entrenched.

It's not just "entrenched", it is a fundamental requirement for being
able to use file descriptor passing with syscalls like write(). If
task A gives a file descriptor to task B, then task B must be able to
write() to that FD without having to worry that the FD actually refers
to some sort of special file that interprets the written data as some
type of command, or something like that, and that this leads to task B
unknowingly passing through access checks.

quoted

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()?

Stephan, do you want that to be included in this patch? Or should I
follow this up with another patch for the sender_euid case?