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

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

[Todd Kjos]

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 <tkjos@google.com>
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

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

[Casey Schaufler]

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.

>
> 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 <tkjos@google.com>
> 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;

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

[Jann Horn]

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> 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.

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

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

[Jann Horn]

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > 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.
>
> 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.

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

[Casey Schaufler]

On 10/1/2021 12:50 PM, Jann Horn wrote:
> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> 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.
>> 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. 

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

[Jann Horn]

On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/1/2021 12:50 PM, Jann Horn wrote:
> > On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
> >> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>> 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.
> >> 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.

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?

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

[Casey Schaufler]

On 10/1/2021 3:58 PM, Jann Horn wrote:
> On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 10/1/2021 12:50 PM, Jann Horn wrote:
>>> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
>>>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> 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.
>>>> 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.
> 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?

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

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

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

[Jann Horn]

On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/1/2021 3:58 PM, Jann Horn wrote:
> > On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 10/1/2021 12:50 PM, Jann Horn wrote:
> >>> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
> >>>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>>> 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.
> >>>> 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.
> > 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.

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

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

[Casey Schaufler]

On 10/4/2021 3:28 PM, Jann Horn wrote:
> On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 10/1/2021 3:58 PM, Jann Horn wrote:
>>> On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 10/1/2021 12:50 PM, Jann Horn wrote:
>>>>> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
>>>>>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>> 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.
>>>>>> 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.
>>> 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.

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

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

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

[Jann Horn]

On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/4/2021 3:28 PM, Jann Horn wrote:
> > On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 10/1/2021 3:58 PM, Jann Horn wrote:
> >>> On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>> On 10/1/2021 12:50 PM, Jann Horn wrote:
> >>>>> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
> >>>>>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>>>>> 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.
> >>>>>> 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.
> >>> 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.

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.

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

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.

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

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

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

[Greg KH]

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 <tkjos@google.com>
> 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

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

[Paul Moore]

On Tue, Oct 5, 2021 at 9:31 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> 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 <tkjos@google.com>
> > 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

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

[Greg KH]

On Tue, Oct 05, 2021 at 09:53:31AM -0400, Paul Moore wrote:
> On Tue, Oct 5, 2021 at 9:31 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > 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 <tkjos@google.com>
> > > 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>

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

[Stephen Smalley]

On Mon, Oct 4, 2021 at 8:27 PM Jann Horn <jannh@google.com> wrote:
>
> On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 10/4/2021 3:28 PM, Jann Horn wrote:
> > > On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >> On 10/1/2021 3:58 PM, Jann Horn wrote:
> > >>> On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >>>> On 10/1/2021 12:50 PM, Jann Horn wrote:
> > >>>>> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
> > >>>>>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >>>>>>> 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.
> > >>>>>> 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.
> > >>> 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.
>
> 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.
>
> > >> 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.
>
> 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.
>
> > > 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.

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

[Todd Kjos]

On Tue, Oct 5, 2021 at 8:21 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Mon, Oct 4, 2021 at 8:27 PM Jann Horn <jannh@google.com> wrote:
> >
> > On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > On 10/4/2021 3:28 PM, Jann Horn wrote:
> > > > On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >> On 10/1/2021 3:58 PM, Jann Horn wrote:
> > > >>> On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >>>> On 10/1/2021 12:50 PM, Jann Horn wrote:
> > > >>>>> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
> > > >>>>>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >>>>>>> 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.
> > > >>>>>> 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.
> > > >>> 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.
> >
> > 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.
> >
> > > >> 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.
> >
> > 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.
> >
> > > > 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.
>

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

[Casey Schaufler]

On 10/5/2021 8:21 AM, Stephen Smalley wrote:
> On Mon, Oct 4, 2021 at 8:27 PM Jann Horn <jannh@google.com> wrote:
>> On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 10/4/2021 3:28 PM, Jann Horn wrote:
>>>> On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> On 10/1/2021 3:58 PM, Jann Horn wrote:
>>>>>> On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>> On 10/1/2021 12:50 PM, Jann Horn wrote:
>>>>>>>> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
>>>>>>>>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>>>> 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.
>>>>>>>>> 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.
>>>>>> 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.
>> 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.
>>
>>>>> 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.
>> 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.
>>
>>>> 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.

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

[Stephen Smalley]

On Tue, Oct 5, 2021 at 12:49 PM Todd Kjos <tkjos@google.com> wrote:
>
> On Tue, Oct 5, 2021 at 8:21 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Mon, Oct 4, 2021 at 8:27 PM Jann Horn <jannh@google.com> wrote:
> > >
> > > On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > On 10/4/2021 3:28 PM, Jann Horn wrote:
> > > > > On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > >> On 10/1/2021 3:58 PM, Jann Horn wrote:
> > > > >>> On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > >>>> On 10/1/2021 12:50 PM, Jann Horn wrote:
> > > > >>>>> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
> > > > >>>>>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > >>>>>>> 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.
> > > > >>>>>> 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.
> > > > >>> 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.
> > >
> > > 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.
> > >
> > > > >> 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.
> > >
> > > 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.
> > >
> > > > > 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?

