Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler <casey@schaufler-ca.com>]

On 9/24/2021 2:16 PM, Vivek Goyal wrote:
> On Fri, Sep 24, 2021 at 01:54:20PM -0700, Casey Schaufler wrote:
>> On 9/24/2021 1:18 PM, Vivek Goyal wrote:
>>> On Fri, Sep 24, 2021 at 12:58:28PM -0700, Casey Schaufler wrote:
>>>> On 9/24/2021 12:24 PM, Vivek Goyal wrote:
>>>>> When a new inode is created, send its security context to server along
>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>>>> This gives server an opportunity to create new file and set security
>>>>> context (possibly atomically). In all the configurations it might not
>>>>> be possible to set context atomically.
>>>>>
>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>>>
>>>>> Following is the information sent to server.
>>>>>
>>>>> - struct fuse_secctx.
>>>>>   This contains total size of security context which follows this structure.
>>>>>
>>>>> - xattr name string.
>>>>>   This string represents name of xattr which should be used while setting
>>>>>   security context. As of now it is hardcoded to "security.selinux".
>>>> Why? It's not like "security.SMACK64' is a secret.
>>> Sorry, I don't understand what's the concern. Can you elaborate a bit
>>> more.
>> Sure. Interfaces that are designed as special case solutions for
>> SELinux tend to make my life miserable as the Smack maintainer and
>> for the efforts to complete LSM stacking. You make the change for
>> SELinux and leave the generalization as an exercise for some poor
>> sod like me to deal with later.
> I am not expecting you do to fuse work. Once you add the new security
> hook which can return multiple labels, I will gladly do fuse work
> to send multiple labels.
>
>>> I am hardcoding name to "security.selinux" because as of now only
>>> SELinux implements this hook.
>> Yes. A Smack hook implementation is on the todo list. If you hard code
>> this in fuse you're adding another thing that has to be done for
>> Smack support.
>>
>>>  And there is no way to know the name
>>> of xattr, so I have had to hardcode it. But tomorrow if interface
>>> changes so that name of xattr is also returned, we can easily get
>>> rid of hardcoding.
>> So why not make the interface do that now?
> Because its unnecessary complexity for me. When multiple label support
> is not even there, I need to write and test code to support multiple
> labels when support is not even there.

Subsystems that treat labels as a special case (as opposed to supporting xattrs
properly) make me sad.


>>> If another LSM decides to implement this hook, then we can send
>>> that name as well. Say "security.SMACK64".
>> Again, why not make it work that way now, and avoid having
>> to change the protocol later? Changing protocols and interfaces
>> is much harder than doing them generally in the first place.
> In case of fuse, it is not that complicated to change protocol and
> add new options. Once you add support for smack and multiple labels,
> I will gladly change fuse to be able to accomodate that.

Cool. I'll hold you to that. Priority has been bumped up.



>>>>> - security context.
>>>>>   This is the actual security context whose size is specified in fuse_secctx
>>>>>   struct.
>>>> The possibility of multiple security contexts on a file is real
>>>> in the not too distant future. Also, a file can have multiple relevant
>>>> security attributes at creation. Smack, for example, may assign a
>>>> security.SMACK64 and a security.SMACK64TRANSMUTE attribute. Your
>>>> interface cannot support either of these cases.
>>> Right. As of now it does not support capability to support multiple
>>> security context. But we should be easily able to extend the protocol
>>> whenever that supports lands in kernel.
>> No. Extending single data item protocols to support multiple
>> data items *hurts* most of the time. If it wasn't so much more
>> complicated you'd be doing it up front without fussing about it.
> Its unnecessary work at this point of time. Once multiple labels
> are supported, I can do this work.
>
> I think we will need to send extra structure which tells how many
> labels are going to follow. And then all the labels will follow
> with same format I am using for single label.
>
> struct fuse_secctx; xattr name string; actual label
>
>>>  Say a new option
>>> FUSE_SECURITY_CTX_EXT which will allow sending multiple security
>>> context labels (along with associated xattr names).
>>>
>>> As of now there is no need to increase the complexity of current
>>> implementation both in fuse as well as virtiofsd when kernel
>>> does not even support multiple lables using security_dentry_init_security()
>>> hook.
>> You're 100% correct. For your purpose today there's no reason to
>> do anything else. It would be really handy if I didn't have yet
>> another thing that I don't have the time to rewrite.
> I can help with adding fuse support once smack supports it. Right now
> I can't even test it even if I sign up for extra complexity.
>
> Vivek
>