selinux: fix residual uses of current_security() for the SELinux blob
diff mbox series

Message ID 20190904143248.7003-1-sds@tycho.nsa.gov
State New
Headers show
Series
  • selinux: fix residual uses of current_security() for the SELinux blob
Related show

Commit Message

Stephen Smalley Sept. 4, 2019, 2:32 p.m. UTC
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(-)

Comments

Casey Schaufler Sept. 4, 2019, 3:16 p.m. UTC | #1
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_ */
Stephen Smalley Sept. 4, 2019, 3:31 p.m. UTC | #2
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_ */
>
John Johansen Sept. 4, 2019, 4:46 p.m. UTC | #3
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_ */
>>
>
James Morris Sept. 4, 2019, 7:35 p.m. UTC | #4
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>
Paul Moore Sept. 4, 2019, 10:50 p.m. UTC | #5
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)?
Stephen Smalley Sept. 5, 2019, 8:01 p.m. UTC | #6
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.
James Morris Sept. 5, 2019, 8:55 p.m. UTC | #7
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.

Patch
diff mbox series

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_ */