This series fixes the possible use of an incorrect security context
when checking selinux permissions, getting a security ID, or lookup
up the euid.
The previous behavior was to save the group_leader 'struct task_struct'
in binder_open() and using that to obtain security IDs or euids.
This has been shown to be unreliable, so this series instead saves the
'struct cred' of the task that called binder_open(). This cred is used
for these lookups instead of the task.
v1 and v2 of this series were a single patch "binder: use euid from"
cred instead of using task". During review, Stephen Smalley identified
two more related issues so the corresponding patches were added to
the series.
v3:
- add 2 patches to fix getsecid and euid
v4:
- fix minor checkpatch issues
- fix build-break for !CONFIG_SECURITY
v5:
- reorder/refactor patches as suggested by Stephen Smalley so eiud fix
is first and saves the cred during binder_open()
- set *secid=0 for !CONFIG_SECURITY version of secuirty_cred_getsecid()
Todd Kjos (3):
binder: use euid from cred instead of using task
binder: use cred instead of task for selinux checks
binder: use cred instead of task for getsecid
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(-)
Save the 'struct cred' associated with a binder process
at initial open to avoid potential race conditions
when converting to an euid.
Set a transaction's sender_euid from the 'struct cred'
saved at binder_open() instead of looking up the euid
from the binder proc's 'struct task'. This ensures
the euid is associated with the security context that
of the task that opened binder.
Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
Signed-off-by: Todd Kjos <[email protected]>
Suggested-by: Stephen Smalley <[email protected]>
Suggested-by: Jann Horn <[email protected]>
Cc: [email protected] # 4.4+
---
v3: added this patch to series (as 3/3)
v5:
- combined with saving of 'struct cred' during binder_open()
- reordered to 1/1 as suggested by Stephen Smalley
drivers/android/binder.c | 4 +++-
drivers/android/binder_internal.h | 4 ++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 9edacc8b9768..a396015e874a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2711,7 +2711,7 @@ static void binder_transaction(struct binder_proc *proc,
t->from = thread;
else
t->from = NULL;
- t->sender_euid = task_euid(proc->tsk);
+ t->sender_euid = proc->cred->euid;
t->to_proc = target_proc;
t->to_thread = target_thread;
t->code = tr->code;
@@ -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);
}
@@ -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;
--
2.33.0.882.g93a45727a2-goog
Use the 'struct cred' saved at binder_open() to lookup
the security ID via security_cred_getsecid(). This
ensures that the security context that opened binder
is the one used to generate the secctx.
Fixes: ec74136ded79 ("binder: create node flag to request sender's
security context")
Signed-off-by: Todd Kjos <[email protected]>
Suggested-by: Stephen Smalley <[email protected]>
Reported-by: kernel test robot <[email protected]>
Cc: [email protected] # 5.4+
---
v3: added this patch to series
v4: fix build-break for !CONFIG_SECURITY
v5: set *secid=0 for !CONFIG_SECURITY version of secuirty_cred_getsecid()
drivers/android/binder.c | 11 +----------
include/linux/security.h | 5 +++++
2 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index bc15325f0579..26382e982c5e 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2722,16 +2722,7 @@ static void binder_transaction(struct binder_proc *proc,
u32 secid;
size_t added_size;
- /*
- * Arguably this should be the task's subjective LSM secid but
- * we can't reliably access the subjective creds of a task
- * other than our own so we must use the objective creds, 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.
- */
- security_task_getsecid_obj(proc->tsk, &secid);
+ security_cred_getsecid(proc->cred, &secid);
ret = security_secid_to_secctx(secid, &secctx, &secctx_sz);
if (ret) {
return_error = BR_FAILED_REPLY;
diff --git a/include/linux/security.h b/include/linux/security.h
index 6344d3362df7..46a02ce34d00 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1041,6 +1041,11 @@ static inline void security_transfer_creds(struct cred *new,
{
}
+static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
+{
+ *secid = 0;
+}
+
static inline int security_kernel_act_as(struct cred *cred, u32 secid)
{
return 0;
--
2.33.0.882.g93a45727a2-goog
On 10/12/2021 9:56 AM, Todd Kjos wrote:
> This series fixes the possible use of an incorrect security context
> when checking selinux permissions, getting a security ID, or lookup
> up the euid.
>
> The previous behavior was to save the group_leader 'struct task_struct'
> in binder_open() and using that to obtain security IDs or euids.
>
> This has been shown to be unreliable, so this series instead saves the
> 'struct cred' of the task that called binder_open(). This cred is used
> for these lookups instead of the task.
>
> v1 and v2 of this series were a single patch "binder: use euid from"
> cred instead of using task". During review, Stephen Smalley identified
> two more related issues so the corresponding patches were added to
> the series.
>
> v3:
> - add 2 patches to fix getsecid and euid
>
> v4:
> - fix minor checkpatch issues
> - fix build-break for !CONFIG_SECURITY
>
> v5:
> - reorder/refactor patches as suggested by Stephen Smalley so eiud fix
> is first and saves the cred during binder_open()
> - set *secid=0 for !CONFIG_SECURITY version of secuirty_cred_getsecid()
>
> Todd Kjos (3):
> binder: use euid from cred instead of using task
> binder: use cred instead of task for selinux checks
> binder: use cred instead of task for getsecid
>
> 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(-)
For the series:
Acked-by: Casey Schaufler <[email protected]>
On Tue, Oct 12, 2021 at 12:56 PM Todd Kjos <[email protected]> wrote:
>
> This series fixes the possible use of an incorrect security context
> when checking selinux permissions, getting a security ID, or lookup
> up the euid.
>
> The previous behavior was to save the group_leader 'struct task_struct'
> in binder_open() and using that to obtain security IDs or euids.
>
> This has been shown to be unreliable, so this series instead saves the
> 'struct cred' of the task that called binder_open(). This cred is used
> for these lookups instead of the task.
Hi Todd,
I just merged all three patches into selinux/next, thanks for your
help patience on this patchset. Ultimately I merged these patches
into selinux/next as opposed to selinux/stable-5.15 because I felt
that a couple of weeks in -next before going to Linus would be a good
thing. I'm also not certain how widespread binder is outside of
Android so I figured the practical difference between next and
stable-5.15 is likely very small. Regardless, all of your Fixes and
stable tags remain in the patches so as soon as they go up to Linus
during the next merge window the stable folks will be notified.
--
paul moore
http://www.paul-moore.com
On Thu, Oct 14, 2021 at 2:34 PM Paul Moore <[email protected]> wrote:
>
> On Tue, Oct 12, 2021 at 12:56 PM Todd Kjos <[email protected]> wrote:
> >
> > This series fixes the possible use of an incorrect security context
> > when checking selinux permissions, getting a security ID, or lookup
> > up the euid.
> >
> > The previous behavior was to save the group_leader 'struct task_struct'
> > in binder_open() and using that to obtain security IDs or euids.
> >
> > This has been shown to be unreliable, so this series instead saves the
> > 'struct cred' of the task that called binder_open(). This cred is used
> > for these lookups instead of the task.
>
> Hi Todd,
>
> I just merged all three patches into selinux/next, thanks for your
> help patience on this patchset. Ultimately I merged these patches
> into selinux/next as opposed to selinux/stable-5.15 because I felt
> that a couple of weeks in -next before going to Linus would be a good
> thing. I'm also not certain how widespread binder is outside of
> Android so I figured the practical difference between next and
> stable-5.15 is likely very small. Regardless, all of your Fixes and
> stable tags remain in the patches so as soon as they go up to Linus
> during the next merge window the stable folks will be notified.
Thanks Paul. This all sounds fine.
>
> --
> paul moore
> http://www.paul-moore.com