diff --git a/interfaces/builtin/desktop_legacy.go b/interfaces/builtin/desktop_legacy.go index 3330b855b200..3d2f97dd87a3 100644 --- a/interfaces/builtin/desktop_legacy.go +++ b/interfaces/builtin/desktop_legacy.go @@ -395,7 +395,7 @@ func (iface *desktopLegacyInterface) AppArmorConnectedPlug(spec *apparmor.Specif // 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") + desktopSnippet := strings.Join(getDesktopFileRules(plug.Snap()), "\n") spec.AddPrioritizedSnippet(desktopSnippet, prioritizedSnippetDesktopFileAccess, desktopLegacyAndUnity7Priority) return nil diff --git a/interfaces/builtin/desktop_legacy_test.go b/interfaces/builtin/desktop_legacy_test.go index 981ddc012747..ca9c51be3afa 100644 --- a/interfaces/builtin/desktop_legacy_test.go +++ b/interfaces/builtin/desktop_legacy_test.go @@ -87,10 +87,6 @@ func (s *DesktopLegacyInterfaceSuite) TestAppArmorSpec(c *C) { // getDesktopFileRules() rules c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `# This leaks the names of snaps with desktop files`) c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `/var/lib/snapd/desktop/applications/ r,`) - c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `/var/lib/snapd/desktop/applications/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,`) - c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `deny /var/lib/snapd/desktop/applications/@{SNAP_INSTANCE_DESKTOP}[^_.]*.desktop r,`) - c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `deny /var/lib/snapd/desktop/applications/[^c]* r,`) - c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `deny /var/lib/snapd/desktop/applications/consume[^r]* r,`) // connected plug to core slot appSet, err = interfaces.NewSnapAppSet(s.coreSlot.Snap(), nil) @@ -111,3 +107,9 @@ func (s *DesktopLegacyInterfaceSuite) TestStaticInfo(c *C) { func (s *DesktopLegacyInterfaceSuite) TestInterfaces(c *C) { c.Check(builtin.Interfaces(), testutil.DeepContains, s.iface) } + +// Test how desktop-legacy interface interacts desktop-file-ids attribute in desktop interface. +var _ = Suite(&desktopFileRulesBaseSuite{ + iface: "desktop-legacy", + slotYaml: desktopLegacyCoreYaml, +}) diff --git a/interfaces/builtin/export_test.go b/interfaces/builtin/export_test.go index feedc46ebe1e..2be4683dd588 100644 --- a/interfaces/builtin/export_test.go +++ b/interfaces/builtin/export_test.go @@ -25,8 +25,10 @@ import ( . "gopkg.in/check.v1" "github.com/snapcore/snapd/interfaces" + "github.com/snapcore/snapd/sandbox/apparmor" "github.com/snapcore/snapd/snap" "github.com/snapcore/snapd/snap/snaptest" + "github.com/snapcore/snapd/testutil" ) var ( @@ -34,7 +36,6 @@ var ( ResolveSpecialVariable = resolveSpecialVariable ImplicitSystemPermanentSlot = implicitSystemPermanentSlot ImplicitSystemConnectedSlot = implicitSystemConnectedSlot - AareExclusivePatterns = aareExclusivePatterns GetDesktopFileRules = getDesktopFileRules StringListAttribute = stringListAttribute PolkitPoliciesSupported = polkitPoliciesSupported @@ -146,3 +147,11 @@ func MockPolkitDaemonPaths(path1, path2 string) (restore func()) { polkitDaemonPath2 = oldDaemonPath2 } } + +func MockApparmorGenerateAAREExclusionPatterns(fn func(excludePatterns []string, opts *apparmor.AAREExclusionPatternsOptions) (string, error)) (restore func()) { + return testutil.Mock(&apparmorGenerateAAREExclusionPatterns, fn) +} + +func MockDesktopFilesFromMount(fn func(s *snap.Info) ([]string, error)) (restore func()) { + return testutil.Mock(&desktopFilesFromMount, fn) +} diff --git a/interfaces/builtin/unity7.go b/interfaces/builtin/unity7.go index c82816950259..71144c4cfd2c 100644 --- a/interfaces/builtin/unity7.go +++ b/interfaces/builtin/unity7.go @@ -695,7 +695,7 @@ func (iface *unity7Interface) AppArmorConnectedPlug(spec *apparmor.Specification // 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") + desktopSnippet := strings.Join(getDesktopFileRules(plug.Snap()), "\n") spec.AddPrioritizedSnippet(desktopSnippet, prioritizedSnippetDesktopFileAccess, desktopLegacyAndUnity7Priority) return nil } diff --git a/interfaces/builtin/unity7_test.go b/interfaces/builtin/unity7_test.go index 413a55faeae2..a1ef1630ab2b 100644 --- a/interfaces/builtin/unity7_test.go +++ b/interfaces/builtin/unity7_test.go @@ -92,10 +92,6 @@ func (s *Unity7InterfaceSuite) TestUsedSecuritySystems(c *C) { // getDesktopFileRules() rules c.Assert(apparmorSpec.SnippetForTag("snap.other-snap.app2"), testutil.Contains, `# This leaks the names of snaps with desktop files`) c.Assert(apparmorSpec.SnippetForTag("snap.other-snap.app2"), testutil.Contains, `/var/lib/snapd/desktop/applications/ r,`) - c.Assert(apparmorSpec.SnippetForTag("snap.other-snap.app2"), testutil.Contains, `/var/lib/snapd/desktop/applications/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,`) - c.Assert(apparmorSpec.SnippetForTag("snap.other-snap.app2"), testutil.Contains, `deny /var/lib/snapd/desktop/applications/@{SNAP_INSTANCE_DESKTOP}[^_.]*.desktop r,`) - c.Assert(apparmorSpec.SnippetForTag("snap.other-snap.app2"), testutil.Contains, `deny /var/lib/snapd/desktop/applications/[^o]* r,`) - c.Assert(apparmorSpec.SnippetForTag("snap.other-snap.app2"), testutil.Contains, `deny /var/lib/snapd/desktop/applications/other-sna[^p]* r,`) // connected plugs for instance name have a non-nil security snippet for apparmor apparmorSpec = apparmor.NewSpecification(s.plugInst.AppSet()) @@ -124,3 +120,9 @@ func (s *Unity7InterfaceSuite) TestUsedSecuritySystems(c *C) { func (s *Unity7InterfaceSuite) TestInterfaces(c *C) { c.Check(builtin.Interfaces(), testutil.DeepContains, s.iface) } + +// Test how unity7 interface interacts desktop-file-ids attribute in desktop interface. +var _ = Suite(&desktopFileRulesBaseSuite{ + iface: "unity7", + slotYaml: unity7mockSlotSnapInfoYaml, +}) diff --git a/interfaces/builtin/utils.go b/interfaces/builtin/utils.go index 6305eeaeadd5..b52ab784dcfa 100644 --- a/interfaces/builtin/utils.go +++ b/interfaces/builtin/utils.go @@ -24,9 +24,12 @@ import ( "fmt" "path/filepath" "regexp" + "strings" "github.com/snapcore/snapd/dirs" "github.com/snapcore/snapd/interfaces" + "github.com/snapcore/snapd/logger" + "github.com/snapcore/snapd/sandbox/apparmor" "github.com/snapcore/snapd/snap" ) @@ -76,61 +79,107 @@ func verifySlotPathAttribute(slotRef *interfaces.SlotRef, attrs interfaces.Attre return cleanPath, nil } -// aareExclusivePatterns takes a string and generates deny alternations. Eg, -// aareExclusivePatterns("foo") returns: -// -// []string{ -// "[^f]*", -// "f[^o]*", -// "fo[^o]*", -// } -// -// For a more generic version of this see GenerateAAREExclusionPatterns -// TODO: can this be rewritten to use/share code with that function instead? -func aareExclusivePatterns(orig string) []string { - // This function currently is only intended to be used with desktop - // prefixes as calculated by info.DesktopPrefix (the snap name and - // instance name, if present). To avoid having to worry about aare - // special characters, etc, perform ValidateDesktopPrefix() and return - // an empty list if invalid. If this function is modified for other - // input, aare/quoting/etc will have to be considered. - if !snap.ValidateDesktopPrefix(orig) { - return nil +func getDesktopFileRulesFallback() []string { + return []string{ + "# Support applications which use the unity messaging menu, xdg-mime, etc", + "# This leaks the names of snaps with desktop files", + fmt.Sprintf("%s/ r,", dirs.SnapDesktopFilesDir), + "# Allowing reading only our desktop files (required by (at least) the unity", + "# messaging menu).", + "# parallel-installs: this leaks read access to desktop files owned by keyed", + "# instances of @{SNAP_NAME} to @{SNAP_NAME} snap", + fmt.Sprintf("%s/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,", dirs.SnapDesktopFilesDir), + "# Explicitly deny access to other snap's desktop files", + fmt.Sprintf("deny %s/@{SNAP_INSTANCE_DESKTOP}[^_.]*.desktop r,", dirs.SnapDesktopFilesDir), + // XXX: Do we need to generate extensive deny rules for the fallback too? } +} - s := make([]string, len(orig)) - - prefix := "" - for i, letter := range orig { - prefix = orig[:i] - s[i] = fmt.Sprintf("%s[^%c]*", prefix, letter) - } - return s +var apparmorGenerateAAREExclusionPatterns = apparmor.GenerateAAREExclusionPatterns +var desktopFilesFromMount = func(s *snap.Info) ([]string, error) { + opts := snap.DesktopFilesFromMountOptions{MangleFileNames: true} + return s.DesktopFilesFromMount(opts) } -// getDesktopFileRules() generates snippet rules for -// allowing access to the specified snap's desktop files in -// 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 { +// getDesktopFileRules generates snippet rules for allowing access to the +// specified snap's desktop files in 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) +// +// The snap must be mounted. +func getDesktopFileRules(s *snap.Info) []string { baseDir := dirs.SnapDesktopFilesDir rules := []string{ "# Support applications which use the unity messaging menu, xdg-mime, etc", "# This leaks the names of snaps with desktop files", fmt.Sprintf("%s/ r,", baseDir), + } + + // Generate allow rules + rules = append(rules, "# Allowing reading only our desktop files (required by (at least) the unity", "# messaging menu).", "# parallel-installs: this leaks read access to desktop files owned by keyed", "# instances of @{SNAP_NAME} to @{SNAP_NAME} snap", - fmt.Sprintf("%s/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,", baseDir), - "# Explicitly deny access to other snap's desktop files", - fmt.Sprintf("deny %s/@{SNAP_INSTANCE_DESKTOP}[^_.]*.desktop r,", baseDir), + fmt.Sprintf("%s/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,", dirs.SnapDesktopFilesDir), + ) + // For allow rules let's be more defensive and not depend on desktop files + // shipped by the snap like what is done below in the deny rules so that if + // a snap figured out a way to trick the checks below it can only shoot + // itself in the foot and deny more stuff. + // Although, given the extensive use of ValidateNoAppArmorRegexp below this + // should never, but still it is better to play it safe with allow rules + desktopFileIDs, err := s.DesktopPlugFileIDs() + if err != nil { + logger.Noticef("error: %v", err) + return getDesktopFileRulesFallback() + } + for _, desktopFileID := range desktopFileIDs { + // Validate IDs, This check should never be triggered because + // desktop-file-ids are already validated during install. + // But still it is better to play it safe and check AARE characters anyway. + if err := apparmor.ValidateNoAppArmorRegexp(desktopFileID); err != nil { + logger.Noticef("error: %v", err) + return getDesktopFileRulesFallback() + } + rules = append(rules, fmt.Sprintf("%s/%s r,", baseDir, desktopFileID+".desktop")) + } + + // Generate deny rules to suppress apparmor warnings + desktopFiles, err := desktopFilesFromMount(s) + if err != nil { + logger.Noticef("error: %v", err) + return getDesktopFileRulesFallback() + } + if len(desktopFiles) == 0 { + // Nothing to do + return getDesktopFileRulesFallback() + } + excludeOpts := &apparmor.AAREExclusionPatternsOptions{ + Prefix: fmt.Sprintf("deny %s", baseDir), + Suffix: ".desktop r,", + } + excludePatterns := make([]string, 0, len(desktopFiles)) + for _, desktopFile := range desktopFiles { + // Check that desktop files found don't contain AARE characters. + // This check should never be triggered because: + // - Prefixed desktop files are sanitized to only contain non-AARE characters + // - Desktop file ids are validated to only contain non-AARE characters + // But still it is better to play it safe and check AARE characters anyway. + if err := apparmor.ValidateNoAppArmorRegexp(desktopFile); err != nil { + logger.Noticef("error: %v", err) + return getDesktopFileRulesFallback() + } + excludePatterns = append(excludePatterns, "/"+strings.TrimSuffix(filepath.Base(desktopFile), ".desktop")) } - for _, t := range aareExclusivePatterns(snapInstanceName) { - rules = append(rules, fmt.Sprintf("deny %s/%s r,", baseDir, t)) + excludeRules, err := apparmorGenerateAAREExclusionPatterns(excludePatterns, excludeOpts) + if err != nil { + logger.Noticef("error: %v", err) + return getDesktopFileRulesFallback() } + rules = append(rules, "# Explicitly deny access to other snap's desktop files") + rules = append(rules, excludeRules) return rules } diff --git a/interfaces/builtin/utils_test.go b/interfaces/builtin/utils_test.go index 04847c5c6e4c..df665119ef01 100644 --- a/interfaces/builtin/utils_test.go +++ b/interfaces/builtin/utils_test.go @@ -20,15 +20,23 @@ package builtin_test import ( + "errors" "fmt" + "os" + "path/filepath" + "strings" . "gopkg.in/check.v1" + "github.com/snapcore/snapd/dirs" "github.com/snapcore/snapd/interfaces" + "github.com/snapcore/snapd/interfaces/apparmor" "github.com/snapcore/snapd/interfaces/builtin" "github.com/snapcore/snapd/interfaces/ifacetest" + apparmorutils "github.com/snapcore/snapd/sandbox/apparmor" "github.com/snapcore/snapd/snap" "github.com/snapcore/snapd/snap/snaptest" + "github.com/snapcore/snapd/testutil" ) type utilsSuite struct { @@ -74,112 +82,6 @@ func (s *utilsSuite) TestImplicitSystemConnectedSlot(c *C) { c.Assert(builtin.ImplicitSystemConnectedSlot(s.conSlotSnapd), Equals, true) } -func (s *utilsSuite) TestAareExclusivePatterns(c *C) { - res := builtin.AareExclusivePatterns("foo-bar") - c.Check(res, DeepEquals, []string{ - "[^f]*", - "f[^o]*", - "fo[^o]*", - "foo[^-]*", - "foo-[^b]*", - "foo-b[^a]*", - "foo-ba[^r]*", - }) -} - -func (s *utilsSuite) TestAareExclusivePatternsInstance(c *C) { - res := builtin.AareExclusivePatterns("foo-bar+baz") - c.Check(res, DeepEquals, []string{ - "[^f]*", - "f[^o]*", - "fo[^o]*", - "foo[^-]*", - "foo-[^b]*", - "foo-b[^a]*", - "foo-ba[^r]*", - "foo-bar[^+]*", - "foo-bar+[^b]*", - "foo-bar+b[^a]*", - "foo-bar+ba[^z]*", - }) -} - -func (s *utilsSuite) TestAareExclusivePatternsInvalid(c *C) { - bad := []string{ - // AARE in name (man apparmor.d: AARE = ?*[]{}^) - "bad{", - "ba}d", - "b[ad", - "]bad", - "b^d", - "b*d", - "b?d", - "bad{+good", - "ba}d+good", - "b[ad+good", - "]bad+good", - "b^d+good", - "b*d+good", - "b?d+good", - // AARE in instance (man apparmor.d: AARE = ?*[]{}^) - "good+bad{", - "good+ba}d", - "good+b[ad", - "good+]bad", - "good+b^d", - "good+b*d", - "good+b?d", - // various other unexpected in name - "+good", - "/bad", - "bad,", - ".bad.", - "ba'd", - "b\"ad", - "=bad", - "b\\0d", - "b\ad", - "(bad", - "bad)", - "bad", - "bad!", - "b#d", - ":bad", - "b@d", - "@{BAD}", - "b**d", - "bad -> evil", - "b a d", - // various other unexpected in instance - "good+", - "good+/bad", - "good+bad,", - "good+.bad.", - "good+ba'd", - "good+b\"ad", - "good+=bad", - "good+b\\0d", - "good+b\ad", - "good+(bad", - "good+bad)", - "good+bad", - "good+bad!", - "good+b#d", - "good+:bad", - "good+b@d", - "good+@{BAD}", - "good+b**d", - "good+bad -> evil", - } - - for _, s := range bad { - res := builtin.AareExclusivePatterns(s) - c.Check(res, IsNil) - } -} - func MockPlug(c *C, yaml string, si *snap.SideInfo, plugName string) *snap.PlugInfo { return builtin.MockPlug(c, yaml, si, plugName) } @@ -261,3 +163,251 @@ plugs: c.Assert(list, IsNil) c.Assert(err, ErrorMatches, `"write" attribute must be a list of strings, not "\[1 two\]"`) } + +// desktopFileRulesBaseSuite should be extended by interfaces that use getDesktopFileRules() +// like unity7 and desktop-legacy +// +// TODO: Add a way to detect interfaces that use getDesktopFileRules() and don't have an +// instance of this test suite. +type desktopFileRulesBaseSuite struct { + iface string + slotYaml string + + rootDir string + fallbackRules []string +} + +func (s *desktopFileRulesBaseSuite) SetUpTest(c *C) { + s.rootDir = c.MkDir() + dirs.SetRootDir(s.rootDir) + + s.fallbackRules = []string{ + // generic fallback snap desktop rules are generated + fmt.Sprintf("%s/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,", dirs.SnapDesktopFilesDir), + "# Explicitly deny access to other snap's desktop files", + fmt.Sprintf("deny %s/@{SNAP_INSTANCE_DESKTOP}[^_.]*.desktop r,", dirs.SnapDesktopFilesDir), + } +} + +func (s *desktopFileRulesBaseSuite) TearDownTest(c *C) { + dirs.SetRootDir("") +} + +type testDesktopFileRulesOptions struct { + snapName string + desktopFiles []string + desktopFileIDs []string + isInstance bool + expectedRules []string +} + +func (s *desktopFileRulesBaseSuite) testDesktopFileRules(c *C, opts testDesktopFileRulesOptions) { + iface := builtin.MustInterface(s.iface) + + const mockPlugSnapInfoYamlTemplate = `name: %s +version: 1.0 +apps: + app2: + command: foo + plugs: + - %s + - desktop +` + mockPlugSnapInfoYaml := fmt.Sprintf(mockPlugSnapInfoYamlTemplate, opts.snapName, iface.Name()) + + if len(opts.desktopFileIDs) > 0 { + mockPlugSnapInfoYaml += ` +plugs: + desktop: + desktop-file-ids: [` + strings.Join(opts.desktopFileIDs, ",") + `] +` + } + + slot, _ := MockConnectedSlot(c, s.slotYaml, nil, iface.Name()) + plug, _ := MockConnectedPlug(c, mockPlugSnapInfoYaml, nil, iface.Name()) + securityTag := "snap." + opts.snapName + ".app2" + if opts.isInstance { + plug.AppSet().Info().InstanceKey = "instance" + securityTag = "snap." + opts.snapName + "_instance.app2" + } + + // mock snap desktop files under snap mount + guiDir := filepath.Join(plug.AppSet().Info().MountDir(), "meta", "gui") + c.Assert(os.MkdirAll(guiDir, 0755), IsNil) + for _, desktopFile := range opts.desktopFiles { + c.Assert(os.WriteFile(filepath.Join(guiDir, desktopFile), nil, 0644), IsNil) + } + + // connected plugs have a non-nil security snippet for apparmor + apparmorSpec := apparmor.NewSpecification(plug.AppSet()) + err := apparmorSpec.AddConnectedPlug(iface, plug, slot) + c.Assert(err, IsNil) + c.Assert(apparmorSpec.SnippetForTag(securityTag), testutil.Contains, `# This leaks the names of snaps with desktop files`) + c.Assert(apparmorSpec.SnippetForTag(securityTag), testutil.Contains, `/var/lib/snapd/desktop/applications/ r,`) + // check generated rules against expected rules + for _, rule := range opts.expectedRules { + // early exit on error for less confusing debugigng + c.Assert(apparmorSpec.SnippetForTag(securityTag), testutil.Contains, rule) + } +} + +func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesHappy(c *C) { + opts := testDesktopFileRulesOptions{ + snapName: "some-snap", + desktopFiles: []string{"org.example.desktop", "org.example.Foo.desktop", "org.example.Bar.desktop", "bar.desktop"}, + desktopFileIDs: []string{"org.example", "org.example.Foo"}, + isInstance: false, + expectedRules: []string{ + // allow rules for snap's desktop files + fmt.Sprintf("%s/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,", dirs.SnapDesktopFilesDir), // prefixed with DesktopPrefix() + fmt.Sprintf("%s r,", filepath.Join(dirs.SnapDesktopFilesDir, "org.example.desktop")), // desktop-file-ids, unchanged + fmt.Sprintf("%s r,", filepath.Join(dirs.SnapDesktopFilesDir, "org.example.Foo.desktop")), // desktop-file-ids, unchanged + // check all deny patterns are generated + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "[^so]**.desktop")), // ^s from some-snap and ^o from org + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{o[^r],s[^o]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{or[^g],so[^m]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{org[^.],som[^e]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{org.[^e],some[^-]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{org.e[^x],some-[^s]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{org.ex[^a],some-s[^n]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{org.exa[^m],some-sn[^a]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{org.exam[^p],some-sna[^p]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{org.examp[^l],some-snap[^_]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{org.exampl[^e],some-snap_[^bo]}**.desktop")), // some-snap_ common prefix then diverging + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{org.example[^.],some-snap_b[^a],some-snap_o[^r]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{org.example.[^F],some-snap_ba[^r],some-snap_or[^g]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{org.example.F[^o],some-snap_org[^.]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "{org.example.Fo[^o],some-snap_org.[^e]}**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_org.e[^x]**.desktop")), // org.example.Foo.desktop ended + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_org.ex[^a]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_org.exa[^m]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_org.exam[^p]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_org.examp[^l]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_org.exampl[^e]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_org.example[^.]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_org.example.[^B]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_org.example.B[^a]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_org.example.Ba[^r]**.desktop")), // longest pattern + }, + } + s.testDesktopFileRules(c, opts) +} + +func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesNoDesktopFilesFallback(c *C) { + opts := testDesktopFileRulesOptions{ + snapName: "some-snap", + desktopFiles: []string{}, + desktopFileIDs: []string{"org.example"}, + isInstance: false, + expectedRules: s.fallbackRules, + } + s.testDesktopFileRules(c, opts) +} + +func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesSnapMountErrorFallback(c *C) { + restore := builtin.MockDesktopFilesFromMount(func(s *snap.Info) ([]string, error) { + return nil, errors.New("boom") + }) + defer restore() + + opts := testDesktopFileRulesOptions{ + snapName: "some-snap", + desktopFiles: []string{"org.example.desktop", "org.example.Foo.desktop", "org.example.Bar.desktop", "bar.desktop"}, + desktopFileIDs: []string{"org.example"}, + isInstance: false, + expectedRules: s.fallbackRules, + } + s.testDesktopFileRules(c, opts) +} + +func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesAAREExclusionPatternsErrorFallback(c *C) { + restore := builtin.MockApparmorGenerateAAREExclusionPatterns(func(excludePatterns []string, opts *apparmorutils.AAREExclusionPatternsOptions) (string, error) { + return "", errors.New("boom") + }) + defer restore() + + opts := testDesktopFileRulesOptions{ + snapName: "some-snap", + desktopFiles: []string{"org.example.desktop", "org.example.Foo.desktop", "org.example.Bar.desktop", "bar.desktop"}, + desktopFileIDs: []string{"org.example"}, + isInstance: false, + expectedRules: s.fallbackRules, + } + s.testDesktopFileRules(c, opts) +} + +func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesCommonSnapNameAndDesktopFileID(c *C) { + opts := testDesktopFileRulesOptions{ + snapName: "some-snap", + desktopFiles: []string{"some-snap.example.desktop", "foo.desktop"}, + desktopFileIDs: []string{"some-snap.example"}, + isInstance: false, + expectedRules: []string{ + // allow rules for snap's desktop files + fmt.Sprintf("%s/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,", dirs.SnapDesktopFilesDir), // prefixed with DesktopPrefix() + fmt.Sprintf("%s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap.example.desktop")), // desktop-file-ids, unchanged + // check all deny patterns are generated + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "[^s]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "s[^o]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "so[^m]**.desktop")), + // ... skip some patterns + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-sna[^p]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap[^_.]**.desktop")), // some-snap common prefix then diverging + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap{.[^e],_[^f]}**.desktop")), + }, + } + s.testDesktopFileRules(c, opts) +} + +func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesSanitizedDesktopFileName(c *C) { + opts := testDesktopFileRulesOptions{ + snapName: "some-snap", + desktopFiles: []string{`AaZz09. -,._?**[]{}^"\$#.desktop`}, + isInstance: false, + expectedRules: []string{ + // allow rules for snap's desktop files + fmt.Sprintf("%s/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,", dirs.SnapDesktopFilesDir), + // check all deny patterns are generated + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "[^s]**.desktop")), + // desktop file name was sanitized by snap.MangleDesktopFileName + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_AaZz09.[^_]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_AaZz09._-_._____[^_]**.desktop")), + fmt.Sprintf("deny %s r,", filepath.Join(dirs.SnapDesktopFilesDir, "some-snap_AaZz09._-_.____________[^_]**.desktop")), + }, + } + s.testDesktopFileRules(c, opts) +} + +func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileName(c *C) { + // Stress the case where a snap file name skipped sanitization somehow + // This should never happen because snap.MangleDesktopFileName is called + restore := builtin.MockDesktopFilesFromMount(func(s *snap.Info) ([]string, error) { + return []string{"foo**?$.desktop"}, nil + }) + defer restore() + + opts := testDesktopFileRulesOptions{ + snapName: "some-snap", + desktopFiles: []string{"foo**?$.desktop"}, + isInstance: false, + expectedRules: s.fallbackRules, + } + s.testDesktopFileRules(c, opts) +} + +func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileIDs(c *C) { + // Stress the case where a desktop file ids attribute skipped validation during + // installation somehow + restore := builtin.MockDesktopFilesFromMount(func(s *snap.Info) ([]string, error) { + return []string{"org.*.example.desktop"}, nil + }) + defer restore() + + opts := testDesktopFileRulesOptions{ + snapName: "some-snap", + desktopFiles: []string{"org.*.example.desktop"}, + desktopFileIDs: []string{"org.*.example"}, + expectedRules: s.fallbackRules, + } + s.testDesktopFileRules(c, opts) +} diff --git a/snap/info.go b/snap/info.go index eb4548ba306a..ab2f24bd3d49 100644 --- a/snap/info.go +++ b/snap/info.go @@ -996,9 +996,16 @@ func (s *Info) DesktopPlugFileIDs() ([]string, error) { return desktopFileIDs, nil } -// MangleDesktopFileName returns the file name prefixed with Info.DesktopPrefix() -// unless its name (without the .desktop extension) is listed under the -// desktop-file-ids desktop interface attribute. +// MangleDesktopFileName returns the sanitized file name prefixed with Info.DesktopPrefix(). +// If the passed name (without the .desktop extension) is listed under the desktop-file-ids +// desktop interface plug attribute then the desktop file name is returned as is without +// mangling. +// +// File name sanitization is done by replacing any character not in [A-Za-z0-9-_.] by +// an underscore '_'. +// - "test*.desktop" -> "PREFIX_test_.desktop +// - "test 123.desktop" -> "PREFIX_test_123.desktop +// - "test, *$$.desktop" -> "PREFIX_test_____.desktop" func (s *Info) MangleDesktopFileName(desktopFile string) (string, error) { desktopFileIDs, err := s.DesktopPlugFileIDs() if err != nil { @@ -1014,11 +1021,20 @@ func (s *Info) MangleDesktopFileName(desktopFile string) (string, error) { return filepath.Join(dir, base), nil } } - // FIXME: don't blindly use the snap desktop filename, mangle it - // but we can't just use the app name because a desktop file - // may call the same app with multiple parameters, e.g. - // --create-new, --open-existing etc - return filepath.Join(dir, fmt.Sprintf("%s_%s", s.DesktopPrefix(), base)), nil + + // Sanitization logic shouldn't worry about being backware compatible because the + // desktop files are always written when snapd starts in ensureDesktopFilesUpdated. + sanitizedBase := "" + for _, c := range base { + if (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '-' || c == '.' { + sanitizedBase += string(c) + continue + } + // Replace with '_' + sanitizedBase += "_" + } + + return filepath.Join(dir, fmt.Sprintf("%s_%s", s.DesktopPrefix(), sanitizedBase)), nil } type DesktopFilesFromMountOptions struct { diff --git a/snap/info_test.go b/snap/info_test.go index 0916ac81126b..779b5e2155be 100644 --- a/snap/info_test.go +++ b/snap/info_test.go @@ -2625,6 +2625,9 @@ plugs: {"org.example.Foo.desktop", "foo_org.example.Foo.desktop"}, {"org.desktop", "foo_org.desktop"}, {"test.desktop", "foo_test.desktop"}, + // character not in [A-Za-z0-9-_.] are replaced by '_' + {"test**.desktop", "foo_test__.desktop"}, + {`AaZz09. -,._?**[]{}^"\$#` + "\x00" + "\000" + ".desktop", "foo_AaZz09._-_._______________.desktop"}, } { mangled, err := info.MangleDesktopFileName(tc.fname) c.Assert(err, IsNil, Commentf(tc.fname)) diff --git a/tests/lib/snaps/test-snapd-desktop-file-ids/bin/check-dirs b/tests/lib/snaps/test-snapd-desktop-file-ids/bin/check-dirs new file mode 100755 index 000000000000..35a597725cc5 --- /dev/null +++ b/tests/lib/snaps/test-snapd-desktop-file-ids/bin/check-dirs @@ -0,0 +1,5 @@ +#!/bin/sh + +for dir in "$@"; do + ls "$dir" +done diff --git a/tests/lib/snaps/test-snapd-desktop-file-ids/bin/check-files b/tests/lib/snaps/test-snapd-desktop-file-ids/bin/check-files new file mode 100755 index 000000000000..d1631e209dd2 --- /dev/null +++ b/tests/lib/snaps/test-snapd-desktop-file-ids/bin/check-files @@ -0,0 +1,5 @@ +#!/bin/sh + +for file in "$@"; do + cat "$file" +done diff --git "a/tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/bad. ,?**[]{}^\"\\$#.desktop" "b/tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/bad. ,?**[]{}^\"\\$#.desktop" new file mode 100644 index 000000000000..a6e4e6e0ec7f --- /dev/null +++ "b/tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/bad. ,?**[]{}^\"\\$#.desktop" @@ -0,0 +1,6 @@ +[Desktop Entry] +Name=test-snapd-desktop.cmd +Comment=A desktop file for test-snapd-desktop.cmd +Exec=test-snapd-desktop.cmd +Terminal=true +Type=Application diff --git a/tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/foo.desktop b/tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/foo.desktop new file mode 100644 index 000000000000..a6e4e6e0ec7f --- /dev/null +++ b/tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/foo.desktop @@ -0,0 +1,6 @@ +[Desktop Entry] +Name=test-snapd-desktop.cmd +Comment=A desktop file for test-snapd-desktop.cmd +Exec=test-snapd-desktop.cmd +Terminal=true +Type=Application diff --git a/tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/org.example.Foo.desktop b/tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/org.example.Foo.desktop new file mode 100644 index 000000000000..a6e4e6e0ec7f --- /dev/null +++ b/tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/org.example.Foo.desktop @@ -0,0 +1,6 @@ +[Desktop Entry] +Name=test-snapd-desktop.cmd +Comment=A desktop file for test-snapd-desktop.cmd +Exec=test-snapd-desktop.cmd +Terminal=true +Type=Application diff --git a/tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/org.example.desktop b/tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/org.example.desktop new file mode 100644 index 000000000000..a6e4e6e0ec7f --- /dev/null +++ b/tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/org.example.desktop @@ -0,0 +1,6 @@ +[Desktop Entry] +Name=test-snapd-desktop.cmd +Comment=A desktop file for test-snapd-desktop.cmd +Exec=test-snapd-desktop.cmd +Terminal=true +Type=Application diff --git a/tests/lib/snaps/test-snapd-desktop-file-ids/meta/snap.yaml b/tests/lib/snaps/test-snapd-desktop-file-ids/meta/snap.yaml new file mode 100644 index 000000000000..78a8187b76ae --- /dev/null +++ b/tests/lib/snaps/test-snapd-desktop-file-ids/meta/snap.yaml @@ -0,0 +1,17 @@ +name: test-snapd-desktop-file-ids +version: 1.0 +summary: Basic desktop interface test snap +description: A basic snap with desktop-file-ids + +apps: + check-files: + command: bin/check-files + plugs: [desktop, desktop-legacy] + check-dirs: + command: bin/check-dirs + plugs: [desktop, desktop-legacy] + +plugs: + desktop: + desktop-file-ids: + - org.example diff --git a/tests/main/desktop-file-ids/task.yaml b/tests/main/desktop-file-ids/task.yaml new file mode 100644 index 000000000000..0e3e59dd8b1e --- /dev/null +++ b/tests/main/desktop-file-ids/task.yaml @@ -0,0 +1,53 @@ +summary: Ensure that the desktop-file-ids desktop interface attribute works + +details: | + Check that when the desktop-file-ids desktop interface attribute + is set, the names of the desktop files matching the ids are not + mangled (with the snap's desktop prefix). + +# Disabled on Ubuntu Core because it doesn't provide the "desktop" slot. +# TODO: Enable for Ubuntu Core Desktop when it is available. +systems: + - -ubuntu-core-* + +environment: + DIR: /var/lib/snapd/desktop/applications + +prepare: | + "$TESTSTOOLS"/snaps-state install-local test-snapd-desktop-file-ids + touch "$DIR/test-confinement.desktop" + tests.session -u test prepare + +restore: | + rm -f "$DIR/test-confinement.desktop" + tests.session -u test restore + +execute: | + files="$DIR/org.example.desktop $DIR/test-snapd-desktop-file-ids_foo.desktop $DIR/test-snapd-desktop-file-ids_org.example.Foo.desktop $DIR/test-snapd-desktop-file-ids_bad.______________.desktop" + + # Connect desktop-legacy interface to test generated desktop allow/deny rules + # only allows access to the snap's desktop files + snap connect test-snapd-desktop-file-ids:desktop-legacy + + # Check that desktop files are installed with expected names and implicitly check + # that for confined systems the snap can access its own desktop files when + # desktop-legacy interface plug is connected + echo "Check that desktop files are installed as expected" + # shellcheck disable=SC2086 + tests.session -u test exec test-snapd-desktop-file-ids.check-dirs $DIR + # shellcheck disable=SC2086 + tests.session -u test exec test-snapd-desktop-file-ids.check-files $files + if [ "$(snap debug confinement)" == strict ]; then + # Check that snap cannot access other desktop files + not tests.session -u test exec test-snapd-desktop-file-ids.check-files "$DIR/test-confinement.desktop" + fi + + snap remove --purge test-snapd-desktop-file-ids + echo "Check desktop files are removed" + for file in $files; do + not test -f "$file" + done + echo "But not other snaps' desktop files" + test -f "$DIR/test-confinement.desktop" + + # TODO: Add check for parallel installs diff --git a/wrappers/desktop.go b/wrappers/desktop.go index 1b0c4e8e31eb..eba18d8f4332 100644 --- a/wrappers/desktop.go +++ b/wrappers/desktop.go @@ -260,6 +260,9 @@ func deriveDesktopFilesContent(s *snap.Info) (map[string]osutil.FileState, error if err != nil { return nil, err } + if _, exists := content[base]; exists { + return nil, fmt.Errorf("duplicate desktop file name after mangling") + } fileContent, err := os.ReadFile(df) if err != nil { return nil, err diff --git a/wrappers/desktop_test.go b/wrappers/desktop_test.go index a8dae0250e34..7cbf5dccf112 100644 --- a/wrappers/desktop_test.go +++ b/wrappers/desktop_test.go @@ -104,6 +104,24 @@ func (s *desktopSuite) TestEnsurePackageDesktopFiles(c *C) { c.Assert(osutil.FileExists(oldDesktopFilePath), Equals, false) } +func (s *desktopSuite) TestEnsurePackageDesktopFilesMangledDuplicateError(c *C) { + expectedDesktopFilePath := filepath.Join(dirs.SnapDesktopFilesDir, "foo_foobar._.desktop") + c.Assert(osutil.FileExists(expectedDesktopFilePath), Equals, false) + + info := snaptest.MockSnap(c, desktopAppYaml, &snap.SideInfo{Revision: snap.R(11)}) + baseDir := info.MountDir() + c.Assert(os.MkdirAll(filepath.Join(baseDir, "meta", "gui"), 0755), IsNil) + // When mangled, both files will be foo_foobar._.desktop and should error + c.Assert(os.WriteFile(filepath.Join(baseDir, "meta", "gui", "foobar.*.desktop"), mockDesktopFile, 0644), IsNil) + c.Assert(os.WriteFile(filepath.Join(baseDir, "meta", "gui", "foobar.$.desktop"), mockDesktopFile, 0644), IsNil) + + err := wrappers.EnsureSnapDesktopFiles([]*snap.Info{info}) + c.Assert(err, ErrorMatches, "duplicate desktop file name after mangling") + + // Nothing shoul dhave been created + c.Assert(osutil.FileExists(expectedDesktopFilePath), Equals, false) +} + func (s *desktopSuite) testEnsurePackageDesktopFilesWithDesktopInterface(c *C, hasDesktopFileIDs bool) { var desktopAppYaml = ` name: foo