[3/3] Enable security.selinux in user namespaces

Submitted by Stefan Berger on June 22, 2017, 6:59 p.m.

Details

Message ID 1498157989-11814-4-git-send-email-stefanb@linux.vnet.ibm.com
State New
Series "Enable namespaced file capabilities"
Headers show

Commit Message

Stefan Berger June 22, 2017, 6:59 p.m.
Before the current modifications, SELinux extended attributes were
visible inside the user namespace but changes in patch 1 hid them.
This patch enables security.selinux in user namespaces and allows
them to be written to in the same way as security.capability.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 fs/xattr.c | 1 +
 1 file changed, 1 insertion(+)

Patch hide | download patch | download mbox

diff --git a/fs/xattr.c b/fs/xattr.c
index 045be85..37686ee 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -138,6 +138,7 @@  xattr_permission(struct inode *inode, const char *name, int mask)
  */
 static const char *const userns_xattrs[] = {
 	XATTR_NAME_CAPS,
+	XATTR_NAME_SELINUX,
 	NULL
 };
 

Comments

Stephen Smalley June 23, 2017, 8:30 p.m.
On Thu, 2017-06-22 at 14:59 -0400, Stefan Berger wrote:
> Before the current modifications, SELinux extended attributes were
> visible inside the user namespace but changes in patch 1 hid them.
> This patch enables security.selinux in user namespaces and allows
> them to be written to in the same way as security.capability.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  fs/xattr.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 045be85..37686ee 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -138,6 +138,7 @@ xattr_permission(struct inode *inode, const char
> *name, int mask)
>   */
>  static const char *const userns_xattrs[] = {
>  	XATTR_NAME_CAPS,
> +	XATTR_NAME_SELINUX,
>  	NULL
>  };
>  

(cc SELinux maintainers, curiously omitted from these patches)

I don't think this works for SELinux. You don't deal with actually
supporting multiple security.selinux attributes within SELinux itself
(and I'm not asking you to do so), and without such support, this can't
operate as intended. With these patches applied, IIUC, a setxattr() of
security.selinux within a userns will end up setting only security.seli
nux@uid=1000 on disk, but will then tell SELinux to update its in-core
security label to the new value (via security_inode_post_setxattr).
Meanwhile, on a subsequent getxattr(), you'll call
security_inode_getsecurity() with the security.selinux@uid=1000 name,
which will always fail because SELinux doesn't know anything about your
new scheme, and then you'll call the filesystem handler and returns its
value, which is no longer connected in any way to the actual label
being used by SELinux.  Also, SELinux itself makes calls to
__vfs_getxattr() and __vfs_setxattr_noperm(), and I don't think your
name remapping is correct in those cases.

You also can't hide security.selinux within user namespaces.  Today
userspace can get and set security.selinux attributes within user
namespaces (if allowed by policy), and further can specify the label to
use for new files via /proc/self/attr/fscreate, which unsurprisingly
isn't addressed by your changes.  Changing that would be a userspace
break.
Stefan Berger June 23, 2017, 11:41 p.m.
On 06/23/2017 04:30 PM, Stephen Smalley wrote:
> On Thu, 2017-06-22 at 14:59 -0400, Stefan Berger wrote:
>> Before the current modifications, SELinux extended attributes were
>> visible inside the user namespace but changes in patch 1 hid them.
>> This patch enables security.selinux in user namespaces and allows
>> them to be written to in the same way as security.capability.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   fs/xattr.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/xattr.c b/fs/xattr.c
>> index 045be85..37686ee 100644
>> --- a/fs/xattr.c
>> +++ b/fs/xattr.c
>> @@ -138,6 +138,7 @@ xattr_permission(struct inode *inode, const char
>> *name, int mask)
>>    */
>>   static const char *const userns_xattrs[] = {
>>   	XATTR_NAME_CAPS,
>> +	XATTR_NAME_SELINUX,
>>   	NULL
>>   };
>>   
> (cc SELinux maintainers, curiously omitted from these patches)
>
> I don't think this works for SELinux. You don't deal with actually
> supporting multiple security.selinux attributes within SELinux itself
> (and I'm not asking you to do so), and without such support, this can't
> operate as intended. With these patches applied, IIUC, a setxattr() of
> security.selinux within a userns will end up setting only security.seli
> nux@uid=1000 on disk, but will then tell SELinux to update its in-core
> security label to the new value (via security_inode_post_setxattr).
> Meanwhile, on a subsequent getxattr(), you'll call
> security_inode_getsecurity() with the security.selinux@uid=1000 name,
> which will always fail because SELinux doesn't know anything about your
> new scheme, and then you'll call the filesystem handler and returns its
> value, which is no longer connected in any way to the actual label
> being used by SELinux.  Also, SELinux itself makes calls to
> __vfs_getxattr() and __vfs_setxattr_noperm(), and I don't think your
> name remapping is correct in those cases.
>
> You also can't hide security.selinux within user namespaces.  Today
> userspace can get and set security.selinux attributes within user
> namespaces (if allowed by policy), and further can specify the label to
> use for new files via /proc/self/attr/fscreate, which unsurprisingly
> isn't addressed by your changes.  Changing that would be a userspace
> break.

I modified the 1st patch now in such a way that only security.capability 
is rewritten, security.selinux and all other ones remain untouched.

https://github.com/stefanberger/linux/commits/xattr_for_userns.v2

    Stefan