interfaces: implement a fuse interface

[stgraber]

I wrote that while working on the LXD snap. We won't actually be needing
it but figured it'd be useful to others.

Signed-off-by: Stéphane Graber stgraber@ubuntu.com

[zyga]

whitelist this please

[zyga]

Apart from anything else, can you please patch docs/interfaces.md with the appropriate description?

[stgraber]

Oh yeah, I'll do that. Must have missed it during my random grepping around to find what to change :)

[stgraber]

docs/interfaces.md done

[zyga]

Looks good.

It would be good to add a note on how this can be tested. We have a file called integration-tests/manual-tests.md that can be used if the procedure is complicated. Ideally we would have a spread test that installs a snap using this interface and actually tries to do something.

[ogra1]

https://bugs.launchpad.net/bugs/1603113 is the related wishlist bug btw

[stgraber]

Yeah, so I expect the right way to test this would be to install a snap using it, so chicken and egg problem going on there :)

There is no standalone tool to test fuse other than actually running a fuse filesystem, at which point you may as well make a snap out of it.

Maybe @ogra1 can do that with sshfs and then a test can be added to snapd to check that the sshfs snap works.

[stgraber]

@ogra1 care to contribute a test for this interface?

[fgimenez]

@stgraber @ogra1 hey, fwiw I'm about to finish a simple spread test for this interface, including a very basic snap declaring a plug on it, I can propose it to this branch so that it can be added before merging.

Thanks! :)

[stgraber]

@fgimenez that'd be great, thanks!

[fgimenez]

Test proposed in stgraber/snapd#1

[stgraber]

@fgimenez could you test the branch again now? I don't have an environment where I can easily test this right now.

It looks like the spread test is hanging in travis so I can't really rely on that either.

[niemeyer]

This should use language that is similar to the other entries. I don't particularly like either style ("you", or "Can foo"), but consistency wins.

[niemeyer]

Why do we need that? We know exactly what lives where in this context, and the guesses made in that file don't look sensible for the snap environment.

[niemeyer]

Let's please drop this module altogether, and do exactly what needs to be done here, if anything is necessary at all. And then, let's not ignore errors in that logic as done below.

[niemeyer]

The test breakages are actually about this branch itself, so need looking and addressing:

https://travis-ci.org/snapcore/snapd/jobs/149297126

There are also conflicts.

[stgraber]

@fgimenez I did the rebase and got the unit test to pass again. Can you look at the spread result, see if it's something wrong with the interface or with the test?

[fgimenez]

hey @stgraber I've proposed stgraber/snapd#3, which should resolve the current problems. It seems that somehow the previous branch changes didn't merge correctly, they are included in this PR plus more enhancements addressing the review comments.

However with these changes I'm getting a Bad system call error in the test, this comes from dmesg:

[ 1345.435133] audit: type=1400 audit(1470303064.052:29): apparmor="DENIED" operation="open" profile="snap.test-snapd-fuse-consumer.create" name="/dev/fuse" pid=27588 comm="python" requested_mask="wr" denied_mask="wr" fsuid=1000 ouid=0
[ 1345.516318] audit: type=1400 audit(1470303064.136:30): apparmor="STATUS" operation="profile_replace" profile="unconfined" name="snap.test-snapd-fuse-consumer.create" pid=27595 comm="apparmor_parser"
[ 1345.656496] audit: type=1326 audit(1470303064.276:31): auid=0 uid=1000 gid=1000 ses=6 pid=27598 comm="python" exe="/snap/test-snapd-fuse-consumer/2/usr/bin/python2.7" sig=31 arch=c000003e syscall=47 compat=0 ip=0x7f86558c8980 code=0x0
[ 1345.662912] audit: type=1326 audit(1470303064.280:32): auid=0 uid=1000 gid=1000 ses=6 pid=27599 comm="fusermount" exe="/snap/test-snapd-fuse-consumer/2/bin/fusermount" sig=31 arch=c000003e syscall=122 compat=0 ip=0x7ff02ed29777 code=0x0

Currently the checks use the test user to try the creation of the filesystem, if they are changed to use root then I get from dmesg:

[ 2225.918284] audit: type=1400 audit(1470303944.509:35): apparmor="DENIED" operation="open" profile="snap.test-snapd-fuse-consumer.create" name="/dev/fuse" pid=30515 comm="python" requested_mask="wr" denied_mask="wr" fsuid=0 ouid=0
[ 2226.003512] audit: type=1400 audit(1470303944.593:36): apparmor="STATUS" operation="profile_replace" profile="unconfined" name="snap.test-snapd-fuse-consumer.create" pid=30523 comm="apparmor_parser"
[ 2226.132900] audit: type=1400 audit(1470303944.721:37): apparmor="DENIED" operation="capable" profile="snap.test-snapd-fuse-consumer.create" pid=30524 comm="python" capability=21  capname="sys_admin"
[ 2226.133495] audit: type=1326 audit(1470303944.725:38): auid=0 uid=0 gid=0 ses=9 pid=30524 comm="python" exe="/snap/test-snapd-fuse-consumer/2/usr/bin/python2.7" sig=31 arch=c000003e syscall=47 compat=0 ip=0x7f757f0ac980 code=0x0
[ 2226.138886] audit: type=1400 audit(1470303944.729:39): apparmor="DENIED" operation="open" profile="snap.test-snapd-fuse-consumer.create" name="/etc/fuse.conf" pid=30525 comm="fusermount" requested_mask="r" denied_mask="r" fsuid=0 ouid=0

