interfaces: give priority to desktop-launch over desktop-legacy (#13933)

* interfaces: give priority to desktop-launch over desktop-legacy

The interface 'desktop-legacy' (and 'unity7') specifically
denies read access to the .desktop files, which means that any
extension that requires it (like gnome or kde) won't be able
to read them.

Unfortunately, there are some specific cases where reading the
.desktop files is mandatory, like when implementing the new
Refresh Awareness specification. This specification requires
to show the "visible name" of a snap, and its icon, and in
order to have access to that, it is mandatory to be able to
read the .desktop files.

The 'desktop-launch' interface does include read access to the
.desktop files. Although it is a very privileged interface, it
is not a problem because the snaps that implement the Refresh
Awareness specification are too, so using it to gain access to
the .desktop files should be enough. Unfortunately, mixing it
with 'desktop-legacy' interface (which happens when the snap
implementing the Refresh Awareness specification also uses the
gnome or the kde extension) results in not having access to
the files, because the 'deny' rules set by the later have
priority over any 'allow' rule set by the former.

This PR adds a check when adding the specific .desktop rules
in the 'desktop-legacy' interface: if the snap has a plug for
the 'desktop-launch' interface, it won't apply the .desktop
rules. This is not a problem, because without them, no access
is granted by default (the rules added by 'desktop-legacy'
allow to list the .desktop files, but not read them).

* Use the interface name instead of the plug name

* Fix tests

* Add extra check with both plugs connected

* Change comparison in test

* Changes requested

* Add FIXME comment for the new code

interfaces/builtin/desktop_launch_test.go

@@ -101,3 +101,26 @@ func (s *desktopLaunchSuite) TestStaticInfo(c *C) {
 	c.Assert(si.BaseDeclarationPlugs, testutil.Contains, "deny-auto-connection: true")
 	c.Assert(si.BaseDeclarationPlugs, testutil.Contains, "allow-installation: false")
 }
+
+func (s *desktopLaunchSuite) TestDesktopLaunchAndDesktopLegacy(c *C) {
+	const desktopLaunchConsumerYamlWithLaunchAndLegacy = `
+name: other
+version: 0
+apps:
+ app:
+    command: foo
+    plugs: [desktop-launch, desktop-legacy]
+`
+
+	plug, _ := MockConnectedPlug(c, desktopLaunchConsumerYamlWithLaunchAndLegacy, nil, "desktop-launch")
+	appSet, err := interfaces.NewSnapAppSet(s.plug.Snap(), nil)
+	c.Assert(err, IsNil)
+	apparmorSpec := apparmor.NewSpecification(appSet)
+	err = apparmorSpec.AddConnectedPlug(s.iface, s.plug, s.slot)
+	c.Assert(err, IsNil)
+	err = apparmorSpec.AddConnectedPlug(builtin.MustInterface("desktop-legacy"), plug, s.slot)
+	c.Assert(err, IsNil)
+	c.Assert(apparmorSpec.SnippetForTag("snap.other.app"), Not(testutil.Contains), "# Explicitly deny access to other snap's desktop files")
+	c.Assert(apparmorSpec.SnippetForTag("snap.other.app"), testutil.Contains, "Description: Can access common desktop legacy methods.")
+	c.Assert(apparmorSpec.SnippetForTag("snap.other.app"), testutil.Contains, "Description: Can identify and launch other snaps.")
+}

interfaces/builtin/desktop_legacy.go

@@ -385,7 +385,7 @@ type desktopLegacyInterface struct {
 }
 
 func (iface *desktopLegacyInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
-	snippet := strings.Join(getDesktopFileRules(plug.Snap().DesktopPrefix()), "\n")
+	snippet := strings.Join(getDesktopFileRules(plug.Snap().DesktopPrefix(), spec), "\n")
 	spec.AddSnippet(strings.Replace(desktopLegacyConnectedPlugAppArmor, "###SNAP_DESKTOP_FILE_RULES###", snippet+"\n", -1))
 
 	return nil

interfaces/builtin/unity7.go

@@ -691,7 +691,7 @@ func (iface *unity7Interface) AppArmorConnectedPlug(spec *apparmor.Specification
 	snippet := strings.Replace(unity7ConnectedPlugAppArmor, old, new, -1)
 
 	old = "###SNAP_DESKTOP_FILE_RULES###"
-	new = strings.Join(getDesktopFileRules(plug.Snap().DesktopPrefix()), "\n")
+	new = strings.Join(getDesktopFileRules(plug.Snap().DesktopPrefix(), spec), "\n")
 	snippet = strings.Replace(snippet, old, new+"\n", -1)
 
 	spec.AddSnippet(snippet)

interfaces/builtin/utils.go

@@ -27,6 +27,7 @@ import (
 
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/interfaces"
+	"github.com/snapcore/snapd/interfaces/apparmor"
 	"github.com/snapcore/snapd/snap"
 )
 
@@ -110,7 +111,23 @@ 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) []string {
+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
+			}
+		}
+	}
+
 	baseDir := dirs.SnapDesktopFilesDir
 
 	rules := []string{

interfaces/builtin/utils_test.go

@@ -25,6 +25,7 @@ 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/ifacetest"
 	"github.com/snapcore/snapd/snap"
@@ -181,7 +182,18 @@ func (s *utilsSuite) TestAareExclusivePatternsInvalid(c *C) {
 }
 
 func (s *utilsSuite) TestGetDesktopFileRules(c *C) {
-	res := builtin.GetDesktopFileRules("foo-bar")
+	// fake apparmor.Specification
+	info := snap.Info{}
+	snapSet, err := interfaces.NewSnapAppSet(&info, nil)
+	c.Assert(err, IsNil)
+	plugInfo := snap.PlugInfo{
+		Name: "test-plug",
+	}
+	snapSet.Info().Plugs = make(map[string]*snap.PlugInfo)
+	snapSet.Info().Plugs["test-plug"] = &plugInfo
+	spec := apparmor.NewSpecification(snapSet)
+
+	res := builtin.GetDesktopFileRules("foo-bar", spec)
 	c.Check(res, DeepEquals, []string{
 		"# Support applications which use the unity messaging menu, xdg-mime, etc",
 		"# This leaks the names of snaps with desktop files",
@@ -203,6 +215,26 @@ func (s *utilsSuite) TestGetDesktopFileRules(c *C) {
 	})
 }
 
+func (s *utilsSuite) TestGetDesktopFileRulesWithDesktopLaunchPlug(c *C) {
+	// fake apparmor.Specification
+	info := snap.Info{}
+	snapSet, err := interfaces.NewSnapAppSet(&info, nil)
+	c.Assert(err, IsNil)
+	// although usually the name is equal to the interface, this is not
+	// guaranteed, so to test it right we must try with a name that is
+	// different than the interface.
+	plugInfo := snap.PlugInfo{
+		Name:      "desktop-launch-iface",
+		Interface: "desktop-launch",
+	}
+	snapSet.Info().Plugs = make(map[string]*snap.PlugInfo)
+	snapSet.Info().Plugs["desktop-launch-iface"] = &plugInfo
+	spec := apparmor.NewSpecification(snapSet)
+
+	res := builtin.GetDesktopFileRules("foo-bar", spec)
+	c.Check(res, IsNil)
+}
+
 func MockPlug(c *C, yaml string, si *snap.SideInfo, plugName string) *snap.PlugInfo {
 	return builtin.MockPlug(c, yaml, si, plugName)
 }