| From foo@baz Sat Nov 13 12:18:28 PM CET 2021 |
| From: Todd Kjos <tkjos@google.com> |
| Date: Wed, 10 Nov 2021 15:00:24 -0800 |
| Subject: binder: use cred instead of task for selinux checks |
| To: stable@vger.kernel.org, gregkh@linuxfoundation.org, arve@android.com, tkjos@android.com, maco@android.com, christian@brauner.io, jmorris@namei.org, serge@hallyn.com, paul@paul-moore.com, stephen.smalley.work@gmail.com, eparis@parisplace.org, keescook@chromium.org, jannh@google.com, jeffv@google.com, zohar@linux.ibm.com, linux-security-module@vger.kernel.org, selinux@vger.kernel.org, devel@driverdev.osuosl.org |
| Cc: joel@joelfernandes.org, kernel-team@android.com, Todd Kjos <tkjos@google.com>, Casey Schaufler <casey@schaufler-ca.com> |
| Message-ID: <20211110230025.3272776-2-tkjos@google.com> |
| |
| From: Todd Kjos <tkjos@google.com> |
| |
| commit 52f88693378a58094c538662ba652aff0253c4fe upstream. |
| |
| 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 using the 'struct cred' saved during binder_open and pass |
| it to the selinux subsystem. |
| |
| Cc: stable@vger.kernel.org # 5.14 (need backport for earlier stables) |
| Fixes: 79af73079d75 ("Add security hooks to binder and implement the hooks for SELinux.") |
| Suggested-by: Jann Horn <jannh@google.com> |
| Signed-off-by: Todd Kjos <tkjos@google.com> |
| Acked-by: Casey Schaufler <casey@schaufler-ca.com> |
| Signed-off-by: Paul Moore <paul@paul-moore.com> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| drivers/android/binder.c | 12 ++++++------ |
| include/linux/lsm_hooks.h | 28 ++++++++++++++-------------- |
| include/linux/security.h | 28 ++++++++++++++-------------- |
| security/security.c | 14 +++++++------- |
| security/selinux/hooks.c | 36 +++++++++++++++--------------------- |
| 5 files changed, 56 insertions(+), 62 deletions(-) |
| |
| --- a/drivers/android/binder.c |
| +++ b/drivers/android/binder.c |
| @@ -2446,7 +2446,7 @@ static int binder_translate_binder(struc |
| 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; |
| } |
| @@ -2492,7 +2492,7 @@ static int binder_translate_handle(struc |
| 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; |
| } |
| @@ -2580,7 +2580,7 @@ static int binder_translate_fd(u32 fd, b |
| 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; |
| @@ -2979,8 +2979,8 @@ static void binder_transaction(struct bi |
| 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__; |
| @@ -4922,7 +4922,7 @@ static int binder_ioctl_set_ctx_mgr(stru |
| 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)) { |
| --- a/include/linux/lsm_hooks.h |
| +++ b/include/linux/lsm_hooks.h |
| @@ -1241,22 +1241,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 |
| @@ -1456,13 +1456,13 @@ |
| * @what: kernel feature being accessed |
| */ |
| union security_list_options { |
| - int (*binder_set_context_mgr)(struct task_struct *mgr); |
| - int (*binder_transaction)(struct task_struct *from, |
| - struct task_struct *to); |
| - int (*binder_transfer_binder)(struct task_struct *from, |
| - struct task_struct *to); |
| - int (*binder_transfer_file)(struct task_struct *from, |
| - struct task_struct *to, |
| + int (*binder_set_context_mgr)(const struct cred *mgr); |
| + int (*binder_transaction)(const struct cred *from, |
| + const struct cred *to); |
| + int (*binder_transfer_binder)(const struct cred *from, |
| + const struct cred *to); |
| + int (*binder_transfer_file)(const struct cred *from, |
| + const struct cred *to, |
| struct file *file); |
| |
| int (*ptrace_access_check)(struct task_struct *child, |
| --- a/include/linux/security.h |
| +++ b/include/linux/security.h |
| @@ -249,13 +249,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, |
| @@ -481,25 +481,25 @@ static inline int early_security_init(vo |
| 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; |
| --- a/security/security.c |
| +++ b/security/security.c |
| @@ -670,25 +670,25 @@ static void __init lsm_early_task(struct |
| |
| /* 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); |
| } |
| --- a/security/selinux/hooks.c |
| +++ b/security/selinux/hooks.c |
| @@ -2050,22 +2050,19 @@ static inline u32 open_file_to_av(struct |
| |
| /* 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) |
| { |
| - u32 mysid = current_sid(); |
| - u32 mgrsid = task_sid(mgr); |
| - |
| return avc_has_perm(&selinux_state, |
| - mysid, mgrsid, 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(from); |
| - u32 tosid = task_sid(to); |
| + u32 fromsid = cred_sid(from); |
| + u32 tosid = cred_sid(to); |
| int rc; |
| |
| if (mysid != fromsid) { |
| @@ -2076,27 +2073,24 @@ static int selinux_binder_transaction(st |
| return rc; |
| } |
| |
| - return avc_has_perm(&selinux_state, |
| - fromsid, tosid, SECCLASS_BINDER, BINDER__CALL, |
| - NULL); |
| + 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) |
| { |
| - u32 fromsid = task_sid(from); |
| - u32 tosid = task_sid(to); |
| - |
| return avc_has_perm(&selinux_state, |
| - fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER, |
| + 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(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; |