| Message ID | 20211001175521.3853257-1-tkjos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] binder: use cred instead of task for selinux checks | expand |
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;
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.)
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.
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.
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?
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.
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?
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.
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).
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
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.
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>
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.
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. >
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.
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.
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.
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...
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.)
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.
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;
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(-)