| Message ID | 20190904143248.7003-1-sds@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selinux: fix residual uses of current_security() for the SELinux blob | expand |
On 9/4/2019 7:32 AM, Stephen Smalley wrote: > We need to use selinux_cred() to fetch the SELinux cred blob instead > of directly using current->security or current_security(). There > were a couple of lingering uses of current_security() in the SELinux code > that were apparently missed during the earlier conversions. Thank you for finding this. > IIUC, this > would only manifest as a bug if multiple security modules including > SELinux are enabled and SELinux is not first in the lsm order. After > this change, there appear to be no other users of current_security() > in-tree; perhaps we should remove it altogether. I agree. > > Fixes: bbd3662a8348 ("Infrastructure management of the cred security blob") > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> Acked-by: Casey Schaufler <casey@schaufler-ca.com> > --- > security/selinux/hooks.c | 2 +- > security/selinux/include/objsec.h | 20 ++++++++++---------- > 2 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index d55571c585ff..f1b763eceef9 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3435,7 +3435,7 @@ static int selinux_inode_copy_up_xattr(const char *name) > static int selinux_kernfs_init_security(struct kernfs_node *kn_dir, > struct kernfs_node *kn) > { > - const struct task_security_struct *tsec = current_security(); > + const struct task_security_struct *tsec = selinux_cred(current_cred()); > u32 parent_sid, newsid, clen; > int rc; > char *context; > diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h > index 231262d8eac9..d2e00c7595dd 100644 > --- a/security/selinux/include/objsec.h > +++ b/security/selinux/include/objsec.h > @@ -40,16 +40,6 @@ struct task_security_struct { > u32 sockcreate_sid; /* fscreate SID */ > }; > > -/* > - * get the subjective security ID of the current task > - */ > -static inline u32 current_sid(void) > -{ > - const struct task_security_struct *tsec = current_security(); > - > - return tsec->sid; > -} > - > enum label_initialized { > LABEL_INVALID, /* invalid or not initialized */ > LABEL_INITIALIZED, /* initialized */ > @@ -188,4 +178,14 @@ static inline struct ipc_security_struct *selinux_ipc( > return ipc->security + selinux_blob_sizes.lbs_ipc; > } > > +/* > + * get the subjective security ID of the current task > + */ > +static inline u32 current_sid(void) > +{ > + const struct task_security_struct *tsec = selinux_cred(current_cred()); > + > + return tsec->sid; > +} > + > #endif /* _SELINUX_OBJSEC_H_ */
On 9/4/19 10:32 AM, Stephen Smalley wrote: > We need to use selinux_cred() to fetch the SELinux cred blob instead > of directly using current->security or current_security(). There > were a couple of lingering uses of current_security() in the SELinux code > that were apparently missed during the earlier conversions. IIUC, this > would only manifest as a bug if multiple security modules including > SELinux are enabled and SELinux is not first in the lsm order. To further qualify that, it would only manifest as a bug if multiple non-exclusive security modules that use the cred security blob are enabled and SELinux is not first. I don't think that is possible today in existing upstream kernels since the cred blob-using modules are currently all exclusive and tomoyo switched to using the task security blob instead. Obviously that will change if/when the stacking for AA support lands and may already be an issue in Ubuntu depending on what stacking patchsets and what lsm order it is using. > After > this change, there appear to be no other users of current_security() > in-tree; perhaps we should remove it altogether. > > Fixes: bbd3662a8348 ("Infrastructure management of the cred security blob") > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > security/selinux/hooks.c | 2 +- > security/selinux/include/objsec.h | 20 ++++++++++---------- > 2 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index d55571c585ff..f1b763eceef9 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3435,7 +3435,7 @@ static int selinux_inode_copy_up_xattr(const char *name) > static int selinux_kernfs_init_security(struct kernfs_node *kn_dir, > struct kernfs_node *kn) > { > - const struct task_security_struct *tsec = current_security(); > + const struct task_security_struct *tsec = selinux_cred(current_cred()); > u32 parent_sid, newsid, clen; > int rc; > char *context; > diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h > index 231262d8eac9..d2e00c7595dd 100644 > --- a/security/selinux/include/objsec.h > +++ b/security/selinux/include/objsec.h > @@ -40,16 +40,6 @@ struct task_security_struct { > u32 sockcreate_sid; /* fscreate SID */ > }; > > -/* > - * get the subjective security ID of the current task > - */ > -static inline u32 current_sid(void) > -{ > - const struct task_security_struct *tsec = current_security(); > - > - return tsec->sid; > -} > - > enum label_initialized { > LABEL_INVALID, /* invalid or not initialized */ > LABEL_INITIALIZED, /* initialized */ > @@ -188,4 +178,14 @@ static inline struct ipc_security_struct *selinux_ipc( > return ipc->security + selinux_blob_sizes.lbs_ipc; > } > > +/* > + * get the subjective security ID of the current task > + */ > +static inline u32 current_sid(void) > +{ > + const struct task_security_struct *tsec = selinux_cred(current_cred()); > + > + return tsec->sid; > +} > + > #endif /* _SELINUX_OBJSEC_H_ */ >
On 9/4/19 8:31 AM, Stephen Smalley wrote: > On 9/4/19 10:32 AM, Stephen Smalley wrote: >> We need to use selinux_cred() to fetch the SELinux cred blob instead >> of directly using current->security or current_security(). There >> were a couple of lingering uses of current_security() in the SELinux code >> that were apparently missed during the earlier conversions. IIUC, this >> would only manifest as a bug if multiple security modules including >> SELinux are enabled and SELinux is not first in the lsm order. > > To further qualify that, it would only manifest as a bug if multiple non-exclusive security modules that use the cred security blob are enabled and SELinux is not first. I don't think that is possible today in existing upstream kernels since the cred blob-using modules are currently all exclusive and tomoyo switched to using the task security blob instead. Obviously that will change if/when the stacking for AA support lands and may already be an issue in Ubuntu depending on what stacking patchsets and what lsm order it is using. Yes currently SELinux needs to be first in the stack for a few reasons. I haven't really played a lot with full SELinux and AA policy at the host level, instead focusing on running an Ubuntu container on an selinux based host. With SELinux and AA competing for userspace interfaces it currently only makes sense to specify SELinux first. I may have run into this bug when messing with the selinux namespacing patches to experiment with flipping the container order but there are enough other problems with that series that I can't say for sure. > >> After >> this change, there appear to be no other users of current_security() >> in-tree; perhaps we should remove it altogether. >> >> Fixes: bbd3662a8348 ("Infrastructure management of the cred security blob") >> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >> --- >> security/selinux/hooks.c | 2 +- >> security/selinux/include/objsec.h | 20 ++++++++++---------- >> 2 files changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index d55571c585ff..f1b763eceef9 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -3435,7 +3435,7 @@ static int selinux_inode_copy_up_xattr(const char *name) >> static int selinux_kernfs_init_security(struct kernfs_node *kn_dir, >> struct kernfs_node *kn) >> { >> - const struct task_security_struct *tsec = current_security(); >> + const struct task_security_struct *tsec = selinux_cred(current_cred()); >> u32 parent_sid, newsid, clen; >> int rc; >> char *context; >> diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h >> index 231262d8eac9..d2e00c7595dd 100644 >> --- a/security/selinux/include/objsec.h >> +++ b/security/selinux/include/objsec.h >> @@ -40,16 +40,6 @@ struct task_security_struct { >> u32 sockcreate_sid; /* fscreate SID */ >> }; >> -/* >> - * get the subjective security ID of the current task >> - */ >> -static inline u32 current_sid(void) >> -{ >> - const struct task_security_struct *tsec = current_security(); >> - >> - return tsec->sid; >> -} >> - >> enum label_initialized { >> LABEL_INVALID, /* invalid or not initialized */ >> LABEL_INITIALIZED, /* initialized */ >> @@ -188,4 +178,14 @@ static inline struct ipc_security_struct *selinux_ipc( >> return ipc->security + selinux_blob_sizes.lbs_ipc; >> } >> +/* >> + * get the subjective security ID of the current task >> + */ >> +static inline u32 current_sid(void) >> +{ >> + const struct task_security_struct *tsec = selinux_cred(current_cred()); >> + >> + return tsec->sid; >> +} >> + >> #endif /* _SELINUX_OBJSEC_H_ */ >> >
On Wed, 4 Sep 2019, Stephen Smalley wrote: > We need to use selinux_cred() to fetch the SELinux cred blob instead > of directly using current->security or current_security(). There > were a couple of lingering uses of current_security() in the SELinux code > that were apparently missed during the earlier conversions. IIUC, this > would only manifest as a bug if multiple security modules including > SELinux are enabled and SELinux is not first in the lsm order. After > this change, there appear to be no other users of current_security() > in-tree; perhaps we should remove it altogether. > > Fixes: bbd3662a8348 ("Infrastructure management of the cred security blob") > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> Reviewed-by: James Morris <jamorris@linux.microsoft.com>
On Wed, Sep 4, 2019 at 10:32 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > We need to use selinux_cred() to fetch the SELinux cred blob instead > of directly using current->security or current_security(). There > were a couple of lingering uses of current_security() in the SELinux code > that were apparently missed during the earlier conversions. IIUC, this > would only manifest as a bug if multiple security modules including > SELinux are enabled and SELinux is not first in the lsm order. After > this change, there appear to be no other users of current_security() > in-tree; perhaps we should remove it altogether. > > Fixes: bbd3662a8348 ("Infrastructure management of the cred security blob") > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > security/selinux/hooks.c | 2 +- > security/selinux/include/objsec.h | 20 ++++++++++---------- > 2 files changed, 11 insertions(+), 11 deletions(-) Thanks Stephen, and everyone who reviewed/commented on the patch. This looks fine to me too, and while it is a little late, I think there is value in getting this into the next merge window so I've gone ahead and merged this into selinux/next. As far as removing current_security is concerned, I also agree that removing it is probably a good idea. Does anyone object if I merge a follow-up patch via the SELinux tree (patch coming shortly)?
On 9/4/19 6:50 PM, Paul Moore wrote: > On Wed, Sep 4, 2019 at 10:32 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: >> We need to use selinux_cred() to fetch the SELinux cred blob instead >> of directly using current->security or current_security(). There >> were a couple of lingering uses of current_security() in the SELinux code >> that were apparently missed during the earlier conversions. IIUC, this >> would only manifest as a bug if multiple security modules including >> SELinux are enabled and SELinux is not first in the lsm order. After >> this change, there appear to be no other users of current_security() >> in-tree; perhaps we should remove it altogether. >> >> Fixes: bbd3662a8348 ("Infrastructure management of the cred security blob") >> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >> --- >> security/selinux/hooks.c | 2 +- >> security/selinux/include/objsec.h | 20 ++++++++++---------- >> 2 files changed, 11 insertions(+), 11 deletions(-) > > Thanks Stephen, and everyone who reviewed/commented on the patch. > This looks fine to me too, and while it is a little late, I think > there is value in getting this into the next merge window so I've gone > ahead and merged this into selinux/next. > > As far as removing current_security is concerned, I also agree that > removing it is probably a good idea. Does anyone object if I merge a > follow-up patch via the SELinux tree (patch coming shortly)? Obviously no objection to the follow-up patch. I had another concern though raised by this bug that I wanted to note. AFAICS, no SELinux maintainer ever acked or reviewed the commit that introduced the bug, or the other patches in that series. In fact that commit and many others in the series only have a single Reviewed-by line and no Acked-by lines at all. I guess the SELinux maintainers (self included) could/should have been more proactive but maybe there should have been a final poke at each of the security module maintainers to provide at least an ack before it was merged or at least sent up to Linus? I've also seen a tendency to assume that passing the selinux-testsuite suffices for ensuring that the patch is correct wrt SELinux. If it wasn't already clear, the selinux-testsuite is by no means complete in its coverage (contributions welcome; known gaps captured as open issues in github), so passing the testsuite is no substitute for code review. For the next and any future rounds of stacking support, I'm hoping we can be a bit more rigorous in our code review and testing requirements.
On Thu, 5 Sep 2019, Stephen Smalley wrote: > For the next and any future rounds of stacking support, I'm hoping we can be a > bit more rigorous in our code review and testing requirements. After the 5th iteration of the patchset and with everything having at least one trusted reviewer, I did ask the list if there were any objections and stated I would otherwise merge to v4.21: http://kernsec.org/pipermail/linux-security-module-archive/2018-December/010209.html Regardless, from now I'll also require signoffs from all of the major LSMs on these kinds of changes. I'd have to say I also have concerns about the direction of LSM stacking and what its final goal is. It seems to makes sense to be able to stack Apparmor inside SELinux or Smack, so that folk can use AA system containers on an SELinux or Smack host. We've not seen other proposed use-cases for full stacking materialize in a sustained and viable manner. Are there really any beyond AA inside a labeling LSM? Because if not I think this is a reasonable goal state for LSM stacking until we concretely know otherwise.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d55571c585ff..f1b763eceef9 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3435,7 +3435,7 @@ static int selinux_inode_copy_up_xattr(const char *name) static int selinux_kernfs_init_security(struct kernfs_node *kn_dir, struct kernfs_node *kn) { - const struct task_security_struct *tsec = current_security(); + const struct task_security_struct *tsec = selinux_cred(current_cred()); u32 parent_sid, newsid, clen; int rc; char *context; diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h index 231262d8eac9..d2e00c7595dd 100644 --- a/security/selinux/include/objsec.h +++ b/security/selinux/include/objsec.h @@ -40,16 +40,6 @@ struct task_security_struct { u32 sockcreate_sid; /* fscreate SID */ }; -/* - * get the subjective security ID of the current task - */ -static inline u32 current_sid(void) -{ - const struct task_security_struct *tsec = current_security(); - - return tsec->sid; -} - enum label_initialized { LABEL_INVALID, /* invalid or not initialized */ LABEL_INITIALIZED, /* initialized */ @@ -188,4 +178,14 @@ static inline struct ipc_security_struct *selinux_ipc( return ipc->security + selinux_blob_sizes.lbs_ipc; } +/* + * get the subjective security ID of the current task + */ +static inline u32 current_sid(void) +{ + const struct task_security_struct *tsec = selinux_cred(current_cred()); + + return tsec->sid; +} + #endif /* _SELINUX_OBJSEC_H_ */
We need to use selinux_cred() to fetch the SELinux cred blob instead of directly using current->security or current_security(). There were a couple of lingering uses of current_security() in the SELinux code that were apparently missed during the earlier conversions. IIUC, this would only manifest as a bug if multiple security modules including SELinux are enabled and SELinux is not first in the lsm order. After this change, there appear to be no other users of current_security() in-tree; perhaps we should remove it altogether. Fixes: bbd3662a8348 ("Infrastructure management of the cred security blob") Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> --- security/selinux/hooks.c | 2 +- security/selinux/include/objsec.h | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-)