i/apparmor: add snippets with priorities (canonical#14061)

* Add snippets with priorities

AppArmor rules that forbid access to a resource have more
priority than rules that allow access to those same resources.
This means that if an interface restricts access to an specific
resource, it won't be possible to enable access to that same
resource from another, more privileged, interface.

An example of this is reading the .desktop files of all the
installed snaps in the system: the superprivileged interface
'desktop-launch' enables access to these files, so any snap
that has a connected plug for this interface should be able
to read them. Unfortunately, the 'desktop-legacy' interface
explicitly denies access to these files, and since it is
connected automatically if a snap uses the 'desktop' or the
'unity7' interfaces, this mean that no graphical application
will be able to read the .desktop files, even if the super-
privileged interface 'desktop-launch' interface is connected.

To allow this specific case, a temporary patch (
canonical#13933) was created and
merged, but it is clearly an ugly and not-generic solution.
For this reason, this new patch was created, following the
specification https://docs.google.com/document/d/1K-1MYhp1RKSW_jzuuyX7TSVCg2rYplKZFdJbZAupP4Y/edit

This patch allows to add "prioritized snippets". Each one has
an UID and a priority. If no prioritized snippet with the same
UID has been previously added, the new prioritized snippet will
be added like any other normal snippet. But if there is already
an added snippet with the same UID, then the priority of both
the old and the new snippets are compared. If the new priority
is lower than the old one, the new snippet is ignored; if the
new priority is bigger than the old one, the new snippet fully
replaces the old one. Finally, if both priorities are the same,
the new snippet will be appended to the old snippet.

This generic mechanism allows to give an interface priority
over others if needed, like in the previous case.

* Remove slices.Contains, since seems to be not supported

* Update interfaces/apparmor/spec.go

Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]>

* Use testutils.Contains

* Replace "uid" with "key" for clarity and sanity

* Add specific type for priority keys and force registering them

* Remove unneeded return

* Use SnippetKey as type

* Don't use "slice" since MacOS seems to not support it

* Update interfaces/apparmor/spec.go

Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]>

* Update interfaces/apparmor/spec.go

Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]>

* Use String instead of GetValue

* Use SnippetKey as key instead of the inner string

* Update interfaces/connection.go

Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]>

* Several changes requested

* Create the SnippetKeys inside Spec

* Move key registration outside Spec

This creates a centralized key registry inside apparmor module,
so keys can be registered using top variables, and any
duplicated key will produce a panic when snapd is launched,
thus just panicking in any test too.

* Added extra ways of working with SnippetKeys

* Add extra check

* Replace GetSnippetKey with GetSnippetKeys

* Update the priority code use case

A previous PR was merged with a Quick&Dirty(tm) solution to the
priority problem between unity7 and desktop-legacy interfaces
against desktop-launch interface.

Now that it has been merged, that code must be updated to the
new mechanism implemented in this PR. This is exactly what this
commit does.

* Add explanation and constants for prioritized snippets

* Fix prioritized snippet key and add test in all_test

* Several changes requested by Zygmunt Vazyli

---------

Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]>

interfaces/apparmor/spec.go

@@ -27,6 +27,7 @@ import (
 	"strings"
 
 	"github.com/snapcore/snapd/interfaces"
+	"github.com/snapcore/snapd/logger"
 	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/strutil"
 )
@@ -42,6 +43,51 @@ const (
 	UnconfinedEnabled
 )
 
