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
drivers/android/binder.c | 11 +----------
include/linux/security.h | 4 ++++
2 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index ca599ebdea4a..989afd0804ca 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..f02cc0211b10 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new,
{
}
+static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
+{
+}
+
static inline int security_kernel_act_as(struct cred *cred, u32 secid)
{
return 0;
--
2.33.0.800.g4c38ced690-goog
On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <[email protected]> wrote:
>
> 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
>
> drivers/android/binder.c | 11 +----------
> include/linux/security.h | 4 ++++
> 2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index ca599ebdea4a..989afd0804ca 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..f02cc0211b10 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new,
> {
> }
>
> +static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +}
Since security_cred_getsecid() doesn't return an error code we should
probably set the secid to 0 in this case, for example:
static inline void security_cred_getsecid(...)
{
*secid = 0;
}
--
paul moore
http://www.paul-moore.com
On 10/11/2021 2:33 PM, Paul Moore wrote:
> On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <[email protected]> wrote:
>> 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
>>
>> drivers/android/binder.c | 11 +----------
>> include/linux/security.h | 4 ++++
>> 2 files changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index ca599ebdea4a..989afd0804ca 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..f02cc0211b10 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new,
>> {
>> }
>>
>> +static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
>> +{
>> +}
> Since security_cred_getsecid() doesn't return an error code we should
> probably set the secid to 0 in this case, for example:
>
> static inline void security_cred_getsecid(...)
> {
> *secid = 0;
> }
If CONFIG_SECURITY is unset there shouldn't be any case where
the secid value is ever used for anything. Are you suggesting that
it be set out of an abundance of caution?
On Mon, Oct 11, 2021 at 5:59 PM Casey Schaufler <[email protected]> wrote:
> On 10/11/2021 2:33 PM, Paul Moore wrote:
> > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <[email protected]> wrote:
> >> 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
> >>
> >> drivers/android/binder.c | 11 +----------
> >> include/linux/security.h | 4 ++++
> >> 2 files changed, 5 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> >> index ca599ebdea4a..989afd0804ca 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..f02cc0211b10 100644
> >> --- a/include/linux/security.h
> >> +++ b/include/linux/security.h
> >> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new,
> >> {
> >> }
> >>
> >> +static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
> >> +{
> >> +}
> >
> > Since security_cred_getsecid() doesn't return an error code we should
> > probably set the secid to 0 in this case, for example:
> >
> > static inline void security_cred_getsecid(...)
> > {
> > *secid = 0;
> > }
>
> If CONFIG_SECURITY is unset there shouldn't be any case where
> the secid value is ever used for anything. Are you suggesting that
> it be set out of an abundance of caution?
It follows a pattern with the other LSM hooks when !CONFIG_SECURITY,
and I'd much rather us keep things consistent.
--
paul moore
http://www.paul-moore.com
On Mon, Oct 11, 2021 at 02:59:13PM -0700, Casey Schaufler wrote:
> On 10/11/2021 2:33 PM, Paul Moore wrote:
> > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <[email protected]> wrote:
> >> 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
> >>
> >> drivers/android/binder.c | 11 +----------
> >> include/linux/security.h | 4 ++++
> >> 2 files changed, 5 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> >> index ca599ebdea4a..989afd0804ca 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..f02cc0211b10 100644
> >> --- a/include/linux/security.h
> >> +++ b/include/linux/security.h
> >> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new,
> >> {
> >> }
> >>
> >> +static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
> >> +{
> >> +}
> > Since security_cred_getsecid() doesn't return an error code we should
> > probably set the secid to 0 in this case, for example:
> >
> > static inline void security_cred_getsecid(...)
> > {
> > *secid = 0;
> > }
>
> If CONFIG_SECURITY is unset there shouldn't be any case where
> the secid value is ever used for anything. Are you suggesting that
> it be set out of an abundance of caution?
The security_secid_to_secctx() function is probably inlined so probably
KMSan will not warn about this. But Smatch will warn about passing
unitialized variables. You probably wouldn't recieve and email about
it, and I would just add an exception that security_cred_getsecid()
should be ignored.
regards,
dan carpenter
On Tue, Oct 12, 2021 at 5:41 AM Dan Carpenter <[email protected]> wrote:
>
> On Mon, Oct 11, 2021 at 02:59:13PM -0700, Casey Schaufler wrote:
> > On 10/11/2021 2:33 PM, Paul Moore wrote:
> > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <[email protected]> wrote:
> > >> 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
> > >>
> > >> drivers/android/binder.c | 11 +----------
> > >> include/linux/security.h | 4 ++++
> > >> 2 files changed, 5 insertions(+), 10 deletions(-)
> > >>
> > >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > >> index ca599ebdea4a..989afd0804ca 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..f02cc0211b10 100644
> > >> --- a/include/linux/security.h
> > >> +++ b/include/linux/security.h
> > >> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new,
> > >> {
> > >> }
> > >>
> > >> +static inline void security_cred_getsecid(const struct cred *c, u32 *secid)
> > >> +{
> > >> +}
> > > Since security_cred_getsecid() doesn't return an error code we should
> > > probably set the secid to 0 in this case, for example:
> > >
> > > static inline void security_cred_getsecid(...)
> > > {
> > > *secid = 0;
> > > }
> >
> > If CONFIG_SECURITY is unset there shouldn't be any case where
> > the secid value is ever used for anything. Are you suggesting that
> > it be set out of an abundance of caution?
>
> The security_secid_to_secctx() function is probably inlined so probably
> KMSan will not warn about this. But Smatch will warn about passing
> unitialized variables. You probably wouldn't recieve and email about
> it, and I would just add an exception that security_cred_getsecid()
> should be ignored.
I'd much rather just see the secid set to zero in the !CONFIG_SECURITY case.
--
paul moore
http://www.paul-moore.com