Please let me know if I can be of any help debugging this, to reproduce you can try to install the test-snapd-fuse-consumer snap in xenial or yakkety with sudo snap install test-snapd-fuse-consumer and then try to create the hello fuse filesystem with test-snapd-fuse-consumer.create <mount_point>.

Cheers,

[fgimenez]

@stgraber the fuse test is passing! \o/ the error you are getting in the upgrade test is fixed in master, merging should fix it.

Thanks! :)

[stgraber]

and test is passing!

[niemeyer]

Review comment still pending here.

[niemeyer]

One trivial comment still pending.

@jdstrand Would appreciate a security review on this one when you have a moment.

[stgraber]

Fixed the description, rebased on master yet again and squashed some commits together to keep things clean. Hopefully travis will be happy.

[stgraber]

ping

[stgraber]

I guess this is stuck on @jdstrand right now.

[jdstrand]

Can you update this for the format in the recently committed interface documentation improvements PR?

[stgraber]

done

[jdstrand]

Can you also add a test to all_test.go?

[stgraber]

fixing

[jdstrand]

Can you put this '`' on a line by itself so we have proper spacing between connected interfaces?

[stgraber]

fixing

[jdstrand]

Because of the way interfaces work where we build up the whitelist in arbitrary ways and because deny rules override allow, we prefer to not use deny rules. Is there a reason why you have an explicit deny rule?

[stgraber]

Not really, just shutting up a denial. Removing it will make dmesg a bit noisier but won't change anything as far as functionality.

[stgraber]

fixing

[jdstrand]

This says 'as root only', but down below you say 'Can run an unprivileged FUSE filesystem.'. These descriptions seem at odds.

[stgraber]

fixing

[stgraber]

@jdstrand that should be all review comments addresses. Let's see if travis is still happy.

[jdstrand]

While I gave a few high-level comments, I'd like to take a close look at fuse to better understand what this interface should look like.

[stgraber]

@jdstrand any idea when you'll be doing that? I'm getting a bit tired having to rebase this almost once a day to cope with other changes that you guys are merging. Especially given that I don't actually need this interface myself anymore :)

[jdstrand]

@stgraber - I started yesterday and am continuing today and likely will continue into tomorrow. It is important that we understand the security implications of the interface before we allow it and I need to perform due diligence.

[niemeyer]

@stgraber Please consider the cost of rebasing compared to the cost of doing security review, code review, writing tests for it, etc. It's not like this is sitting idle on a queue.

[jdstrand]

FYI, good progress has been made but the investigation continues. I can say that assuming everything checks out with the investigation, the policy will need to be fine-tuned a bit. I'll post that with my findings.

[jdstrand]

SUMMARY: I took some time to look at this and believe that a fuse interface is ok in general and with the proper interface policy that is detailed in this response, can be manually connected at this time. This investigation was shared and discussed with the security team. After discussions with upstream and/or a deep audit we may be able to allow auto-connect of the interface.

The kernel mediates the path of the actual file, not the path of the fuse mount (ie mount '/' on SNAP_DATA/mnt, then access to SNAP_DATA/mnt/etc/shadow is mediated as /etc/shadow. This is true of nested local mounts too) so there are no sandbox escapes with using this interface.

Per upstream documentation, when using fusermount, mounts work as one would expect and they are mounted with fuse imposing the following restrictions:

• The user can only mount on a mountpoint, for which it has write permission
• The mountpoint is not a sticky directory which isn't owned by the user (like /tmp usually is)
• No other user (including root) can access the contents of the mounted filesystem (though this can be relaxed by allowing the use of the allow_other and allow_root mount options in fuse.conf)

Our fuse interface should not allow setting allow_other or allow_root so we should not allow reading /etc/fuse.conf.

Unmounts and fuse aborts should not be in the main policy because our mediation is not fine-grained enough. Most fuse apps don't need to unmount their own mounts so this shouldn't be an issue. For those that do, we can introduce a 'fuse-control' interface that allows the unmounts and fuse aborts.

fusermount is a userspace tool that is setuid in Ubuntu so that it has CAP_SYS_ADMIN and can perform mount operations for unprivileged users. It is not present in the core snap today and snaps can't (currently) ship setuid binaries, so unprivileged fuse mounts won't be supported at this time. If we want to support this, the core snap could ship fusermount and the security policy for the main interface could add this rule: /{,usr/}bin/fusermount ixr,

Importantly, because fusermount is not included in the core snap and because root processes are under no obligation to use it (or libfuse), we need to enforce strict mount options (ie, nosuid and nodev) and mount paths.

