interfaces/builtin: use udev tagging more broadly #3617

Merged
merged 45 commits into from Aug 29, 2017
Commits
Jump to file or symbol
Failed to load files and symbols.
+390 −98
Split
@@ -46,6 +46,16 @@ const alsaConnectedPlugAppArmor = `
@{PROC}/asound/** rw,
`
+const alsaConnectedPlugUDev = `
+KERNEL=="controlC[0-9]*", TAG+="###CONNECTED_SECURITY_TAGS###"
+KERNEL=="hwC[0-9]*D[0-9]*", TAG+="###CONNECTED_SECURITY_TAGS###"
+KERNEL=="pcmC[0-9]*D[0-9]*[cp]", TAG+="###CONNECTED_SECURITY_TAGS###"
+KERNEL=="midiC[0-9]*D[0-9]*", TAG+="###CONNECTED_SECURITY_TAGS###"
+KERNEL=="timer", TAG+="###CONNECTED_SECURITY_TAGS###"
+KERNEL=="seq", TAG+="###CONNECTED_SECURITY_TAGS###"
+SUBSYSTEM=="sound", KERNEL=="card[0-9]*", TAG+="###CONNECTED_SECURITY_TAGS###"
+`
+
func init() {
registerIface(&commonInterface{
name: "alsa",
@@ -54,6 +64,7 @@ func init() {
implicitOnClassic: true,
baseDeclarationSlots: alsaBaseDeclarationSlots,
connectedPlugAppArmor: alsaConnectedPlugAppArmor,
+ connectedPlugUDev: alsaConnectedPlugUDev,
reservedForOS: true,
})
}
@@ -25,6 +25,7 @@ import (
"github.com/snapcore/snapd/interfaces"
"github.com/snapcore/snapd/interfaces/apparmor"
"github.com/snapcore/snapd/interfaces/builtin"
+ "github.com/snapcore/snapd/interfaces/udev"
"github.com/snapcore/snapd/snap"
"github.com/snapcore/snapd/testutil"
)
@@ -82,6 +83,13 @@ func (s *AlsaInterfaceSuite) TestAppArmorSpec(c *C) {
c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "/dev/snd/* rw,")
}
+func (s *AlsaInterfaceSuite) TestUDevpec(c *C) {
+ spec := &udev.Specification{}
+ c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil), IsNil)
+ c.Assert(spec.Snippets(), HasLen, 1)
+ c.Assert(spec.Snippets()[0], testutil.Contains, `KERNEL=="pcmC[0-9]*D[0-9]*[cp]", TAG+="snap_consumer_app"`)
+}
+
func (s *AlsaInterfaceSuite) TestStaticInfo(c *C) {
si := interfaces.StaticInfoOf(s.iface)
c.Assert(si.ImplicitOnCore, Equals, true)
@@ -1,7 +1,7 @@
// -*- Mode: Go; indent-tabs-mode: t -*-
/*
- * Copyright (C) 2016 Canonical Ltd
+ * 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
@@ -56,6 +56,8 @@ const bluetoothControlConnectedPlugSecComp = `
bind
`
+const bluetoothControlConnectedPlugUDev = `SUBSYSTEM=="bluetooth", TAG+="###CONNECTED_SECURITY_TAGS###"`
+
func init() {
registerIface(&commonInterface{
name: "bluetooth-control",
@@ -65,6 +67,7 @@ func init() {
baseDeclarationSlots: bluetoothControlBaseDeclarationSlots,
connectedPlugAppArmor: bluetoothControlConnectedPlugAppArmor,
connectedPlugSecComp: bluetoothControlConnectedPlugSecComp,
+ connectedPlugUDev: bluetoothControlConnectedPlugUDev,
reservedForOS: true,
})
}
@@ -1,7 +1,7 @@
// -*- Mode: Go; indent-tabs-mode: t -*-
/*
- * Copyright (C) 2016 Canonical Ltd
+ * Copyright (C) 2016-2017 Canonical Ltd
@jdstrand

jdstrand Aug 17, 2017

Contributor

You changed the copyright in bluetooth_control_test.go, but forgot to update it and bluetooth_control.go for /dev/vhci, which is included in the apparmor policy. Can you add that?

@adglkh

adglkh Aug 18, 2017

Contributor

/dev/vhci is just a node that vhci driver created. I am not able to find it in udev database even if it exists. http://paste.ubuntu.com/25336192/
If we add udev rules, the devices we want to tag are the virtual devices.
`SUBSYSTEM=="bluetooth", KERNEL=="hci[0-9]*", TAG+="%s"

Yes, "%s" will be changed to ###CONNECTED_SECURITY_TAGS###

@jdstrand

jdstrand Aug 18, 2017

Contributor

Oh that is interesting, but I really like your idea of SUBSYSTEM bluetooth. How about just treat this like raw-usb and do:

SUBSYSTEM=="bluetooth", TAG+="%s

The 'hci' devices are virtual devices and not yet mediated by AppArmor or devices that need to be added to the device cgroup. However, tagging all bluetooth devices helps future-proof a bit in case /dev/vhci ends up showing up in udev or other bluetooth devices show up in /dev.

@adglkh

adglkh Aug 18, 2017

Contributor

Yeah, that's exactly what we're looking for.
Thanks.

*
* 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
@@ -26,6 +26,7 @@ import (
"github.com/snapcore/snapd/interfaces/apparmor"
"github.com/snapcore/snapd/interfaces/builtin"
"github.com/snapcore/snapd/interfaces/seccomp"
+ "github.com/snapcore/snapd/interfaces/udev"
"github.com/snapcore/snapd/snap"
"github.com/snapcore/snapd/snap/snaptest"
"github.com/snapcore/snapd/testutil"
@@ -86,20 +87,25 @@ func (s *BluetoothControlInterfaceSuite) TestSanitizePlug(c *C) {
c.Assert(s.plug.Sanitize(s.iface), IsNil)
}
-func (s *BluetoothControlInterfaceSuite) TestUsedSecuritySystems(c *C) {
- // connected plugs have a non-nil security snippet for apparmor
- apparmorSpec := &apparmor.Specification{}
- err := apparmorSpec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil)
- c.Assert(err, IsNil)
- c.Assert(apparmorSpec.SecurityTags(), DeepEquals, []string{"snap.other.app2"})
- c.Assert(apparmorSpec.SnippetForTag("snap.other.app2"), testutil.Contains, "capability net_admin")
+func (s *BluetoothControlInterfaceSuite) TestAppArmorSpec(c *C) {
+ spec := &apparmor.Specification{}
+ c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil), IsNil)
+ c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.other.app2"})
+ c.Assert(spec.SnippetForTag("snap.other.app2"), testutil.Contains, "capability net_admin")
+}
+
+func (s *BluetoothControlInterfaceSuite) TestSecCompSpec(c *C) {
+ spec := &seccomp.Specification{}
+ c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil), IsNil)
+ c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.other.app2"})
+ c.Assert(spec.SnippetForTag("snap.other.app2"), testutil.Contains, "\nbind\n")
+}
- // connected plugs have a non-nil security snippet for seccomp
- seccompSpec := &seccomp.Specification{}
- err = seccompSpec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil)
- c.Assert(err, IsNil)
- c.Assert(seccompSpec.SecurityTags(), DeepEquals, []string{"snap.other.app2"})
- c.Check(seccompSpec.SnippetForTag("snap.other.app2"), testutil.Contains, "\nbind\n")
+func (s *BluetoothControlInterfaceSuite) TestUDevSpec(c *C) {
+ spec := &udev.Specification{}
+ c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil), IsNil)
+ c.Assert(spec.Snippets(), HasLen, 1)
+ c.Assert(spec.Snippets()[0], testutil.Contains, `SUBSYSTEM=="bluetooth", TAG+="snap_other_app2"`)
}
func (s *BluetoothControlInterfaceSuite) TestInterfaces(c *C) {
@@ -26,6 +26,7 @@ import (
"github.com/snapcore/snapd/interfaces/apparmor"
"github.com/snapcore/snapd/interfaces/dbus"
"github.com/snapcore/snapd/interfaces/seccomp"
+ "github.com/snapcore/snapd/interfaces/udev"
)
const bluezSummary = `allows operating as the bluez service`
@@ -192,6 +193,8 @@ const bluezPermanentSlotDBus = `
</policy>
`
+const bluezConnectedPlugUDev = `KERNEL=="rfkill", TAG+="###CONNECTED_SECURITY_TAGS###"`
+
type bluezInterface struct{}
func (iface *bluezInterface) Name() string {
@@ -226,6 +229,16 @@ func (iface *bluezInterface) AppArmorConnectedSlot(spec *apparmor.Specification,
return nil
}
+func (iface *bluezInterface) UDevConnectedPlug(spec *udev.Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error {
+ old := "###CONNECTED_SECURITY_TAGS###"
+ for appName := range plug.Apps {
+ tag := udevSnapSecurityName(plug.Snap.Name(), appName)
+ snippet := strings.Replace(bluezConnectedPlugUDev, old, tag, -1)
+ spec.AddSnippet(snippet)
+ }
+ return nil
+}
+
func (iface *bluezInterface) AppArmorPermanentSlot(spec *apparmor.Specification, slot *interfaces.Slot) error {
spec.AddSnippet(bluezPermanentSlotAppArmor)
return nil
@@ -27,6 +27,7 @@ import (
"github.com/snapcore/snapd/interfaces/builtin"
"github.com/snapcore/snapd/interfaces/dbus"
"github.com/snapcore/snapd/interfaces/seccomp"
+ "github.com/snapcore/snapd/interfaces/udev"
"github.com/snapcore/snapd/testutil"
)
@@ -152,13 +153,20 @@ func (s *BluezInterfaceSuite) TestDBusSpec(c *C) {
c.Assert(spec.SnippetForTag("snap.producer.app"), testutil.Contains, `<allow own="org.bluez"/>`)
}
-func (s *BluezInterfaceSuite) TestSecCompSec(c *C) {
+func (s *BluezInterfaceSuite) TestSecCompSpec(c *C) {
spec := &seccomp.Specification{}
c.Assert(spec.AddPermanentSlot(s.iface, s.slot), IsNil)
c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.producer.app"})
c.Assert(spec.SnippetForTag("snap.producer.app"), testutil.Contains, "listen\n")
}
+func (s *BluezInterfaceSuite) TestUDevSpec(c *C) {
+ spec := &udev.Specification{}
+ c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil), IsNil)
+ c.Assert(spec.Snippets(), HasLen, 1)
+ c.Assert(spec.Snippets()[0], testutil.Contains, `KERNEL=="rfkill", TAG+="snap_consumer_app"`)
+}
+
func (s *BluezInterfaceSuite) TestStaticInfo(c *C) {
si := interfaces.StaticInfoOf(s.iface)
c.Assert(si.ImplicitOnCore, Equals, false)
@@ -38,12 +38,20 @@ const broadcomAsicControlConnectedPlugAppArmor = `
/dev/linux-user-bde rw,
/dev/linux-kernel-bde rw,
/dev/linux-bcm-knet rw,
+
+# These are broader than they needs to be, but until we query udev
+# for specific devices, use a broader glob
@niemeyer

niemeyer Aug 23, 2017

Contributor

Indeed, that looks way too broad. In fact, a bit too broad to be right. Not sure it makes sense to turn the "broadcoam-asic" interface in a sort of "read about every single PCI device in the system".

@jdstrand Note that we won't be able to change that in the future without the potential of breaking clients unknowingly.

What's the plan here?

@adglkh

adglkh Aug 24, 2017

Contributor

I think it's related to hardware assignment. We have a bug on launchpad tracking on this. Not sure where are we with that.
@jdstrand Could you share the updates and plan here?
Thanks

@jdstrand

jdstrand Aug 24, 2017

Contributor

@niemeyer, @adglkh - they are necessarily broad because in PR review it was found there was nothing in the /sys/devices path alone that made it 'broadcom-asic'. I believe it is technically possible for snapd to interrogate the system via udev to get the precise /sys/devices and /run/udev/data paths for the device (see #1226 (comment) for back of the napkin idea), but this work is neither prioritized nor assigned. As a result, this interface along with existing opengl and camera interfaces use rules that are broader than they should be (but we comment in the policy in camera and opengl about the leak). While the accesses in question do constitute an information leak, they are not terribly significant (which is why the work is not prioritized/assigned) and in the 'future work' bucket.

I think the plan should be:

  • continue with the access as written since without it, things break
  • add a comment that this is an information leak (see opengl.go)
  • at some future time, implement the udev querying in snapd and have new interfaces in series 16 use this
  • in some future iteration of snappy which is generating different policy (eg, something not 'series 16'), migrate existing interfaces that use the broad globs to using only the outputs of udev querying
+/sys/devices/pci[0-9]*/**/config r,
+/sys/devices/pci[0-9]*/**/{,subsystem_}device r,
+/sys/devices/pci[0-9]*/**/{,subsystem_}vendor r,
+
+/sys/bus/pci/devices/ r,
+/run/udev/data/+pci:[0-9]* r,
`
const broadcomAsicControlConnectedPlugUDev = `
-KERNEL=="linux-user-bde", TAG+="###SLOT_SECURITY_TAGS###"
-KERNEL=="linux-kernel-bde", TAG+="###SLOT_SECURITY_TAGS###"
-KERNEL=="linux-bcm-knet", TAG+="###SLOT_SECURITY_TAGS###"
+SUBSYSTEM=="pci", DRIVER=="linux-kernel-bde", TAG+="###CONNECTED_SECURITY_TAGS###"
+SUBSYSTEM=="net", KERNEL=="bcm[0-9]*", TAG+="###CONNECTED_SECURITY_TAGS###"
`
// The upstream linux kernel doesn't come with support for the
@@ -89,8 +89,8 @@ func (s *BroadcomAsicControlSuite) TestAppArmorSpec(c *C) {
func (s *BroadcomAsicControlSuite) TestUDevSpec(c *C) {
spec := &udev.Specification{}
c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil), IsNil)
- c.Assert(len(spec.Snippets()), Equals, 1)
- c.Assert(spec.Snippets()[0], testutil.Contains, `KERNEL=="linux-user-bde", TAG+="snap_consumer_app"`)
+ c.Assert(spec.Snippets(), HasLen, 1)
+ c.Assert(spec.Snippets()[0], testutil.Contains, `SUBSYSTEM=="net", KERNEL=="bcm[0-9]*", TAG+="snap_consumer_app"`)
}
func (s *BroadcomAsicControlSuite) TestKModSpec(c *C) {
@@ -1,7 +1,7 @@
// -*- Mode: Go; indent-tabs-mode: t -*-
/*
- * Copyright (C) 2016 Canonical Ltd
+ * 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
@@ -42,6 +42,8 @@ const cameraConnectedPlugAppArmor = `
/sys/devices/pci**/usb*/**/video4linux/** r,
`
+const cameraConnectedPlugUDev = `KERNEL=="video[0-9]*", TAG+="###CONNECTED_SECURITY_TAGS###"`
+
func init() {
registerIface(&commonInterface{
name: "camera",
@@ -50,6 +52,7 @@ func init() {
implicitOnClassic: true,
baseDeclarationSlots: cameraBaseDeclarationSlots,
connectedPlugAppArmor: cameraConnectedPlugAppArmor,
+ connectedPlugUDev: cameraConnectedPlugUDev,
reservedForOS: true,
})
}
@@ -25,6 +25,7 @@ import (
"github.com/snapcore/snapd/interfaces"
"github.com/snapcore/snapd/interfaces/apparmor"
"github.com/snapcore/snapd/interfaces/builtin"
+ "github.com/snapcore/snapd/interfaces/udev"
"github.com/snapcore/snapd/snap"
"github.com/snapcore/snapd/testutil"
)
@@ -82,6 +83,13 @@ func (s *CameraInterfaceSuite) TestAppArmorSpec(c *C) {
c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "/dev/video[0-9]* rw")
}
+func (s *CameraInterfaceSuite) TestUDevSpec(c *C) {
+ spec := &udev.Specification{}
+ c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil), IsNil)
+ c.Assert(spec.Snippets(), HasLen, 1)
+ c.Assert(spec.Snippets()[0], testutil.Contains, `KERNEL=="video[0-9]*", TAG+="snap_consumer_app"`)
+}
+
func (s *CameraInterfaceSuite) TestStaticInfo(c *C) {
si := interfaces.StaticInfoOf(s.iface)
c.Assert(si.ImplicitOnCore, Equals, true)
@@ -147,7 +147,7 @@ func (iface *commonInterface) SecCompConnectedPlug(spec *seccomp.Specification,
}
func (iface *commonInterface) UDevConnectedPlug(spec *udev.Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error {
- old := "###SLOT_SECURITY_TAGS###"
+ old := "###CONNECTED_SECURITY_TAGS###"
if iface.connectedPlugUDev != "" {
for appName := range plug.Apps {
tag := udevSnapSecurityName(plug.Snap.Name(), appName)
@@ -49,7 +49,7 @@ slots:
// common interface can define connected plug udev rules
iface := &commonInterface{
name: "common",
- connectedPlugUDev: `KERNEL="foo", TAG+="###SLOT_SECURITY_TAGS###"`,
+ connectedPlugUDev: `KERNEL="foo", TAG+="###CONNECTED_SECURITY_TAGS###"`,
}
spec := &udev.Specification{}
c.Assert(spec.AddConnectedPlug(iface, plug, nil, slot, nil), IsNil)
@@ -1,7 +1,7 @@
// -*- Mode: Go; indent-tabs-mode: t -*-
/*
- * Copyright (C) 2017 Canonical Ltd
+ * 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
@@ -37,12 +37,7 @@ const framebufferConnectedPlugAppArmor = `
/run/udev/data/c29:[0-9]* r,
`
-// This will fix access denied of opengl interface when it's used with
-// framebuffer interface in the same snap.
-// https://bugs.launchpad.net/snapd/+bug/1675738
-// TODO: we are not doing this due to the bug and we'll be reintroducing
-// the udev tagging soon.
-// const framebufferUDevConnectedPlug = `KERNEL=="fb[0-9]*", TAG+="###SLOT_SECURITY_TAGS###"`
+const framebufferConnectedPlugUDev = `KERNEL=="fb[0-9]*", TAG+="###CONNECTED_SECURITY_TAGS###"`
func init() {
registerIface(&commonInterface{
@@ -52,6 +47,7 @@ func init() {
implicitOnClassic: true,
baseDeclarationSlots: framebufferBaseDeclarationSlots,
connectedPlugAppArmor: framebufferConnectedPlugAppArmor,
+ connectedPlugUDev: framebufferConnectedPlugUDev,
reservedForOS: true,
})
}
@@ -82,21 +82,14 @@ func (s *FramebufferInterfaceSuite) TestAppArmorSpec(c *C) {
spec := &apparmor.Specification{}
c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil), IsNil)
c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
- c.Assert(spec.SnippetForTag("snap.consumer.app"), Equals, `
-# Description: Allow reading and writing to the universal framebuffer (/dev/fb*) which
-# gives privileged access to the console framebuffer.
-
-/dev/fb[0-9]* rw,
-/run/udev/data/c29:[0-9]* r,
-`)
+ c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `/dev/fb[0-9]* rw,`)
}
func (s *FramebufferInterfaceSuite) TestUDevSpec(c *C) {
spec := &udev.Specification{}
c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil), IsNil)
- // UDev tagging is disabled and will be enabled with a separate patch.
- // Remove this comment when enabling udev tagging.
- c.Assert(spec.Snippets(), DeepEquals, []string(nil))
+ c.Assert(spec.Snippets(), HasLen, 1)
+ c.Assert(spec.Snippets()[0], Equals, `KERNEL=="fb[0-9]*", TAG+="snap_consumer_app"`)
}
func (s *FramebufferInterfaceSuite) TestStaticInfo(c *C) {
@@ -1,7 +1,7 @@
// -*- Mode: Go; indent-tabs-mode: t -*-
/*
- * Copyright (C) 2016 Canonical Ltd
+ * 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
@@ -83,8 +83,9 @@ deny /etc/fuse.conf r,
#/{,usr/}bin/fusermount ixr,
`
+const fuseSupportConnectedPlugUDev = `KERNEL=="fuse", TAG+="###CONNECTED_SECURITY_TAGS###"`
+
func init() {
- // Ubuntu 14.04 does not support the fuse-support interface.
registerIface(&commonInterface{
name: "fuse-support",
summary: fuseSupportSummary,
@@ -94,5 +95,6 @@ func init() {
reservedForOS: true,
connectedPlugAppArmor: fuseSupportConnectedPlugAppArmor,
connectedPlugSecComp: fuseSupportConnectedPlugSecComp,
+ connectedPlugUDev: fuseSupportConnectedPlugUDev,
})
}
@@ -26,6 +26,7 @@ import (
"github.com/snapcore/snapd/interfaces/apparmor"
"github.com/snapcore/snapd/interfaces/builtin"
"github.com/snapcore/snapd/interfaces/seccomp"
+ "github.com/snapcore/snapd/interfaces/udev"
"github.com/snapcore/snapd/snap"
"github.com/snapcore/snapd/testutil"
)
@@ -90,6 +91,13 @@ func (s *FuseSupportInterfaceSuite) TestSecCompSpec(c *C) {
c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "mount\n")
}
+func (s *FuseSupportInterfaceSuite) TestUDevSpec(c *C) {
+ spec := &udev.Specification{}
+ c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.slot, nil), IsNil)
+ c.Assert(spec.Snippets(), HasLen, 1)
+ c.Assert(spec.Snippets()[0], testutil.Contains, `KERNEL=="fuse", TAG+="snap_consumer_app"`)
+}
+
func (s *FuseSupportInterfaceSuite) TestStaticInfo(c *C) {
si := interfaces.StaticInfoOf(s.iface)
c.Assert(si.ImplicitOnCore, Equals, true)
Oops, something went wrong.