Re: fuse doesn't use security_inode_init_security?

Re: fuse doesn't use security_inode_init_security?

[Miklos Szeredi]

On Fri, May 1, 2020 at 8:55 AM Chirantan Ekbote <chirantan@chromium.org> wrote:
>
> Hello,
>
> I noticed that the fuse module doesn't currently call
> security_inode_init_security and I was wondering if there is a
> specific reason for that.  I found a patch from 2013[1] that would
> change fuse so that it would call that function but it doesn't appear
> that the patch was merged.
>
> For background: I currently have a virtio-fs server with a guest VM
> that wants to use selinux.  I was able to enable selinux support
> without much issue by adding
>
>     fs_use_xattr virtiofs u:object_r:labeledfs:s0;
>
> to the selinux policy in the guest.  This works for the most part
> except that `setfscreatecon` doesn't appear to work.  From what I can
> tell, this ends up writing to `/proc/[pid]/attr/fscreate` and the
> attributes actually get set via the `inode_init_security` lsm hook in
> selinux.  However, since fuse doesn't call
> `security_inode_init_security` the hook never runs so the
> file/directory doesn't have the right attributes.
>
> Is it safe to just call `security_inode_init_security` whenever fuse
> creates a new inode?  How does this affect non-virtiofs fuse servers?

Not sure,  Adding more Cc's.

I know there's a deadlock scenario with getxattr called on root inode
before mount returns, which causes a deadlock unless mount is run in
the background.  Current libfuse doesn't handle this, but I think some
fuse fs work around this by not using libfuse, or at least have some
special setup code (glusterfs? ceph-fuse? not sure...).  I also don't
know whether the ->inode_init_security hook is related to this or not.


Thanks,
Miklos

Re: fuse doesn't use security_inode_init_security?

[Stephen Smalley]

On Fri, May 1, 2020 at 3:54 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, May 1, 2020 at 8:55 AM Chirantan Ekbote <chirantan@chromium.org> wrote:
> >
> > Hello,
> >
> > I noticed that the fuse module doesn't currently call
> > security_inode_init_security and I was wondering if there is a
> > specific reason for that.  I found a patch from 2013[1] that would
> > change fuse so that it would call that function but it doesn't appear
> > that the patch was merged.
> >
> > For background: I currently have a virtio-fs server with a guest VM
> > that wants to use selinux.  I was able to enable selinux support
> > without much issue by adding
> >
> >     fs_use_xattr virtiofs u:object_r:labeledfs:s0;
> >
> > to the selinux policy in the guest.  This works for the most part
> > except that `setfscreatecon` doesn't appear to work.  From what I can
> > tell, this ends up writing to `/proc/[pid]/attr/fscreate` and the
> > attributes actually get set via the `inode_init_security` lsm hook in
> > selinux.  However, since fuse doesn't call
> > `security_inode_init_security` the hook never runs so the
> > file/directory doesn't have the right attributes.
> >
> > Is it safe to just call `security_inode_init_security` whenever fuse
> > creates a new inode?  How does this affect non-virtiofs fuse servers?
>
> Not sure,  Adding more Cc's.
>
> I know there's a deadlock scenario with getxattr called on root inode
> before mount returns, which causes a deadlock unless mount is run in
> the background.  Current libfuse doesn't handle this, but I think some
> fuse fs work around this by not using libfuse, or at least have some
> special setup code (glusterfs? ceph-fuse? not sure...).  I also don't
> know whether the ->inode_init_security hook is related to this or not.

(cc selinux list)

security_inode_init_security() calls the initxattrs callback to
actually set each xattr in the backing store (if any), so unless you
have a way to pass that to the daemon along with the create request
the attribute won't be persisted with the file.  Setting the xattrs is
supposed to be atomic with the file creation, not a separate
setxattr() operation after creating the file, similar to ACL
inheritance on new files.

Also possibly related
https://lore.kernel.org/selinux/6df9b58c-fe9b-28f3-c151-f77aa6dd67e7@tycho.nsa.gov/.

Re: fuse doesn't use security_inode_init_security?

[Chirantan Ekbote]

On Sat, May 2, 2020 at 12:46 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> I am wondering how would fuse initialize the security context of newly
> created file. One way seems to be that it passes that information
> as part of FUSE_CREATE/FUSE_MKNOD calls to server and server sets
> its "fscreate" accordingly and then creates new file. This is similar
> to virtiofsd changing its effective uid/gid to the fuse client so that
> file is created with caller's uid/gid. Seems to be selinux context for
> file creation probably should be handled similiarly.
>

How would the fuse driver get the correct context without going
through security_inode_init_security?

> Other method could be to first create new file and then new fuse
> commands to do setxattrs. But that will be racy as file will have
> some default label for sometime between creation and setxattr.
>
> Having said that, I have a question. How do you reconcile host selinux
> policy and guest selinux labels. I am assuming host selinux policy
> will have to know about guest labels so that it allows virtiofsd do
> set those labels? Dan, you might have more thoughts on this.
>

My understanding is that we currently merge the guest and host
policies so that all types in the guest are available on the host.
The host itself uses selinux in permissive mode.  The top-level
directory is also owned exclusively by the guest and host processes
don't have access to it.


