On Fri, Oct 1, 2021 at 7:38 AM Paul Moore paul@paul-moore.com wrote:
On Thu, Sep 30, 2021 at 10:45 PM Todd Kjos tkjos@google.com 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)
drivers/android/binder.c | 14 +++++---- drivers/android/binder_internal.h | 3 ++ include/linux/lsm_hook_defs.h | 14 ++++----- include/linux/security.h | 28 +++++++++--------- security/security.c | 14 ++++----- security/selinux/hooks.c | 48 +++++++++---------------------- 6 files changed, 52 insertions(+), 69 deletions(-)
Thanks Todd, I'm happy to see someone with a better understanding of binder than me pitch in to clean this up :) A couple of quick comments/questions below ...
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 @@ -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);Is it *always* true that filp->f_cred is going to be the same as current->group_leader->cred?
Not necessarily -- it is current->cred of the task in binder_open() (not group_leader). This is fine. We used to set proc->tsk to current, but switched to group_leader a few years ago to make it easier to detect the same process with multiple opens during mmap (to solve some unrelated issues). We still use group_leader for that purpose, but for the cred, the current cred in binder_open() is sufficient.
Or rather does this help resolve the issue of wanting the subjective creds but not being able to access them mentioned in the task_sid_binder() comment? If the latter, it might be nice to add something to the related comment in struct binder_ref (below).
Yes, we want the subjective cred so that is part of the point. I started with "proc->cred = get_task_cred(current->group_leader)" and got feedback that the "subjective" cred is preferred to avoid some subtle races that could be introduced, for example, if /dev/binder is opened through io_uring.
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..886fc327a534 100644 --- a/drivers/android/binder_internal.h +++ b/drivers/android/binder_internal.h @@ -364,6 +364,8 @@ struct binder_ref {
(invariant after initialized)- @tsk task_struct for group_leader of process
(invariant after initialized)
- @cred struct cred for group_leader of process
(invariant after initialized)Related to the question above. At the very least the comment should probably be changed to indicate to make it clear the creds are coming directly from the binder file/device and not always the group_leader.
Good catch. Will update the comment (it's actually struct binder_proc).
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);-}
-- paul moore www.paul-moore.com
and from your next response:
Ooops, I was a little over zealous when trimming my response and I accidentally cut off my comment that the associated comment blocks in include/linux/lsm_hooks.h should also be updated to reflect the binder LSM hook changes.
Thanks for pointing this out! I didn't notice these comment blocks.
-Todd