Re: [PATCH] selinux: fix residual uses of current_security() for the SELinux blob

[John Johansen <john.johansen@canonical.com>]

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