| Message ID | 20211007004629.1113572-3-tkjos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | binder: use cred instead of task for security context | expand |
On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> 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 <tkjos@google.com> > Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> > Reported-by: kernel test robot <lkp@intel.com> > Cc: stable@vger.kernel.org # 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; }
On 10/11/2021 2:33 PM, Paul Moore wrote: > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> 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 <tkjos@google.com> >> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> >> Reported-by: kernel test robot <lkp@intel.com> >> Cc: stable@vger.kernel.org # 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 <casey@schaufler-ca.com> wrote: > On 10/11/2021 2:33 PM, Paul Moore wrote: > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos <tkjos@google.com> 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 <tkjos@google.com> > >> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> > >> Reported-by: kernel test robot <lkp@intel.com> > >> Cc: stable@vger.kernel.org # 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.
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 <tkjos@google.com> 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 <tkjos@google.com> > >> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> > >> Reported-by: kernel test robot <lkp@intel.com> > >> Cc: stable@vger.kernel.org # 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 <dan.carpenter@oracle.com> 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 <tkjos@google.com> 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 <tkjos@google.com> > > >> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > >> Reported-by: kernel test robot <lkp@intel.com> > > >> Cc: stable@vger.kernel.org # 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.
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;
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 <tkjos@google.com> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com> Reported-by: kernel test robot <lkp@intel.com> Cc: stable@vger.kernel.org # 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(-)