Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

interfaces: add cifs-mount interface #5340

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 74 additions & 0 deletions interfaces/builtin/cifs_mount_control.go
@@ -0,0 +1,74 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2016-2018 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package builtin

const cifsMountControlSummary = `allows to mount and unmount CIFS shares`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`allows mounting and unmounting CIFS filesystems`

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed


const cifsMountControlBaseDeclarationSlots = `
cifs-mount-control:
allow-installation:
slot-snap-type:
- core
deny-auto-connection: true
`

const cifsMountControlConnectedPlugSecComp = `
# Description: Allow mount and umount syscall access.

mount
umount
umount2
`

const cifsMountControlConnectedPlugAppArmor = `
# Description: Allow to mount and unmount CIFS filesystems.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Allow mounting and unmounting", maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed


# Required for mounts and unmounts
capability sys_admin,

# Allow mounts to our snap-specific writable directories
mount fstype=cifs ** -> @{HOMEDIRS}/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs ** -> @{HOMEDIRS}/*/snap/@{SNAP_NAME}/common/{,**},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to https://launchpad.net/bugs/1612393, @{HOMEDIRS} can't be used here. That said, I'd very much prefer if we didn't allow cifs mounts into the user's home. The operation would require root privileges and it is a tricky proposition for root to mount into the user's home.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, if there are important use cases for this functionality, please list them here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the user home dir specific rules

mount fstype=cifs ** -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs ** -> /var/snap/@{SNAP_NAME}/common/{,**},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using '**' means that the snap could try to mount anything from the host into SNAP_DATA or SNAP_COMMON, which would bypass intended access restrictions. That said, the fstype should cause things to fail, but a crafted mount command shipped in the snap might be used to tickle kernel bugs. What do denials look like when you try to mount cifs filesystems and you don't have these rules?

Copy link
Contributor Author

@datenhahn datenhahn Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= AppArmor =
Time: Jul 15 05:52:53
Log: apparmor="DENIED" operation="mount" info="failed mntpnt match" error=-13 profile="snap.smb-copier.smb-copier" name="/var/snap/smb-copier/common/shares/" pid=14381 comm="mount.cifs" fstype="cifs" srcname="//192.168.123.1/sambashare"

smb-copier is a simple daemon stripped down to only trigger mount umount with which I am testing the interface:

https://github.com/datenhahn/devtools-cifs-mount-control

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdstrand what would you suggest otherwise? I would like to allow all kind of cifs options and arbitrary cifs pathes. Would it help to let the mount pathes start with // ? How would I allow all mount options and pathes starting with // ?

Copy link

@jdstrand jdstrand Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would look better if we could do:

mount fstype=cifs //*/** -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs //*/** -> /var/snap/@{SNAP_NAME}/common/{,**},

Can you try it locally and see if it works? I suspect however that it will not and that apparmor_parser will minimize that to '/**' internally and it won't match. If it doesn't work, simply use:

// srcname is of form '//<ip>/dir1/dirN' and is therefore not predictable so we'll
// rely on the fstype.
mount fstype=cifs -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs -> /var/snap/@{SNAP_NAME}/common/{,**},


umount fstype=cifs @{HOMEDIRS}/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
umount fstype=cifs @{HOMEDIRS}/*/snap/@{SNAP_NAME}/common/{,**},
Copy link

@jdstrand jdstrand Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be removed if the above are.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the user home dir specific rules

umount fstype=cifs /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
umount fstype=cifs /var/snap/@{SNAP_NAME}/common/{,**},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, due to https://bugs.launchpad.net/apparmor/+bug/1613403, specifying fstype=cifs doesn't actually limit the umount. Please adjust to be:

# 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=cifs /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
umount fstype=cifs /var/snap/@{SNAP_NAME}/common/{,**},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as requested

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a more serious problem, @jdstrand? If the snap can unmount, it can override mounts and get access to the underlying filesystem. Not sure if there's already a security issue created by this, but it's an axis we don't generally account for when reviewing logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, nevermind... I see the unmounts are restricted to $SNAP_DATA, so it won't really be a problem.


# Don't log violations for this file, see discussion here:
# - https://github.com/snapcore/snapd/pull/5340#issuecomment-398071797
# - https://forum.snapcraft.io/t/namespace-awareness-of-run-mount-utab-and-libmount/5987
deny /run/mount/utab rw,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The described need for "mount-observe" seems a bit curious. Why is that necessary? Is it because the tools require it, or is there another limitation that prevent the mount syscall from working at all?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of libmount requires the effective permissions granted by mount-observe

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't care for the explicit denial here since deny rules take precedent over allow rules. Ie, this means if another interface came along that allowed the access, this denial would still deny it. We either need to not worry about the logged denial or extend the comment for this. I prefer the former as a reminder to do something about https://forum.snapcraft.io/t/namespace-awareness-of-run-mount-utab-and-libmount/5987/3.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the explicit denial and updated the comment

`