On Sat, May 2, 2020 at 3:32 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
>
> (cc selinux list)
>
> security_inode_init_security() calls the initxattrs callback to
> actually set each xattr in the backing store (if any), so unless you
> have a way to pass that to the daemon along with the create request
> the attribute won't be persisted with the file.  Setting the xattrs is
> supposed to be atomic with the file creation, not a separate
> setxattr() operation after creating the file, similar to ACL
> inheritance on new files.
>

But it's not truly atomic, is it?  The underlying file system creates
the inode and then the inode_init_security selinux hook actually sets
the attributes.  What would happen if the computer lost power after
the file system driver created the inode but before the selinux hook
set the attributes?

> Also possibly related
> https://lore.kernel.org/selinux/6df9b58c-fe9b-28f3-c151-f77aa6dd67e7@tycho.nsa.gov/.

Interesting.  Let me pull out the relevant bits and respond inline.

> - deadlock during mount with userspace waiting for mount(2) to complete
> and the kernel blocked on requesting the security.selinux xattr of the
> root directory as part of superblock setup during mount

I haven't personally run into this.  It Just Works, except for the
fscreate issue.

> - there was an attempt to introduce distinctions based on filesystem
> "subtype" so that whitelisted fuse filesystems could have xattr support
> enabled when it was known that their userspace would handle mount(2)
> safely [3] but this was apparently always broken and later reverted [4].

I think we kind of side-stepped this issue.  The fstype for virtiofs
is "virtiofs" instead of something like "fuse.virtiofs" so there is no
subtype handling required.

> - there is the issue of trusting the fuse filesystem for its labeling
> information and for domain/context transitions

This is definitely an issue for regular fuse file systems.  However,
the virtiofs device has read/write access to all the VM's memory so
there isn't much the VM can do if it doesn't trust the device.



I guess what I'm trying to understand is: what are the issues with
having the fuse driver call the inode_init_security hooks?  Even if
it's not something that can be turned on by default in mainline, I'd
like to evaluate whether we can turn it on locally in our restricted
environment.

One issue is the lack of atomicity guarantees.  This is likely a
deal-breaker for general fuse usage.  However, I don't think it's an
issue for our restricted use of virtiofs because the attributes will
be set "atomically" from the guest userspace's perspective.  It won't
be atomic on the host side, but host processes don't have access to
those directories anyway.

Are there any other issues?

Thank you,
Chirantan

Re: fuse doesn't use security_inode_init_security?

[Stephen Smalley]

On Thu, May 7, 2020 at 3:53 AM Chirantan Ekbote <chirantan@chromium.org> wrote:
> On Sat, May 2, 2020 at 3:32 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> >
> > (cc selinux list)
> >
> > security_inode_init_security() calls the initxattrs callback to
> > actually set each xattr in the backing store (if any), so unless you
> > have a way to pass that to the daemon along with the create request
> > the attribute won't be persisted with the file.  Setting the xattrs is
> > supposed to be atomic with the file creation, not a separate
> > setxattr() operation after creating the file, similar to ACL
> > inheritance on new files.
> >
>
> But it's not truly atomic, is it?  The underlying file system creates
> the inode and then the inode_init_security selinux hook actually sets
> the attributes.  What would happen if the computer lost power after
> the file system driver created the inode but before the selinux hook
> set the attributes?

IIUC, in the case of ext4 and xfs at least, the setting of the
security.selinux xattr is performed in the same transaction as the
file create, so a crash will either yield a file that has its xattr
set or no file at all.  This is also true of POSIX ACLs.  Labeled NFS
(NFSv4.2) likewise is supposed to send the MAC label with the file
create request and either create it with that label or not create it
at all.  Note that nfs however uses security_dentry_init_security() to
get the MAC label since it doesn't yet have an inode and MAC labels
are a first class abstraction in NFS not merely xattrs.

> > - deadlock during mount with userspace waiting for mount(2) to complete
> > and the kernel blocked on requesting the security.selinux xattr of the
> > root directory as part of superblock setup during mount
>
> I haven't personally run into this.  It Just Works, except for the
> fscreate issue.

Yes, this can be worked around in your fuse daemon if it supports
handling getxattr during mount (e.g. multi-threaded, other threads can
service the getxattr request while the mount(2) is still in progress).
But not supported by stock fuse userspace IIUC.

> I guess what I'm trying to understand is: what are the issues with
> having the fuse driver call the inode_init_security hooks?  Even if
> it's not something that can be turned on by default in mainline, I'd
> like to evaluate whether we can turn it on locally in our restricted
> environment.
>
> One issue is the lack of atomicity guarantees.  This is likely a
> deal-breaker for general fuse usage.  However, I don't think it's an
> issue for our restricted use of virtiofs because the attributes will
> be set "atomically" from the guest userspace's perspective.  It won't
> be atomic on the host side, but host processes don't have access to
> those directories anyway.
>
> Are there any other issues?

I don't have a problem with fuse calling the hook (either
security_inode_init_security or security_dentry_init_security).  It is
just a question of what it will do with the result (i.e. what its
initxattr callback will do for the former or what it will do with the
returned label in the latter). Optimally it will take the label
information and bundle it up along with the create request to the
daemon, which can then handle it as a single transaction.  Failing
that, it needs to support setting the label in some manner during file
creation that at least provides atomicity with respect to the user of
the filesystem (the guest in your case).