Either way is fine with me. Fixing sender_euid arguably is a fix that
should go all the way back to the introduction of binder unless I
misunderstand; it is independent of SELinux. Fixing the
security_task_getsecid -> cred_secid only goes back to
ec74136ded792deed80780a2f8baf3521eeb72f9. So having it as 3 separate
patches may help with the different Fixes tags and back-porting
purposes.

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

[Paul Moore]

On Tue, Oct 5, 2021 at 1:12 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, Oct 5, 2021 at 12:49 PM Todd Kjos <tkjos@google.com> wrote:
> >
> > On Tue, Oct 5, 2021 at 8:21 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Mon, Oct 4, 2021 at 8:27 PM Jann Horn <jannh@google.com> wrote:
> > > >
> > > > On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > > On 10/4/2021 3:28 PM, Jann Horn wrote:
> > > > > > On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > > >> On 10/1/2021 3:58 PM, Jann Horn wrote:
> > > > > >>> On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > > >>>> On 10/1/2021 12:50 PM, Jann Horn wrote:
> > > > > >>>>> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
> > > > > >>>>>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > > >>>>>>> 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.
> > > > > >>>>>> 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.
> > > > > >>> 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.
> > > >
> > > > 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.
> > > >
> > > > > >> 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.
> > > >
> > > > 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.
> > > >
> > > > > > 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?
>
> Either way is fine with me. Fixing sender_euid arguably is a fix that
> should go all the way back to the introduction of binder unless I
> misunderstand; it is independent of SELinux. Fixing the
> security_task_getsecid -> cred_secid only goes back to
> ec74136ded792deed80780a2f8baf3521eeb72f9. So having it as 3 separate
> patches may help with the different Fixes tags and back-porting
> purposes.

Yes, as annoying as it may be, please do separate patches as the
-stable and distro folks will have an easier time that way.

-- 
paul moore
www.paul-moore.com

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

[Jann Horn]

On Tue, Oct 5, 2021 at 5:21 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Mon, Oct 4, 2021 at 8:27 PM Jann Horn <jannh@google.com> wrote:
> > On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > On 10/4/2021 3:28 PM, Jann Horn wrote:
> > > > On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >> On 10/1/2021 3:58 PM, Jann Horn wrote:
> > > >>> On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >>>> On 10/1/2021 12:50 PM, Jann Horn wrote:
> > > >>>>> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@google.com> wrote:
> > > >>>>>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >>>>>>> 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.
> > > >>>>>> 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.
> > > >>> 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.
> >
> > 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.
> >
> > > >> 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.
> >
> > 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.
> >
> > > > 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()?

Good point.

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

The SELinux hook for sending binder transactions does actually have a
delegation check on the sending side, checking against the current
task, although that would be unreliable if another task in a different
security context also has access to the binder mapping. But the
security context used for the SELinux access check and delegation
check is read separately from the security context transmitted to the
other side, and if you race an execve() in between the two (which is
possible because the creds are read from another thread), you can
effectively bypass the delegation check with regards to the
transmitted security context.

binder_transfer_binder_ and binder_transfer_file are also effectively
covered by that delegation check.

The hook for setting the binder context manager also has a delegation
check against the current task.

If you actually wanted to prevent uncontrolled delegation, I think
you'd also have to check all the VMAs for forbidden file mappings,
ensure that in particular binder/io_uring/... FDs or VMAs can not be
shared across any kind of privilege transition (even if both sides of
the transition would've been allowed to have such an FD/VMA), and
completely prevent pin_user_pages() on mappings that wouldn't be
allowed to be inherited across a privilege transition (because
otherwise someone could abuse a subsystem like io_uring to grab a
reference to a bunch of pages from such a mapping and then write into
them later)?

At the moment, SELinux scans through file descriptors on transition
but not through memory mappings of files, which is kinda weird...

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

[Jann Horn]

On Tue, Oct 5, 2021 at 6:59 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/5/2021 8:21 AM, Stephen Smalley wrote:
> > On Mon, Oct 4, 2021 at 8:27 PM Jann Horn <jannh@google.com> wrote:
> >> On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>> On 10/4/2021 3:28 PM, Jann Horn wrote:
> >>>> 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.)

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

[Casey Schaufler]

On 10/5/2021 7:27 PM, Jann Horn wrote:
> On Tue, Oct 5, 2021 at 6:59 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 10/5/2021 8:21 AM, Stephen Smalley wrote:
>>> On Mon, Oct 4, 2021 at 8:27 PM Jann Horn <jannh@google.com> wrote:
>>>> On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> On 10/4/2021 3:28 PM, Jann Horn wrote:
>>>>>> 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.)

It appears that I'll be better off using some other mechanism for
the recipient to identify the security module used by the sender
than the arguments to security_binder_transaction(). It's likely to
be more invasive to the binder driver, but that's the way things go.
At any rate, I'm no longer concerned about either the data type or
the source of what goes into the hook.