Furthermore, the snap-confine command creates a new mount namespace so the fuse mounts aren’t accessible to other processes outside of the app’s process, which somewhat limits the utility of the interface but it also offers some protection against misbehaving fuse filesystems since the files won’t be available in the global mount namespace or the mount namespaces of other snaps.

Fuse has a long security history and the security team does not feel that the interface should be auto-connected at this time. It is believed that the interface policy detailed below covers many attacks, but a discussion with upstream and/or a detailed audit of the fuse design and kernel interface for resiliency against crafted filesystems and libfuse needs to happen before it can be considered for auto-connect.

As such, the PR should:

• rename as fuse-support
• fuse-support must not auto-connect
• fuse-support apparmor policy should be:

const fuseSupportConnectedPlugAppArmor = `
# Description: Can run a FUSE filesystem. Unprivileged fuse mounts are
# not supported at this time.

# Allow communicating with fuse kernel driver
# https://www.kernel.org/doc/Documentation/filesystems/fuse.txt
/dev/fuse rw,

# Required for mounts
capability sys_admin,

# Allow mounts to our snap-specific writable directories
# Note 1: fstype is 'fuse.<command>', eg 'fuse.sshfs'
# Note 2: due to LP: #1612393 - @{HOME} can't be used in mountpoint
# Note 3: local fuse mounts of filesystem directories are mediated by 
#         AppArmor. The actual underlying file in the source directory is
#         mediated, not the presentation layer of the target directory, so
#         we can safely allow all local mounts to our snap-specific writable
#         directories.
# Note 4: fuse supports a lot of different mount options, and applications
#         are not obligated to use fusermount to mount fuse filesystems, so
#         be very strict and only support the default (rw,nosuid,nodev) and
#         read-only.
mount fstype=fuse.* options=(ro,nosuid,nodev) ** -> /home/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**/},
mount fstype=fuse.* options=(rw,nosuid,nodev) ** -> /home/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**/},
mount fstype=fuse.* options=(ro,nosuid,nodev) ** -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**/},
mount fstype=fuse.* options=(rw,nosuid,nodev) ** -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**/},

# Explicitly deny reads to /etc/fuse.conf. We do this to ensure that
# the safe defaults of fuse are used (which are enforced by our mount
# rules) and not system-specific options from /etc/fuse.conf that
# may conflict with our mount rules.
deny /etc/fuse.conf r,

# Allow read access to the fuse filesystem
/sys/fs/fuse/ r,
/sys/fs/fuse/** r,

# Unprivileged fuser mounts must use the setuid helper in the core snap
# (not currently available, so don't include in policy at this time).
#/{,usr/}bin/fusermount ixr,
`

• fuse-support seccomp policy should be:

const fuseSupportConnectedPlugSecComp = `
# Description: Can run a FUSE filesystem. Unprivileged fuse mounts are
# not supported at this time.

mount
# for communicating with kernel
recvmsg
`

• if required, implement a separate fuse-control interface that must manually connect
• fuse-control apparmor policy should be:

const fuseControlConnectedPlugAppArmor = `
# Description: Can unmount and abort FUSE filesystems. This interface
# provides privileged access to mounts in snap mount directories

/bin/umount ixr,
owner @{PROC}/[0-9]*/mounts r,
owner @{PROC}/[0-9]*/mountinfo r,
/run/mount/utab rw,

# NOTE: due to LP: #1613403, fstype is not mediated and as such, these rules
# allow, for example, unmounting bind mounts from the content interface
umount fstype=fuse.* /home/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**/},
umount fstype=fuse.* /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**/},

# Writes to /sys/fs/fuse/connections/[0-9]*/abort cause the mount to be aborted
# immediately. Currently there is no way to map the integer to the policy, so
# allowing writes here would allow tampering with other snaps' fuse mounts.
/sys/fs/fuse/connections/[0-9]*/abort w,
`

• fuse-control seccomp policy should be:

const fuseControlConnectedPlugSecComp = `
# Description: Can unmount and abort FUSE filesystems. This interface
# provides privileged access to mounts in snap mount directories
umount
umount2
`

[stgraber]

@fgimenez can you update the snap in the store to use the new interface name?

[fgimenez]

@stgraber done, btw you should change https://github.com/snapcore/snapd/pull/1598/files#diff-a0b5529dbb07412519ba3f91fffda35dR33 to check for $DISCONNECTED_PATTERN, and perhaps add here https://github.com/snapcore/snapd/pull/1598/files#diff-a0b5529dbb07412519ba3f91fffda35dR46 a check for $CONNECTED_PATTERN

Cheers :)

[stgraber]

Travis looks happy. This should address everything @jdstrand mentioned.

[jdstrand]

A TestConnectedPlug test where you search for a rule or string from each of the apparmor and seccomp policies would be a nice touch (see mpris_test.go for an example) but because you have a spread test, that is not strictly required (and I won't block on the lack of it).

[jdstrand]

Thank you for making these changes.

+1

[mvo5]

Nice and clever!

[mvo5]

Looks good, thank you!