func init() {
registerIface(&commonInterface{
name: "cifs-mount-control",
summary: cifsMountControlSummary,
implicitOnCore: true,
implicitOnClassic: true,
baseDeclarationSlots: cifsMountControlBaseDeclarationSlots,
connectedPlugAppArmor: cifsMountControlConnectedPlugAppArmor,
connectedPlugSecComp: cifsMountControlConnectedPlugSecComp,
reservedForOS: true,
})
}
109 changes: 109 additions & 0 deletions interfaces/builtin/cifs_mount_control_test.go
@@ -0,0 +1,109 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2016-2017 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package builtin_test

import (
. "gopkg.in/check.v1"

"github.com/snapcore/snapd/interfaces"
"github.com/snapcore/snapd/interfaces/apparmor"
"github.com/snapcore/snapd/interfaces/builtin"
"github.com/snapcore/snapd/interfaces/seccomp"
"github.com/snapcore/snapd/snap"
"github.com/snapcore/snapd/testutil"
)

type CifsMountControlInterfaceSuite struct {
iface interfaces.Interface
slotInfo *snap.SlotInfo
slot *interfaces.ConnectedSlot
plugInfo *snap.PlugInfo
plug *interfaces.ConnectedPlug
}

const cifsMountControlConsumerYaml = `name: consumer
version: 0
apps:
app:
plugs: [cifs-mount-control]
`

const cifsMountControlCoreYaml = `name: core
version: 0
type: os
slots:
cifs-mount-control:
`

var _ = Suite(&CifsMountControlInterfaceSuite{
iface: builtin.MustInterface("cifs-mount-control"),
})

func (s *CifsMountControlInterfaceSuite) SetUpTest(c *C) {
s.plug, s.plugInfo = MockConnectedPlug(c, cifsMountControlConsumerYaml, nil, "cifs-mount-control")
s.slot, s.slotInfo = MockConnectedSlot(c, cifsMountControlCoreYaml, nil, "cifs-mount-control")
}

func (s *CifsMountControlInterfaceSuite) TestName(c *C) {
c.Assert(s.iface.Name(), Equals, "cifs-mount-control")
}

func (s *CifsMountControlInterfaceSuite) TestSanitizeSlot(c *C) {
c.Assert(interfaces.BeforePrepareSlot(s.iface, s.slotInfo), IsNil)
slot := &snap.SlotInfo{
Snap: &snap.Info{SuggestedName: "some-snap"},
Name: "cifs-mount-control",
Interface: "cifs-mount-control",
}
c.Assert(interfaces.BeforePrepareSlot(s.iface, slot), ErrorMatches,
"cifs-mount-control slots are reserved for the core snap")
}

func (s *CifsMountControlInterfaceSuite) TestSanitizePlug(c *C) {
c.Assert(interfaces.BeforePreparePlug(s.iface, s.plugInfo), IsNil)
}

func (s *CifsMountControlInterfaceSuite) TestAppArmorSpec(c *C) {
spec := &apparmor.Specification{}
c.Assert(spec.AddConnectedPlug(s.iface, s.plug, s.slot), IsNil)
c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `/run/mount/utab`)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be adjusted since you removed it (please adjust to be #deny /run/mount/utab,\\n since I recommended a commented out rule.

}

func (s *CifsMountControlInterfaceSuite) TestSecCompSpec(c *C) {
spec := &seccomp.Specification{}
c.Assert(spec.AddConnectedPlug(s.iface, s.plug, s.slot), IsNil)
c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "mount\n")
c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "umount\n")
c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "umount2\n")
}

func (s *CifsMountControlInterfaceSuite) TestStaticInfo(c *C) {
si := interfaces.StaticInfoOf(s.iface)
c.Assert(si.ImplicitOnCore, Equals, true)
c.Assert(si.ImplicitOnClassic, Equals, true)
c.Assert(si.Summary, Equals, `allows to mount and unmount CIFS shares`)
c.Assert(si.BaseDeclarationSlots, testutil.Contains, "cifs-mount-control")
}

func (s *CifsMountControlInterfaceSuite) TestInterfaces(c *C) {
c.Check(builtin.Interfaces(), testutil.DeepContains, s.iface)
}
3 changes: 3 additions & 0 deletions tests/lib/snaps/test-snapd-policy-app-consumer/meta/snap.yaml
Expand Up @@ -41,6 +41,9 @@ apps:
camera:
command: bin/run
plugs: [ camera ]
cifs-mount-control:
command: bin/run
plugs: [ cifs-mount-control ]
classic-support:
command: bin/run
plugs: [ classic-support ]
Expand Down