+type prioritizedSnippets struct {
+	priority uint
+	list     []string
+}
+
+// SnippetKey is an opaque string identifying a class of snippets.
+//
+// Some APIs require the use of snippet keys to allow adding many different snippets
+// with the same key but possibly different priority.
+type SnippetKey struct {
+	key string
+}
+
+func (pk *SnippetKey) String() string {
+	return pk.key
+}
+
+func newSnippetKey(key string) SnippetKey {
+	return SnippetKey{key: key}
+}
+
+// registeredKeys is a list of allowed keys for prioritized snippets.
+// Trying to add a prioritized snippet with an unregistered key is
+// an error. Trying to register the same key twice is also an error.
+var registeredKeys map[SnippetKey]bool = make(map[SnippetKey]bool)
+
+// RegisterSnippetKey adds a key to the list of valid keys
+func RegisterSnippetKey(key string) SnippetKey {
+	snippetKey := newSnippetKey(key)
+	if _, ok := registeredKeys[snippetKey]; ok {
+		logger.Panicf("priority key %s is already registered", key)
+	}
+	registeredKeys[snippetKey] = true
+	return snippetKey
+}
+
+// GetSnippetKey retrieves all the current valid keys
+func RegisteredSnippetKeys() []string {
+	keylist := make([]string, 0, len(registeredKeys))
+	for k := range registeredKeys {
+		keylist = append(keylist, k.key)
+	}
+	return keylist
+}
+
 // Specification assists in collecting apparmor entries associated with an interface.
 type Specification struct {
 	// appSet is the set of snap applications and hooks that the specification
@@ -56,6 +102,15 @@ type Specification struct {
 	// of the application or hook.
 	snippets map[string][]string
 
+	// prioritizedSnippets are just like snippets, but they have a priority value
+	// and a snippet key. An interface can add a snippet with a specific key and a priority.
+	// If there doesn't exist any snippet with that key, the passed snippet will be
+	// added as-is. But if it does exist, the new snippet will replace the old one if
+	// the new priority is bigger than the old one; will be appended if the new
+	// priority is the same as the old one, and will be discarded if the new priority
+	// is smaller than the old one.
+	prioritizedSnippets map[string]map[SnippetKey]prioritizedSnippets
+
 	// dedupSnippets are just like snippets but are added only once to the
 	// resulting policy in an effort to avoid certain expensive to de-duplicate
 	// rules by apparmor_parser.
@@ -149,6 +204,51 @@ func (spec *Specification) AddSnippet(snippet string) {
 	}
 }
 
+// AddPrioritizedSnippet adds a new apparmor snippet to all applications and hooks using the interface,
+// but identified with a key and a priority. If no other snippet exists with that key, the snippet is
+// added like with AddSnippet, but if there is already another snippet with that key, the priority of
+// both will be taken into account to decide whether the new snippet replaces the old one, is appended
+// to it, or is just ignored. The key must have been previously registered using RegisterSnippetKey().
+func (spec *Specification) AddPrioritizedSnippet(snippet string, key SnippetKey, priority uint) {
+	if _, ok := registeredKeys[key]; !ok {
+		logger.Panicf("priority key %s is not registered", key.String())
+	}
+	if len(spec.securityTags) == 0 {
+		return
+	}
+	if spec.prioritizedSnippets == nil {
+		spec.prioritizedSnippets = make(map[string]map[SnippetKey]prioritizedSnippets)
+	}
+
+	for _, tag := range spec.securityTags {
+		if _, exists := spec.prioritizedSnippets[tag]; !exists {
+			spec.prioritizedSnippets[tag] = make(map[SnippetKey]prioritizedSnippets)
+		}
+		snippets := spec.prioritizedSnippets[tag][key]
+		// If the entry doesn't exist, it will return a snippet with an empty
+		// snippet string and zero priority.
+		if snippets.priority == priority {
+			// if the priority is the same, just append the snippet to the snippets already there
+			snippets.list = append(snippets.list, snippet)
+		} else if snippets.priority < priority {
+			// if the priority is bigger, replace the snippets with the new one
+			snippets.list = append([]string(nil), snippet)
+			snippets.priority = priority
+		} // smaller priority, discard
+		spec.prioritizedSnippets[tag][key] = snippets
+	}
+}
+
+func (spec *Specification) composeSnippetsForTag(tag string) []string {
+	// Compose the normal and the prioritized snippets in a single string array
+	composedSnippets := append([]string(nil), spec.snippets[tag]...)
+	for key := range spec.prioritizedSnippets[tag] {
+		composedSnippets = append(composedSnippets, spec.prioritizedSnippets[tag][key].list...)
+	}
+	sort.Strings(composedSnippets)
+	return composedSnippets
+}
+
 // AddDeduplicatedSnippet adds a new apparmor snippet to all applications and hooks using the interface.
 //
 // Certain combinations of snippets may be computationally expensive for
@@ -537,6 +637,12 @@ func (spec *Specification) SecurityTags() []string {
 		tags = append(tags, t)
 		seen[t] = true
 	}
+	for t := range spec.prioritizedSnippets {
+		if !seen[t] {
+			tags = append(tags, t)
+			seen[t] = true
+		}
+	}
 	for t := range spec.dedupSnippets {
 		if !seen[t] {
 			tags = append(tags, t)
@@ -552,7 +658,7 @@ func (spec *Specification) SecurityTags() []string {
 }
 
 func (spec *Specification) snippetsForTag(tag string) []string {
-	snippets := append([]string(nil), spec.snippets[tag]...)
+	snippets := append([]string(nil), spec.composeSnippetsForTag(tag)...)
 	// First add any deduplicated snippets
 	if bag := spec.dedupSnippets[tag]; bag != nil {
 		snippets = append(snippets, bag.Items()...)

interfaces/apparmor/spec_test.go

@@ -605,3 +605,67 @@ func (s *specSuite) TestSetSuppressPycacheDeny(c *C) {
 	s.spec.SetSuppressPycacheDeny()
 	c.Assert(s.spec.SuppressPycacheDeny(), Equals, true)
 }
+
+var key1 = apparmor.RegisterSnippetKey("testkey1")
+var key2 = apparmor.RegisterSnippetKey("testkey2")
+
+func (s *specSuite) TestPrioritySnippets(c *C) {
+	restoreScope1 := apparmor.SetSpecScope(s.spec, []string{"snap.demo.scope1"})
+	defer restoreScope1()
+
+	// Test a scope with a normal snippet and prioritized ones
+	s.spec.AddSnippet("Test snippet 1")
+	s.spec.AddPrioritizedSnippet("Prioritized snippet 1", key1, 0)
+	s.spec.AddPrioritizedSnippet("Prioritized snippet 2", key1, 0)
+	s.spec.AddPrioritizedSnippet("Prioritized snippet 3", key2, 1)
+	s.spec.AddPrioritizedSnippet("Prioritized snippet 4", key2, 2)
+	s.spec.AddPrioritizedSnippet("Prioritized snippet 5", key2, 0)
+
+	// Test a scope with only prioritized snippets
+	restoreScope2 := apparmor.SetSpecScope(s.spec, []string{"snap.demo.scope2"})
+	defer restoreScope2()
+
+	s.spec.AddPrioritizedSnippet("Prioritized snippet 6", key1, 0)
+	s.spec.AddPrioritizedSnippet("Prioritized snippet 7", key1, 0)
+	s.spec.AddPrioritizedSnippet("Prioritized snippet 8", key2, 1)
+	s.spec.AddPrioritizedSnippet("Prioritized snippet 9", key2, 2)
+	s.spec.AddPrioritizedSnippet("Prioritized snippet 10", key2, 0)
+
+	snippets := s.spec.SnippetForTag("snap.demo.scope1")
+	c.Assert(snippets, testutil.Contains, "Test snippet 1")
+	c.Assert(snippets, testutil.Contains, "Prioritized snippet 1")
+	c.Assert(snippets, testutil.Contains, "Prioritized snippet 2")
+	c.Assert(snippets, Not(testutil.Contains), "Prioritized snippet 3")
+	c.Assert(snippets, testutil.Contains, "Prioritized snippet 4")
+	c.Assert(snippets, Not(testutil.Contains), "Prioritized snippet 5")
+
+	snippets = s.spec.SnippetForTag("snap.demo.scope2")
+	c.Assert(snippets, testutil.Contains, "Prioritized snippet 6")
+	c.Assert(snippets, testutil.Contains, "Prioritized snippet 7")
+	// Overridden by higher-priority snippet 9 with the same key (key2)
+	c.Assert(snippets, Not(testutil.Contains), "Prioritized snippet 8")
+	c.Assert(snippets, testutil.Contains, "Prioritized snippet 9")
+	// Overridden by higher-priority snippet 9 with the same key (key2)
+	c.Assert(snippets, Not(testutil.Contains), "Prioritized snippet 10")
+
+	tags := s.spec.SecurityTags()
+	c.Assert(tags, testutil.Contains, "snap.demo.scope1")
+	c.Assert(tags, testutil.Contains, "snap.demo.scope2")
+}
+
+func (s *specSuite) TestPrioritySnippetsNoRegisteredKey(c *C) {
+	var key1 apparmor.SnippetKey = apparmor.SnippetKey{}
+	c.Assert(func() { s.spec.AddPrioritizedSnippet("Prioritized snippet 1", key1, 0) }, PanicMatches, "priority key  is not registered")
+}
+
+func (s *specSuite) TestRegisterSameSnippetKeyTwice(c *C) {
+	c.Assert(func() { apparmor.RegisterSnippetKey("testkey1") }, PanicMatches, "priority key testkey1 is already registered")
+}
+
+func (s *specSuite) TestMoreSnippets(c *C) {
+	keylist := apparmor.RegisteredSnippetKeys()
+	c.Assert(keylist, testutil.Contains, "testkey1")
+	c.Assert(keylist, testutil.Contains, "testkey2")
+	c.Assert(len(keylist), Equals, 2)
+
+}

interfaces/builtin/all_test.go

@@ -39,6 +39,7 @@ import (
 	"github.com/snapcore/snapd/interfaces/udev"
 	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/snap/snaptest"
+	"github.com/snapcore/snapd/testutil"
 )
 
 type AllSuite struct{}
@@ -465,3 +466,9 @@ func (s *AllSuite) TestAppArmorUnconfinedSlots(c *C) {
 		c.Assert(interfaces.StaticInfoOf(iface).AppArmorUnconfinedSlots, Equals, appArmorUnconfinedSlots[iface.Name()])
 	}
 }
+
+func (s *AllSuite) TestPrioritizedSnippets(c *C) {
+	keys := apparmor.RegisteredSnippetKeys()
+	c.Assert(len(keys), Equals, 1)
+	c.Assert(keys, testutil.Contains, "desktop-file-access")
+}

interfaces/builtin/desktop_launch.go

@@ -19,6 +19,34 @@
 
 package builtin
 
+import (
+	"github.com/snapcore/snapd/interfaces"
+	"github.com/snapcore/snapd/interfaces/apparmor"
+	"github.com/snapcore/snapd/snap"
+)
+
+// The desktop-legacy and unity7 interfaces denies access to the contents of the .desktop files
+// located at /var/lib/snapd/desktop/applications; only the list of files is readable.
+// The desktop-launch interface, instead, gives access to those files contents. But since, in
+// AppArmor, a deny access is "stronger", if the desktop-legacy or the unity7 interfaces are
+// connected, the access to those contents will be forbidden, no matter what the desktop-launch
+// interface adds to the AppArmor configuration.
+//
+// To fix this, a prioritized block is defined with `prioritizedSnippetDesktopFileAccess`, where
+// the AppArmor code inserted by desktop-legacy and unity7 to forbid access to those files is
+// given a lower priority than the code inserted by desktop-launch to allow it. This way, if
+// only the desktop-legacy or the unity7 interfaces are connected, everything will work as
+// before (no access to the contents of the .desktop files), but if the desktop-launch interface
+// is connected too (which, let's remind, it's a very privileged interface), then access to those
+// .desktop files is granted, because the code from desktop-legacy and unity7 is removed.
+
+const desktopLegacyAndUnity7Priority = 0
+const desktopLaunchPriority = 100
+
+var prioritizedSnippetDesktopFileAccess = apparmor.RegisterSnippetKey("desktop-file-access")
+
+type desktopLaunchInterface struct{}
+
 const desktopLaunchSummary = `allows snaps to identify and launch desktop applications in (or from) other snaps`
 
 const desktopLaunchBaseDeclarationPlugs = `
@@ -55,14 +83,36 @@ dbus (send)
     peer=(label=unconfined),
 `
 
+func (iface *desktopLaunchInterface) Name() string {
+	return "desktop-launch"
+}
+
+func (iface *desktopLaunchInterface) StaticInfo() interfaces.StaticInfo {
+	return interfaces.StaticInfo{
+		Summary:              desktopLaunchSummary,
+		ImplicitOnClassic:    true,
+		BaseDeclarationPlugs: desktopLaunchBaseDeclarationPlugs,
+		BaseDeclarationSlots: desktopLaunchBaseDeclarationSlots,
+	}
+}
+
+func (iface *desktopLaunchInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
+	spec.AddSnippet(desktopLaunchConnectedPlugAppArmor)
+	// the DesktopFileRules added by other, less privileged, interfaces (like unity7
+	// or desktop-legacy) can conflict with the rules in this, more privileged,
+	// interface, so they are added there with the minimum priority, while in this
+	// one an empty string is added with a bigger privilege value, thus removing
+	// those rules when desktop-launch plug is connected.
+	spec.AddPrioritizedSnippet("", prioritizedSnippetDesktopFileAccess, desktopLaunchPriority)
+	return nil
+}
+
+func (iface *desktopLaunchInterface) AutoConnect(*snap.PlugInfo, *snap.SlotInfo) bool {
+	// allow what declarations allowed
+	return true
+}
+
 // Only implicitOnClassic since userd isn't yet usable on core
 func init() {
-	registerIface(&commonInterface{
-		name:                  "desktop-launch",
-		summary:               desktopLaunchSummary,
-		implicitOnClassic:     true,
-		baseDeclarationPlugs:  desktopLaunchBaseDeclarationPlugs,
-		baseDeclarationSlots:  desktopLaunchBaseDeclarationSlots,
-		connectedPlugAppArmor: desktopLaunchConnectedPlugAppArmor,
-	})
+	registerIface(&desktopLaunchInterface{})
 }

interfaces/builtin/desktop_legacy.go

@@ -251,7 +251,6 @@ dbus (send)
     interface=org.gtk.vfs.MountTracker
     member=LookupMount,
 
-###SNAP_DESKTOP_FILE_RULES###
 # Snaps are unable to use the data in mimeinfo.cache (since they can't execute
 # the returned desktop file themselves). unity messaging menu doesn't require
 # mimeinfo.cache and xdg-mime will fallback to reading the desktop files
@@ -385,8 +384,14 @@ type desktopLegacyInterface struct {
 }
 
 func (iface *desktopLegacyInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
-	snippet := strings.Join(getDesktopFileRules(plug.Snap().DesktopPrefix(), spec), "\n")
-	spec.AddSnippet(strings.Replace(desktopLegacyConnectedPlugAppArmor, "###SNAP_DESKTOP_FILE_RULES###", snippet+"\n", -1))
+	spec.AddSnippet(desktopLegacyConnectedPlugAppArmor)
+
+	// the DesktopFileRules can conflict with the rules in other, more privileged,
+	// interfaces (like desktop-launch), so they are added here with the minimum
+	// priority, while those other, more privileged, interfaces will add an empty
+	// string with a bigger privilege value.
+	desktopSnippet := strings.Join(getDesktopFileRules(plug.Snap().DesktopPrefix()), "\n")
+	spec.AddPrioritizedSnippet(desktopSnippet, prioritizedSnippetDesktopFileAccess, desktopLegacyAndUnity7Priority)
 
 	return nil
 }

interfaces/builtin/unity7.go

@@ -689,12 +689,14 @@ func (iface *unity7Interface) AppArmorConnectedPlug(spec *apparmor.Specification
 	new = strings.Replace(new, "+", "_", -1)
 	old := "###UNITY_SNAP_NAME###"
 	snippet := strings.Replace(unity7ConnectedPlugAppArmor, old, new, -1)
-
-	old = "###SNAP_DESKTOP_FILE_RULES###"
-	new = strings.Join(getDesktopFileRules(plug.Snap().DesktopPrefix(), spec), "\n")
-	snippet = strings.Replace(snippet, old, new+"\n", -1)
-
 	spec.AddSnippet(snippet)
+
+	// the DesktopFileRules can conflict with the rules in other, more privileged,
+	// interfaces (like desktop-launch), so they are added here with the minimum
+	// priority, while those other, more privileged, interfaces will add an empty
+	// string with a bigger privilege value.
+	desktopSnippet := strings.Join(getDesktopFileRules(plug.Snap().DesktopPrefix()), "\n")
+	spec.AddPrioritizedSnippet(desktopSnippet, prioritizedSnippetDesktopFileAccess, desktopLegacyAndUnity7Priority)
 	return nil
 }
 

interfaces/builtin/utils.go

@@ -27,7 +27,6 @@ import (
 
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/interfaces"
-	"github.com/snapcore/snapd/interfaces/apparmor"
 	"github.com/snapcore/snapd/snap"
 )
 
@@ -114,23 +113,7 @@ func aareExclusivePatterns(orig string) []string {
 // dirs.SnapDesktopFilesDir, but explicitly denies access to all other snaps'
 // desktop files since xdg libraries may try to read all the desktop files
 // in the dir, causing excessive noise. (LP: #1868051)
-func getDesktopFileRules(snapInstanceName string, spec *apparmor.Specification) []string {
-	// desktop-launch allows to read all .desktop files; but "deny" rules overrule any "allow"
-	// rule, so we must not add these rules if this snap uses the desktop-launch interface.
-	// Also, for security reasons, all these rules are removed if the desktop-launch interface
-	// is listed, thus only if it is really connected will the snap have any kind of access to
-	// these folders/files.
-	//
-	// FIXME: this is really an ugly trick, so a better and more general mechanism is required
-	// in the future to define priorities between AppArmor rules blocks.
-	if spec != nil {
-		for _, plug := range spec.SnapAppSet().Info().Plugs {
-			if plug.Interface == "desktop-launch" {
-				return nil
-			}
-		}
-	}
-
+func getDesktopFileRules(snapInstanceName string) []string {
 	baseDir := dirs.SnapDesktopFilesDir
 
 	rules := []string{