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

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

On 9/28/2021 5:49 AM, Vivek Goyal wrote:
> On Mon, Sep 27, 2021 at 02:45:13PM -0700, Casey Schaufler wrote:
> [..]
>>>>> I see that NFS and ceph are supporting single security label at
>>>>> the time of file creation and I added support for the same for
>>>>> fuse.
>>>> NFS took that course because the IETF refused for a very long time
>>>> to accept a name+value pair in the protocol. The implementation
>>>> was a compromise.
>>>>
>>>>> You seem to want to have capability to send multiple "name,value,len"
>>>>> tuples so that you can support multiple xattrs/labels down the
>>>>> line.
>>>> No, so I can do it now. Smack keeps multiple xattrs on filesystem objects.
>>>> 	security.SMACK64		- the "security label"
>>>> 	security.SMACK64EXEC		- the Smack label to run the program with
>>>> 	security.SMACK64TRANSMUTE	- controls labeling on files created
>>>>
>>>> There has been discussion about using additional attributes for things
>>>> like socket labeling.
>>>>
>>>> This isn't hypothetical. It's real today. 
>>> It is real from SMACK point of view but it is not real from 
>>> security_dentry_init_security() hook point of view. What's equivalent
>>> of that hook to support SMACK and multiple labels?
>> When multiple security modules support this hook they will
>> each get called. So where today security_dentry_init_security()
>> calls selinux_dentry_init_security(), in the future it will
>> also call any other <lsm>_dentry_init_security() hook that
>> is registered. No LSM infrastructure change required.
> I don't think security_dentry_init_security() can handle multiple
> security labels without change.
>
> int security_dentry_init_security(struct dentry *dentry, int mode,
>                                         const struct qstr *name, void **ctx,
>                                         u32 *ctxlen)
> {
>         return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
>                                 name, ctx, ctxlen);
> }
>
> It can reutrn only one security context. So most likely you will have
> to add another hook to return multiple security context and slowly
> deprecate this one.

That hasn't been the approach to date. Have a look at the stacking patches
I've been posting to see how the "interface_lsm" is being used.

> IOW, as of today security_dentry_init_security() can't return multiple
> security labels. In fact it does not even tell the caller what's the
> name of the xattr. So caller has no idea if this security label came
> from SELinux or some other LSM. So for all practical purposes this
> is a hook for getting SELinux label and does not scale to support
> other LSMs.

Yup. This is a case, like yours, where the developer from SELinux
could have created a general interface but instead decided that
it wasn't worth the bother. As a result I have to fix it for the
general case. Well, SELinux/RedHat doesn't care about stacking,
so I guess that's the way it goes.


>>>>> Even if I do that, I am not sure what to do with those xattrs at
>>>>> the other end. I am using /proc/thread-self/attr/fscreate to
>>>>> set the security attribute of file.
>>>> Either you don't realize that attr/fscreate is SELinux specific, or
>>>> you don't care, or possibly (and sadly) both.
>>> I do realize that it is SELinux specific and that's why I have raised
>>> the concern that it does not work with SMACK.
>>>
>>> What's the "fscreate" equivalent for SMACK so that I file server can
>>> set it before creation of file and get correct context file?
>> The Smack attribute will be inherited from the creating process.
>> There is no way to generally change the attribute of a file on
>> creation. The appropriateness of such a facility has been debated
>> long and loud over the years. SELinux, which implements so varied
>> a set of "security" controls opted for it. Smack, which sticks much
>> more closely to an access control model, considers it too dangerous.
>> You can change the Smack label with setxattr(1) if you have
>> CAP_MAC_ADMIN.
> Ok, calling setxattr() after file creation will make the operation
> non-atomic. Will be good if it continues to be atomic.

That's a known downside. If you run the daemon with a Smack label that
is generally not accessible (easy to do) to the public you can do it
safely.


>> If you really want the file created with a particular
>> Smack label you can change the process Smack label by writing to
>> /proc/self/attr/smack/current on newer kernels and /proc/self/attr/current
>> on older ones.
> I guess /proc/thread-self/attr/smack/current is the way to go in this
> context, when one wants to support SMACK.

Label flipping is pretty dangerous. I prefer the run-with-safe-label,
call setxattr() approach. It's explicit.


>>>>> https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html
>>>>>
>>>>> How will this work with multiple labels. I think you will have to
>>>>> extend fscreate or create new interface to be able to deal with it.
>>>> Yeah. That thread didn't go to the LSM mail list. It was essentially
>>>> kept within the RedHat SELinux community. It's no wonder everyone
>>>> involved thought that your approach is swell. No one who would get
>>>> goobsmacked by it was on the thread.
>>> My goal is to support SELinux at this point of time. If you goal is
>>> to support SMACK, feel free to send patches on top to support that.
>> It helps to know what's going on before it becomes a major overhaul.
> Fair enough.
>
>>> I sent kernel patches to LSM list to make it plenty clear that this
>>> interface only supports single label which is SELinux. So there is
>>> no hiding here. And when I am supporting only SELinux, making use
>>> of fscreate makes perfect sense to me.
>> I bet it does.
>>
>>>>> That's why I think that it seems premature that fuse interface be
>>>>> written to deal with multiple labels when rest of the infrastructure
>>>>> is not ready. It should be other way, instead. First rest of the
>>>>> infrastructure should be written and then all the users make use
>>>>> of new infra.
>>>> Today the LSM infrastructure allows a security module to use as many
>>>> xattrs as it likes. Again, Smack uses multiple security.* xattrs today.
>>> security_dentry_init_security() can handle that? If not, what's the
>>> equivalent.
>> Yes, it can.
> How? How will security_dentry_init_security() return multiple lables?
> It has parameters "u32 *ctxlen" and you can return only one. If you
> try to return multiple labels and return total length in "ctxlen",
> that does not help as you need to know length of individiual labels.
> So you need to know the names of xattrs too. Without that its not
> going to work.
>
> So no, security_dentry_init_security() can not handle multiple 
> security labels (and associated names). 

As I mentioned before, this is an example of why I don't want to
see yet another special-for-SELinux case.

>
> Vivek
>