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

[Vivek Goyal <vgoyal@redhat.com>]

On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
> > On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
> >> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
> >>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
> >>>> On Fri, Sep 24, 2021, at 3: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".
> >>>> Any reason not to just send all `security.*` xattrs found on the inode? 
> >>>>
> >>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
> >>> So this inode is about to be created. There are no xattrs yet. And
> >>> filesystem is asking LSMs, what security labels should be set on this
> >>> inode before it is published. 
> >> No. That's imprecise. It's what SELinux does. An LSM can add any
> >> number of attributes on inode creation, or none. These attributes
> >> may or may not be "security labels". Assuming that they are is the
> >> kind of thinking that leads people like Linus to conclude that the
> >> LSM community is clueless.
> >>
> >>
> >>> For local filesystems it is somewhat easy. They are the one creating
> >>> inode and can set all xattrs/labels before inode is added to inode
> >>> cache.
> >>>
> >>> But for remote like filesystems, it is more tricky. Actual inode
> >>> creation first will happen on server and then client will instantiate
> >>> an inode based on information returned by server (Atleast that's
> >>> what fuse does).
> >>>
> >>> So security_dentry_init_security() was created (I think by NFS folks)
> >>> so that they can query the label and send it along with create
> >>> request and server can take care of setting label (along with file
> >>> creation).
> >>>
> >>> One limitation of security_dentry_init_security() is that it practically
> >>> supports only one label. And only SELinux has implemented. So for
> >>> all practical purposes this is a hook to obtain selinux label. NFS
> >>> and ceph already use it in that way.
> >>>
> >>> Now there is a desire to be able to return more than one security
> >>> labels and support smack and possibly other LSMs. Sure, that great.
> >>> But I think for that we will have to implement a new hook which
> >>> can return multiple labels and filesystems like nfs, ceph and fuse
> >>> will have to be modified to cope with this new hook to support
> >>> multiple lables. 
> >>>
> >>> And I am arguing that we can modify fuse when that hook has been
> >>> implemented. There is no point in adding that complexity in fuse
> >>> code as well all fuse-server implementations when there is nobody
> >>> generating multiple labels. We can't even test it.
> >> There's a little bit of chicken-and-egg going on here.
> >> There's no point in accommodating multiple labels in
> >> this code because you can't have multiple labels. There's
> >> no point in trying to support multiple labels because
> >> you can't use them in virtiofs and a bunch of other
> >> places.
> > Once security subsystem provides a hook to support multiple lables, then
> > atleast one filesystem will have to be converted to make use of this new
> > hook at the same time and rest of the filesystems can catch up later.
> 
> Clearly you haven't been following the work I've been doing on
> module stacking. That's completely understandable. There aren't
> new hooks being added, or at least haven't been yet. Some of the
> existing hooks are getting changed to provide the data required
> for multiple security modules (e.g. secids become a set of secids).
> Filesystems that support xattrs properly are unaffected because,
> for all it's shortcomings, the LSM layer hides the details of
> the security modules sufficiently. 
> 
> Which filesystem are you saying will have to "be converted"?

When I grep for "security_dentry_init_security()" in current code,
I see two users, ceph and nfs.

fs/ceph/xattr.c
ceph_security_init_secctx()

fs/nfs/nfs4proc.c
nfs4_label_init_security()

So looks like these two file systems will have to be converted
(along with fuse).

Vivek

> NFS is going to require some work, but that's because it was
> done as a special case for "MAC labels". The NFS support for
> security.* xattrs came much later. This is one of the reasons
> why I'm concerned about the virtiofs implementation you're
> proposing. We were never able to get the NFS "MAC label"
> implementation to work properly with Smack, even though there is
> no obvious reason it wouldn't.
> 
> 
> >
> > Vivek
> >
>