Re: [PATCH 6/7] selinux: Ignore security labels on user namespace mounts

Stephen Smalley
2015-07-23T15:37:50Z

On 07/23/2015 10:39 AM, Seth Forshee wrote:
> On Thu, Jul 23, 2015 at 09:57:20AM -0400, Stephen Smalley wrote:
>> On 07/22/2015 04:40 PM, Stephen Smalley wrote:
>>> On 07/22/2015 04:25 PM, Stephen Smalley wrote:
>>>> On 07/22/2015 12:14 PM, Seth Forshee wrote:
>>>>> On Wed, Jul 22, 2015 at 12:02:13PM -0400, Stephen Smalley wrote:
>>>>>> On 07/16/2015 09:23 AM, Stephen Smalley wrote:
>>>>>>> On 07/15/2015 03:46 PM, Seth Forshee wrote:
>>>>>>>> Unprivileged users should not be able to supply security labels
>>>>>>>> in filesystems, nor should they be able to supply security
>>>>>>>> contexts in unprivileged mounts. For any mount where s_user_ns is
>>>>>>>> not init_user_ns, force the use of SECURITY_FS_USE_NONE behavior
>>>>>>>> and return EPERM if any contexts are supplied in the mount
>>>>>>>> options.
>>>>>>>>
>>>>>>>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>>>>>>>
>>>>>>> I think this is obsoleted by the subsequent discussion, but just for the
>>>>>>> record: this patch would cause the files in the userns mount to be left
>>>>>>> with the "unlabeled" label, and therefore under typical policies,
>>>>>>> completely inaccessible to any process in a confined domain.
>>>>>>
>>>>>> The right way to handle this for SELinux would be to automatically use
>>>>>> mountpoint labeling (SECURITY_FS_USE_MNTPOINT, normally set by
>>>>>> specifying a context= mount option), with the sbsec->mntpoint_sid set
>>>>>> from some related object (e.g. the block device file context, as in your
>>>>>> patches for Smack). That will cause SELinux to use that value instead
>>>>>> of any xattr value from the filesystem and will cause attempts by
>>>>>> userspace to set the security.selinux xattr to fail on that filesystem.
>>>>>> That is how SELinux normally deals with untrusted filesystems, except
>>>>>> that it is normally specified as a mount option by a trusted mounting
>>>>>> process, whereas in your case you need to automatically set it.
>>>>>
>>>>> Excellent, thank you for the advice. I'll start on this when I've
>>>>> finished with Smack.
>>>>
>>>> Not tested, but something like this should work. Note that it should
>>>> come after the call to security_fs_use() so we know whether SELinux
>>>> would even try to use xattrs supplied by the filesystem in the first place.
>>>>
>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>> index 564079c..84da3a2 100644
>>>> --- a/security/selinux/hooks.c
>>>> +++ b/security/selinux/hooks.c
>>>> @@ -745,6 +745,30 @@ static int selinux_set_mnt_opts(struct super_block *sb,
>>>> goto out;
>>>> }
>>>> }
>>>> +
>>>> + /*
>>>> + * If this is a user namespace mount, no contexts are allowed
>>>> + * on the command line and security labels must be ignored.
>>>> + */
>>>> + if (sb->s_user_ns != &init_user_ns) {
>>>> + if (context_sid || fscontext_sid || rootcontext_sid ||
>>>> + defcontext_sid) {
>>>> + rc = -EACCES;
>>>> + goto out;
>>>> + }
>>>> + if (sbsec->behavior == SECURITY_FS_USE_XATTR) {
>>>> + struct block_device *bdev = sb->s_bdev;
>>>> + sbsec->behavior = SECURITY_FS_USE_MNTPOINT;
>>>> + if (bdev) {
>>>> + struct inode_security_struct *isec =
>>>> bdev->bd_inode;
>>>
>>> That should be bdev->bd_inode->i_security.
>>
>> Sorry, this won't work. bd_inode is not the inode of the block device
>> file that was passed to mount, and it isn't labeled in any way. It will
>> just be unlabeled.
>>
>> So I guess the only real option here as a fallback is
>> sbsec->mntpoint_sid = current_sid(). Which isn't great either, as the
>> only case where we currently assign task labels to files is for their
>> /proc/pid inodes, and no current policy will therefore allow create
>> permission to such files.
>
> Darn, you're right, that isn't the inode we want. There really doesn't
> seem to be any way to get back to the one we want from the LSM, short of
> adding a new hook.

Maybe list_first_entry(&sb->s_bdev->bd_inodes, struct inode, i_devices)?
Feels like a layering violation though...