From a315bf8512f44195979479b2eb143fd692e7be91 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Fri, 30 Aug 2024 13:52:40 +0300 Subject: [PATCH 1/9] i/builtin: support desktop-file-ids in desktop files rule generation Signed-off-by: Zeyad Gouda --- interfaces/builtin/desktop_legacy.go | 2 +- interfaces/builtin/desktop_legacy_test.go | 10 +- interfaces/builtin/export_test.go | 11 +- interfaces/builtin/unity7.go | 2 +- interfaces/builtin/unity7_test.go | 10 +- interfaces/builtin/utils.go | 127 ++++-- interfaces/builtin/utils_test.go | 362 +++++++++++++----- snap/info.go | 32 +- snap/info_test.go | 3 + .../bin/check-dirs | 5 + .../bin/check-files | 5 + .../meta/gui/bad. ,?**[]{}^\\$#.desktop" | 6 + .../meta/gui/foo.desktop | 6 + .../meta/gui/org.example.Foo.desktop | 6 + .../meta/gui/org.example.desktop | 6 + .../meta/snap.yaml | 17 + tests/main/desktop-file-ids/task.yaml | 53 +++ wrappers/desktop.go | 3 + wrappers/desktop_test.go | 18 + 19 files changed, 520 insertions(+), 164 deletions(-) create mode 100755 tests/lib/snaps/test-snapd-desktop-file-ids/bin/check-dirs create mode 100755 tests/lib/snaps/test-snapd-desktop-file-ids/bin/check-files create mode 100644 "tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/bad. ,?**[]{}^\\$#.desktop" create mode 100644 tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/foo.desktop create mode 100644 tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/org.example.Foo.desktop create mode 100644 tests/lib/snaps/test-snapd-desktop-file-ids/meta/gui/org.example.desktop create mode 100644 tests/lib/snaps/test-snapd-desktop-file-ids/meta/snap.yaml create mode 100644 tests/main/desktop-file-ids/task.yaml diff --git a/interfaces/builtin/desktop_legacy.go b/interfaces/builtin/desktop_legacy.go index 3330b855b20..3d2f97dd87a 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 981ddc01274..ca9c51be3af 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 feedc46ebe1..2be4683dd58 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 c8281695025..71144c4cfd2 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 413a55faeae..a1ef1630ab2 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 6305eeaeadd..b52ab784dcf 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 04847c5c6e4..df665119ef0 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 eb4548ba306..ab2f24bd3d4 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 0916ac81126..779b5e2155b 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 00000000000..35a597725cc --- /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 00000000000..d1631e209dd --- /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 00000000000..a6e4e6e0ec7 --- /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 00000000000..a6e4e6e0ec7 --- /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 00000000000..a6e4e6e0ec7 --- /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 00000000000..a6e4e6e0ec7 --- /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 00000000000..78a8187b76a --- /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 00000000000..055de26481c --- /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 1b0c4e8e31e..eba18d8f433 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 a8dae0250e3..7cbf5dccf11 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 From 4c93ce8beef40fc2f88675642f4e3a3db0ebd557 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Sun, 15 Sep 2024 16:23:13 +0300 Subject: [PATCH 2/9] i/builtin: address review comments (thanks @bboozzoo) Signed-off-by: Zeyad Gouda --- interfaces/builtin/desktop_legacy.go | 4 +- interfaces/builtin/unity7.go | 2 +- interfaces/builtin/utils.go | 79 +++++++++++++++------------- snap/info.go | 25 +++++---- 4 files changed, 60 insertions(+), 50 deletions(-) diff --git a/interfaces/builtin/desktop_legacy.go b/interfaces/builtin/desktop_legacy.go index 3d2f97dd87a..3b9900f02b3 100644 --- a/interfaces/builtin/desktop_legacy.go +++ b/interfaces/builtin/desktop_legacy.go @@ -20,8 +20,6 @@ package builtin import ( - "strings" - "github.com/snapcore/snapd/interfaces" "github.com/snapcore/snapd/interfaces/apparmor" ) @@ -395,7 +393,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()), "\n") + desktopSnippet := getDesktopFileRules(plug.Snap()) spec.AddPrioritizedSnippet(desktopSnippet, prioritizedSnippetDesktopFileAccess, desktopLegacyAndUnity7Priority) return nil diff --git a/interfaces/builtin/unity7.go b/interfaces/builtin/unity7.go index 71144c4cfd2..c621dc895c1 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()), "\n") + desktopSnippet := getDesktopFileRules(plug.Snap()) spec.AddPrioritizedSnippet(desktopSnippet, prioritizedSnippetDesktopFileAccess, desktopLegacyAndUnity7Priority) return nil } diff --git a/interfaces/builtin/utils.go b/interfaces/builtin/utils.go index b52ab784dcf..2be229c68ee 100644 --- a/interfaces/builtin/utils.go +++ b/interfaces/builtin/utils.go @@ -79,20 +79,21 @@ func verifySlotPathAttribute(slotRef *interfaces.SlotRef, attrs interfaces.Attre return cleanPath, 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? - } +func getDesktopFileRulesFallback() string { + const template = ` +# Support applications which use the unity messaging menu, xdg-mime, etc +# This leaks the names of snaps with desktop files +%[1]s/ r, +# 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 +%[1]s/@{SNAP_INSTANCE_DESKTOP}_*.desktop r, +# Explicitly deny access to other snap's desktop files +deny %[1]s/@{SNAP_INSTANCE_DESKTOP}[^_.]*.desktop r, +` + // XXX: Do we need to generate extensive deny rules for the fallback too? + return fmt.Sprintf(template[1:], dirs.SnapDesktopFilesDir) } var apparmorGenerateAAREExclusionPatterns = apparmor.GenerateAAREExclusionPatterns @@ -107,32 +108,36 @@ var desktopFilesFromMount = func(s *snap.Info) ([]string, error) { // 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 +func getDesktopFileRules(s *snap.Info) string { + const template = ` +# Support applications which use the unity messaging menu, xdg-mime, etc +# This leaks the names of snaps with desktop files +%s/ r, - 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), - } +# Allow rules: +%s + +# Deny rules: +%s +` // 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,", dirs.SnapDesktopFilesDir), - ) + allowRules := ` +# 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 +` + allowRules += fmt.Sprintf("%s/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,\n", 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 + // should never fail, but still it is better to play it safe with allow rules. desktopFileIDs, err := s.DesktopPlugFileIDs() if err != nil { - logger.Noticef("error: %v", err) + logger.Noticef("cannot list desktop plug file IDs: %v", err) return getDesktopFileRulesFallback() } for _, desktopFileID := range desktopFileIDs { @@ -140,13 +145,14 @@ func getDesktopFileRules(s *snap.Info) []string { // 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) + logger.Noticef("invalid desktop file id %q found in %q: %v", desktopFileID, s.InstanceName(), err) return getDesktopFileRulesFallback() } - rules = append(rules, fmt.Sprintf("%s/%s r,", baseDir, desktopFileID+".desktop")) + allowRules += fmt.Sprintf("%s/%s r,\n", dirs.SnapDesktopFilesDir, desktopFileID+".desktop") } // Generate deny rules to suppress apparmor warnings + denyRules := "# Explicitly deny access to other snap's desktop files\n" desktopFiles, err := desktopFilesFromMount(s) if err != nil { logger.Noticef("error: %v", err) @@ -157,7 +163,7 @@ func getDesktopFileRules(s *snap.Info) []string { return getDesktopFileRulesFallback() } excludeOpts := &apparmor.AAREExclusionPatternsOptions{ - Prefix: fmt.Sprintf("deny %s", baseDir), + Prefix: fmt.Sprintf("deny %s", dirs.SnapDesktopFilesDir), Suffix: ".desktop r,", } excludePatterns := make([]string, 0, len(desktopFiles)) @@ -168,7 +174,7 @@ func getDesktopFileRules(s *snap.Info) []string { // - 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) + logger.Noticef("invalid desktop file name %q found in snap %q: %v", desktopFile, s.InstanceName(), err) return getDesktopFileRulesFallback() } excludePatterns = append(excludePatterns, "/"+strings.TrimSuffix(filepath.Base(desktopFile), ".desktop")) @@ -178,10 +184,9 @@ func getDesktopFileRules(s *snap.Info) []string { logger.Noticef("error: %v", err) return getDesktopFileRulesFallback() } - rules = append(rules, "# Explicitly deny access to other snap's desktop files") - rules = append(rules, excludeRules) + denyRules += excludeRules - return rules + return fmt.Sprintf(template, dirs.SnapDesktopFilesDir, allowRules[1:], denyRules) } // stringListAttribute returns a list of strings for the given attribute key if the attribute exists. diff --git a/snap/info.go b/snap/info.go index ab2f24bd3d4..ac6f454a65a 100644 --- a/snap/info.go +++ b/snap/info.go @@ -996,6 +996,21 @@ func (s *Info) DesktopPlugFileIDs() ([]string, error) { return desktopFileIDs, nil } +func sanitizeDesktopFileName(base string) string { + var b strings.Builder + b.Grow(len(base)) + + for _, c := range base { + if (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '-' || c == '.' { + b.WriteRune(c) + } else { + b.WriteRune('_') + } + } + + return b.String() +} + // 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 @@ -1024,15 +1039,7 @@ func (s *Info) MangleDesktopFileName(desktopFile string) (string, error) { // 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 += "_" - } + sanitizedBase := sanitizeDesktopFileName(base) return filepath.Join(dir, fmt.Sprintf("%s_%s", s.DesktopPrefix(), sanitizedBase)), nil } From b364f5cae27eb9a95ca44083bd27513f54e895ac Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Mon, 16 Sep 2024 12:24:13 +0300 Subject: [PATCH 3/9] wrappers: ignore duplicates when installing snap desktop files Signed-off-by: Zeyad Gouda --- wrappers/desktop.go | 3 ++- wrappers/desktop_test.go | 11 +++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/wrappers/desktop.go b/wrappers/desktop.go index eba18d8f433..833d75d703e 100644 --- a/wrappers/desktop.go +++ b/wrappers/desktop.go @@ -261,7 +261,8 @@ func deriveDesktopFilesContent(s *snap.Info) (map[string]osutil.FileState, error return nil, err } if _, exists := content[base]; exists { - return nil, fmt.Errorf("duplicate desktop file name after mangling") + logger.Noticef("skipping %q as a duplicate file name %q was found after mangling", filepath.Base(df), base) + continue } fileContent, err := os.ReadFile(df) if err != nil { diff --git a/wrappers/desktop_test.go b/wrappers/desktop_test.go index 7cbf5dccf11..560553712bc 100644 --- a/wrappers/desktop_test.go +++ b/wrappers/desktop_test.go @@ -104,7 +104,7 @@ func (s *desktopSuite) TestEnsurePackageDesktopFiles(c *C) { c.Assert(osutil.FileExists(oldDesktopFilePath), Equals, false) } -func (s *desktopSuite) TestEnsurePackageDesktopFilesMangledDuplicateError(c *C) { +func (s *desktopSuite) TestEnsurePackageDesktopFilesMangledDuplicate(c *C) { expectedDesktopFilePath := filepath.Join(dirs.SnapDesktopFilesDir, "foo_foobar._.desktop") c.Assert(osutil.FileExists(expectedDesktopFilePath), Equals, false) @@ -116,10 +116,13 @@ func (s *desktopSuite) TestEnsurePackageDesktopFilesMangledDuplicateError(c *C) 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") + c.Assert(err, Equals, nil) - // Nothing shoul dhave been created - c.Assert(osutil.FileExists(expectedDesktopFilePath), Equals, false) + // Only one will be written, duplicates will be skipped + c.Assert(osutil.FileExists(expectedDesktopFilePath), Equals, true) + files, err := os.ReadDir(dirs.SnapDesktopFilesDir) + c.Assert(err, IsNil) + c.Assert(files, HasLen, 1) } func (s *desktopSuite) testEnsurePackageDesktopFilesWithDesktopInterface(c *C, hasDesktopFileIDs bool) { From b44c8a2624e9025cd2fcbb5edc7e673e80c2fa17 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Tue, 17 Sep 2024 17:30:18 +0300 Subject: [PATCH 4/9] many: s/DesktopFilesFromMount/DesktopFilesFromInstalledSnap Signed-off-by: Zeyad Gouda --- interfaces/builtin/export_test.go | 4 ++-- interfaces/builtin/utils.go | 8 ++++---- interfaces/builtin/utils_test.go | 6 +++--- snap/info.go | 6 +++--- snap/info_test.go | 18 +++++++++--------- wrappers/desktop.go | 2 +- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/interfaces/builtin/export_test.go b/interfaces/builtin/export_test.go index 2be4683dd58..e129d078014 100644 --- a/interfaces/builtin/export_test.go +++ b/interfaces/builtin/export_test.go @@ -152,6 +152,6 @@ func MockApparmorGenerateAAREExclusionPatterns(fn func(excludePatterns []string, return testutil.Mock(&apparmorGenerateAAREExclusionPatterns, fn) } -func MockDesktopFilesFromMount(fn func(s *snap.Info) ([]string, error)) (restore func()) { - return testutil.Mock(&desktopFilesFromMount, fn) +func MockDesktopFilesFromInstalledSnap(fn func(s *snap.Info) ([]string, error)) (restore func()) { + return testutil.Mock(&desktopFilesFromInstalledSnap, fn) } diff --git a/interfaces/builtin/utils.go b/interfaces/builtin/utils.go index 2be229c68ee..178d9aec11b 100644 --- a/interfaces/builtin/utils.go +++ b/interfaces/builtin/utils.go @@ -97,9 +97,9 @@ deny %[1]s/@{SNAP_INSTANCE_DESKTOP}[^_.]*.desktop r, } var apparmorGenerateAAREExclusionPatterns = apparmor.GenerateAAREExclusionPatterns -var desktopFilesFromMount = func(s *snap.Info) ([]string, error) { - opts := snap.DesktopFilesFromMountOptions{MangleFileNames: true} - return s.DesktopFilesFromMount(opts) +var desktopFilesFromInstalledSnap = func(s *snap.Info) ([]string, error) { + opts := snap.DesktopFilesFromInstalledSnapOptions{MangleFileNames: true} + return s.DesktopFilesFromInstalledSnap(opts) } // getDesktopFileRules generates snippet rules for allowing access to the @@ -153,7 +153,7 @@ func getDesktopFileRules(s *snap.Info) string { // Generate deny rules to suppress apparmor warnings denyRules := "# Explicitly deny access to other snap's desktop files\n" - desktopFiles, err := desktopFilesFromMount(s) + desktopFiles, err := desktopFilesFromInstalledSnap(s) if err != nil { logger.Noticef("error: %v", err) return getDesktopFileRulesFallback() diff --git a/interfaces/builtin/utils_test.go b/interfaces/builtin/utils_test.go index df665119ef0..70f9500ffe8 100644 --- a/interfaces/builtin/utils_test.go +++ b/interfaces/builtin/utils_test.go @@ -305,7 +305,7 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesNoDesktopFilesFallback(c } func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesSnapMountErrorFallback(c *C) { - restore := builtin.MockDesktopFilesFromMount(func(s *snap.Info) ([]string, error) { + restore := builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { return nil, errors.New("boom") }) defer restore() @@ -381,7 +381,7 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesSanitizedDesktopFileName 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) { + restore := builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { return []string{"foo**?$.desktop"}, nil }) defer restore() @@ -398,7 +398,7 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileName(c *C) 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) { + restore := builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { return []string{"org.*.example.desktop"}, nil }) defer restore() diff --git a/snap/info.go b/snap/info.go index ac6f454a65a..857e754a222 100644 --- a/snap/info.go +++ b/snap/info.go @@ -1044,13 +1044,13 @@ func (s *Info) MangleDesktopFileName(desktopFile string) (string, error) { return filepath.Join(dir, fmt.Sprintf("%s_%s", s.DesktopPrefix(), sanitizedBase)), nil } -type DesktopFilesFromMountOptions struct { +type DesktopFilesFromInstalledSnapOptions struct { // Mangles found desktop files using Info.MangleDesktopFileName() MangleFileNames bool } -// DesktopFilesFromMount returns the desktop files found under /meta/gui. -func (s *Info) DesktopFilesFromMount(opts DesktopFilesFromMountOptions) ([]string, error) { +// DesktopFilesFromInstalledSnap returns the desktop files found under /meta/gui. +func (s *Info) DesktopFilesFromInstalledSnap(opts DesktopFilesFromInstalledSnapOptions) ([]string, error) { rootDir := filepath.Join(s.MountDir(), "meta", "gui") if !osutil.IsDirectory(rootDir) { return nil, nil diff --git a/snap/info_test.go b/snap/info_test.go index 779b5e2155b..fedd3472e58 100644 --- a/snap/info_test.go +++ b/snap/info_test.go @@ -2651,7 +2651,7 @@ plugs: c.Assert(err, NotNil) } -func (s *infoSuite) TestDesktopFilesFromMountNoFiles(c *C) { +func (s *infoSuite) TestDesktopFilesFromInstalledSnapNoFiles(c *C) { const desktopAppYaml = ` name: foo version: 1.0 @@ -2660,12 +2660,12 @@ version: 1.0 info, err := snap.InfoFromSnapYaml([]byte(desktopAppYaml)) c.Assert(err, IsNil) - desktopFiles, err := info.DesktopFilesFromMount(snap.DesktopFilesFromMountOptions{}) + desktopFiles, err := info.DesktopFilesFromInstalledSnap(snap.DesktopFilesFromInstalledSnapOptions{}) c.Assert(err, IsNil) c.Assert(desktopFiles, IsNil) } -func (s *infoSuite) testDesktopFilesFromMount(c *C, mangle bool) { +func (s *infoSuite) testDesktopFilesFromInstalledSnap(c *C, mangle bool) { const desktopAppYaml = ` name: foo version: 1.0 @@ -2687,8 +2687,8 @@ plugs: c.Assert(err, IsNil) } - opts := snap.DesktopFilesFromMountOptions{MangleFileNames: mangle} - desktopFilesFound, err := info.DesktopFilesFromMount(opts) + opts := snap.DesktopFilesFromInstalledSnapOptions{MangleFileNames: mangle} + desktopFilesFound, err := info.DesktopFilesFromInstalledSnap(opts) c.Assert(err, IsNil) c.Assert(desktopFilesFound, HasLen, len(testDesktopFiles)) @@ -2711,12 +2711,12 @@ plugs: } } -func (s *infoSuite) TestDesktopFilesFromMount(c *C) { +func (s *infoSuite) TestDesktopFilesFromInstalledSnap(c *C) { const mangle = false - s.testDesktopFilesFromMount(c, mangle) + s.testDesktopFilesFromInstalledSnap(c, mangle) } -func (s *infoSuite) TestDesktopFilesFromMountMangled(c *C) { +func (s *infoSuite) TestDesktopFilesFromInstalledSnapMangled(c *C) { const mangle = true - s.testDesktopFilesFromMount(c, mangle) + s.testDesktopFilesFromInstalledSnap(c, mangle) } diff --git a/wrappers/desktop.go b/wrappers/desktop.go index 833d75d703e..d8cf330b815 100644 --- a/wrappers/desktop.go +++ b/wrappers/desktop.go @@ -248,7 +248,7 @@ func findDesktopFiles(rootDir string) ([]string, error) { } func deriveDesktopFilesContent(s *snap.Info) (map[string]osutil.FileState, error) { - desktopFiles, err := s.DesktopFilesFromMount(snap.DesktopFilesFromMountOptions{}) + desktopFiles, err := s.DesktopFilesFromInstalledSnap(snap.DesktopFilesFromInstalledSnapOptions{}) if err != nil { return nil, err } From 9ef0181e951373f019827345acc82ebde73e630d Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Wed, 18 Sep 2024 12:43:16 +0300 Subject: [PATCH 5/9] i/builtin: improve desktop file rules error logs Signed-off-by: Zeyad Gouda --- interfaces/builtin/utils.go | 8 ++++---- interfaces/builtin/utils_test.go | 29 +++++++++++++++++++++++++---- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/interfaces/builtin/utils.go b/interfaces/builtin/utils.go index 178d9aec11b..03d8672edbb 100644 --- a/interfaces/builtin/utils.go +++ b/interfaces/builtin/utils.go @@ -145,7 +145,7 @@ func getDesktopFileRules(s *snap.Info) string { // 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("invalid desktop file id %q found in %q: %v", desktopFileID, s.InstanceName(), err) + logger.Noticef("internal error: invalid desktop file id %q found in snap %q which should have failed in BeforePreparePlug checks: %v", desktopFileID, s.InstanceName(), err) return getDesktopFileRulesFallback() } allowRules += fmt.Sprintf("%s/%s r,\n", dirs.SnapDesktopFilesDir, desktopFileID+".desktop") @@ -155,7 +155,7 @@ func getDesktopFileRules(s *snap.Info) string { denyRules := "# Explicitly deny access to other snap's desktop files\n" desktopFiles, err := desktopFilesFromInstalledSnap(s) if err != nil { - logger.Noticef("error: %v", err) + logger.Noticef("internal error: failed to collect desktop files from snap %q: %v", s.InstanceName(), err) return getDesktopFileRulesFallback() } if len(desktopFiles) == 0 { @@ -174,14 +174,14 @@ func getDesktopFileRules(s *snap.Info) string { // - 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("invalid desktop file name %q found in snap %q: %v", desktopFile, s.InstanceName(), err) + logger.Noticef("internal error: invalid desktop file name %q found in snap %q which should have been validated earlier: %v", desktopFile, s.InstanceName(), err) return getDesktopFileRulesFallback() } excludePatterns = append(excludePatterns, "/"+strings.TrimSuffix(filepath.Base(desktopFile), ".desktop")) } excludeRules, err := apparmorGenerateAAREExclusionPatterns(excludePatterns, excludeOpts) if err != nil { - logger.Noticef("error: %v", err) + logger.Noticef("internal error: failed to generate deny rules for snap %q: %v", s.InstanceName(), err) return getDesktopFileRulesFallback() } denyRules += excludeRules diff --git a/interfaces/builtin/utils_test.go b/interfaces/builtin/utils_test.go index 70f9500ffe8..b6db3772aeb 100644 --- a/interfaces/builtin/utils_test.go +++ b/interfaces/builtin/utils_test.go @@ -33,6 +33,7 @@ import ( "github.com/snapcore/snapd/interfaces/apparmor" "github.com/snapcore/snapd/interfaces/builtin" "github.com/snapcore/snapd/interfaces/ifacetest" + "github.com/snapcore/snapd/logger" apparmorutils "github.com/snapcore/snapd/sandbox/apparmor" "github.com/snapcore/snapd/snap" "github.com/snapcore/snapd/snap/snaptest" @@ -305,7 +306,10 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesNoDesktopFilesFallback(c } func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesSnapMountErrorFallback(c *C) { - restore := builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { + logbuf, restore := logger.MockLogger() + defer restore() + + restore = builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { return nil, errors.New("boom") }) defer restore() @@ -318,10 +322,15 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesSnapMountErrorFallback(c expectedRules: s.fallbackRules, } s.testDesktopFileRules(c, opts) + + c.Check(logbuf.String(), testutil.Contains, `internal error: failed to collect desktop files from snap "some-snap": boom`) } func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesAAREExclusionPatternsErrorFallback(c *C) { - restore := builtin.MockApparmorGenerateAAREExclusionPatterns(func(excludePatterns []string, opts *apparmorutils.AAREExclusionPatternsOptions) (string, error) { + logbuf, restore := logger.MockLogger() + defer restore() + + restore = builtin.MockApparmorGenerateAAREExclusionPatterns(func(excludePatterns []string, opts *apparmorutils.AAREExclusionPatternsOptions) (string, error) { return "", errors.New("boom") }) defer restore() @@ -334,6 +343,8 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesAAREExclusionPatternsErr expectedRules: s.fallbackRules, } s.testDesktopFileRules(c, opts) + + c.Check(logbuf.String(), testutil.Contains, `internal error: failed to generate deny rules for snap "some-snap": boom`) } func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesCommonSnapNameAndDesktopFileID(c *C) { @@ -379,9 +390,12 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesSanitizedDesktopFileName } func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileName(c *C) { + logbuf, restore := logger.MockLogger() + defer restore() + // Stress the case where a snap file name skipped sanitization somehow // This should never happen because snap.MangleDesktopFileName is called - restore := builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { + restore = builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { return []string{"foo**?$.desktop"}, nil }) defer restore() @@ -393,12 +407,17 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileName(c *C) expectedRules: s.fallbackRules, } s.testDesktopFileRules(c, opts) + + c.Check(logbuf.String(), testutil.Contains, `internal error: invalid desktop file name "foo**?$.desktop" found in snap "some-snap" which should have been validated earlier: "foo**?$.desktop" contains a reserved apparmor char`) } func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileIDs(c *C) { + logbuf, restore := logger.MockLogger() + defer restore() + // Stress the case where a desktop file ids attribute skipped validation during // installation somehow - restore := builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { + restore = builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { return []string{"org.*.example.desktop"}, nil }) defer restore() @@ -410,4 +429,6 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileIDs(c *C) expectedRules: s.fallbackRules, } s.testDesktopFileRules(c, opts) + + c.Check(logbuf.String(), testutil.Contains, `internal error: invalid desktop file id "org.*.example" found in snap "some-snap" which should have failed in BeforePreparePlug checks: "org.*.example" contains a reserved apparmor char`) } From 9578c24b04937effd11b784a81e9be4f2d731959 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Wed, 18 Sep 2024 14:46:18 +0300 Subject: [PATCH 6/9] i/builtin: update error log message Signed-off-by: Zeyad Gouda --- interfaces/builtin/utils.go | 2 +- interfaces/builtin/utils_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/interfaces/builtin/utils.go b/interfaces/builtin/utils.go index 03d8672edbb..88d3230a32e 100644 --- a/interfaces/builtin/utils.go +++ b/interfaces/builtin/utils.go @@ -155,7 +155,7 @@ func getDesktopFileRules(s *snap.Info) string { denyRules := "# Explicitly deny access to other snap's desktop files\n" desktopFiles, err := desktopFilesFromInstalledSnap(s) if err != nil { - logger.Noticef("internal error: failed to collect desktop files from snap %q: %v", s.InstanceName(), err) + logger.Noticef("failed to collect desktop files from snap %q: %v", s.InstanceName(), err) return getDesktopFileRulesFallback() } if len(desktopFiles) == 0 { diff --git a/interfaces/builtin/utils_test.go b/interfaces/builtin/utils_test.go index b6db3772aeb..c67be6b6ac8 100644 --- a/interfaces/builtin/utils_test.go +++ b/interfaces/builtin/utils_test.go @@ -323,7 +323,7 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesSnapMountErrorFallback(c } s.testDesktopFileRules(c, opts) - c.Check(logbuf.String(), testutil.Contains, `internal error: failed to collect desktop files from snap "some-snap": boom`) + c.Check(logbuf.String(), testutil.Contains, `failed to collect desktop files from snap "some-snap": boom`) } func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesAAREExclusionPatternsErrorFallback(c *C) { From e5a4729123a81a14072342d14c9432350af08b70 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Thu, 19 Sep 2024 18:53:43 +0300 Subject: [PATCH 7/9] {i/builtin,wrappers}: adjust comment and logs wording Signed-off-by: Zeyad Gouda --- interfaces/builtin/utils.go | 8 ++++++-- interfaces/builtin/utils_test.go | 4 ++-- wrappers/desktop.go | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/interfaces/builtin/utils.go b/interfaces/builtin/utils.go index 88d3230a32e..026f7668f59 100644 --- a/interfaces/builtin/utils.go +++ b/interfaces/builtin/utils.go @@ -145,7 +145,8 @@ func getDesktopFileRules(s *snap.Info) string { // 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("internal error: invalid desktop file id %q found in snap %q which should have failed in BeforePreparePlug checks: %v", desktopFileID, s.InstanceName(), err) + // Unexpected, should have failed in BeforePreparePlug + logger.Noticef("internal error: invalid desktop file ID %q found in snap %q: %v", desktopFileID, s.InstanceName(), err) return getDesktopFileRulesFallback() } allowRules += fmt.Sprintf("%s/%s r,\n", dirs.SnapDesktopFilesDir, desktopFileID+".desktop") @@ -174,7 +175,10 @@ func getDesktopFileRules(s *snap.Info) string { // - 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("internal error: invalid desktop file name %q found in snap %q which should have been validated earlier: %v", desktopFile, s.InstanceName(), err) + // Unexpected, should have been validated/sanitized earlier in: + // - Desktop interface's BeforePreparePlug for desktop file ids + // - MangleDesktopFileName for prefixed desktop files + logger.Noticef("internal error: invalid desktop file name %q found in snap %q: %v", desktopFile, s.InstanceName(), err) return getDesktopFileRulesFallback() } excludePatterns = append(excludePatterns, "/"+strings.TrimSuffix(filepath.Base(desktopFile), ".desktop")) diff --git a/interfaces/builtin/utils_test.go b/interfaces/builtin/utils_test.go index c67be6b6ac8..663130e5957 100644 --- a/interfaces/builtin/utils_test.go +++ b/interfaces/builtin/utils_test.go @@ -408,7 +408,7 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileName(c *C) } s.testDesktopFileRules(c, opts) - c.Check(logbuf.String(), testutil.Contains, `internal error: invalid desktop file name "foo**?$.desktop" found in snap "some-snap" which should have been validated earlier: "foo**?$.desktop" contains a reserved apparmor char`) + c.Check(logbuf.String(), testutil.Contains, `internal error: invalid desktop file name "foo**?$.desktop" found in snap "some-snap": "foo**?$.desktop" contains a reserved apparmor char`) } func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileIDs(c *C) { @@ -430,5 +430,5 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileIDs(c *C) } s.testDesktopFileRules(c, opts) - c.Check(logbuf.String(), testutil.Contains, `internal error: invalid desktop file id "org.*.example" found in snap "some-snap" which should have failed in BeforePreparePlug checks: "org.*.example" contains a reserved apparmor char`) + c.Check(logbuf.String(), testutil.Contains, `internal error: invalid desktop file ID "org.*.example" found in snap "some-snap": "org.*.example" contains a reserved apparmor char`) } diff --git a/wrappers/desktop.go b/wrappers/desktop.go index d8cf330b815..534ecaff4f2 100644 --- a/wrappers/desktop.go +++ b/wrappers/desktop.go @@ -261,7 +261,7 @@ func deriveDesktopFilesContent(s *snap.Info) (map[string]osutil.FileState, error return nil, err } if _, exists := content[base]; exists { - logger.Noticef("skipping %q as a duplicate file name %q was found after mangling", filepath.Base(df), base) + logger.Noticef("error: identified %q as a duplicate file name %q after mangling in snap %q", filepath.Base(df), base, s.InstanceName()) continue } fileContent, err := os.ReadFile(df) From b1eede9465e4ecbaddc08d6807518fe6e9ca91bb Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Thu, 19 Sep 2024 21:31:46 +0300 Subject: [PATCH 8/9] i/builtin: use string builder for desktop file rules generation Signed-off-by: Zeyad Gouda --- interfaces/builtin/utils.go | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/interfaces/builtin/utils.go b/interfaces/builtin/utils.go index 026f7668f59..c0a5fee8c7b 100644 --- a/interfaces/builtin/utils.go +++ b/interfaces/builtin/utils.go @@ -109,26 +109,18 @@ var desktopFilesFromInstalledSnap = func(s *snap.Info) ([]string, error) { // // The snap must be mounted. func getDesktopFileRules(s *snap.Info) string { - const template = ` -# Support applications which use the unity messaging menu, xdg-mime, etc -# This leaks the names of snaps with desktop files -%s/ r, + var b strings.Builder -# Allow rules: -%s - -# Deny rules: -%s -` + b.WriteString("# Support applications which use the unity messaging menu, xdg-mime, etc\n") + b.WriteString("# This leaks the names of snaps with desktop files\n") + fmt.Fprintf(&b, "%s/ r,\n", dirs.SnapDesktopFilesDir) // Generate allow rules - allowRules := ` -# 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 -` - allowRules += fmt.Sprintf("%s/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,\n", dirs.SnapDesktopFilesDir) + b.WriteString("# Allowing reading only our desktop files (required by (at least) the unity\n") + b.WriteString("# messaging menu).\n") + b.WriteString("# parallel-installs: this leaks read access to desktop files owned by keyed\n") + b.WriteString("# instances of @{SNAP_NAME} to @{SNAP_NAME} snap\n") + fmt.Fprintf(&b, "%s/@{SNAP_INSTANCE_DESKTOP}_*.desktop r,\n", 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 @@ -149,11 +141,11 @@ func getDesktopFileRules(s *snap.Info) string { logger.Noticef("internal error: invalid desktop file ID %q found in snap %q: %v", desktopFileID, s.InstanceName(), err) return getDesktopFileRulesFallback() } - allowRules += fmt.Sprintf("%s/%s r,\n", dirs.SnapDesktopFilesDir, desktopFileID+".desktop") + fmt.Fprintf(&b, "%s/%s r,\n", dirs.SnapDesktopFilesDir, desktopFileID+".desktop") } // Generate deny rules to suppress apparmor warnings - denyRules := "# Explicitly deny access to other snap's desktop files\n" + b.WriteString("# Explicitly deny access to other snap's desktop files\n") desktopFiles, err := desktopFilesFromInstalledSnap(s) if err != nil { logger.Noticef("failed to collect desktop files from snap %q: %v", s.InstanceName(), err) @@ -188,9 +180,9 @@ func getDesktopFileRules(s *snap.Info) string { logger.Noticef("internal error: failed to generate deny rules for snap %q: %v", s.InstanceName(), err) return getDesktopFileRulesFallback() } - denyRules += excludeRules + b.WriteString(excludeRules) - return fmt.Sprintf(template, dirs.SnapDesktopFilesDir, allowRules[1:], denyRules) + return b.String() } // stringListAttribute returns a list of strings for the given attribute key if the attribute exists. From 93d2e69827cd1825dab4e60215458c2a9141edf7 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Mon, 23 Sep 2024 12:39:56 +0300 Subject: [PATCH 9/9] i/builtin: error for invalid desktop file names in rule generation Those errors should never be triggered and indicate that early validation and sanitization was bypassed by the snap so we should fail hard and not do the fallback and prevent the connection. Signed-off-by: Zeyad Gouda --- interfaces/builtin/desktop_legacy.go | 5 ++++- interfaces/builtin/export_test.go | 1 - interfaces/builtin/unity7.go | 5 ++++- interfaces/builtin/utils.go | 18 ++++++++---------- interfaces/builtin/utils_test.go | 21 +++++++++------------ 5 files changed, 25 insertions(+), 25 deletions(-) diff --git a/interfaces/builtin/desktop_legacy.go b/interfaces/builtin/desktop_legacy.go index 3b9900f02b3..90a2eb0e742 100644 --- a/interfaces/builtin/desktop_legacy.go +++ b/interfaces/builtin/desktop_legacy.go @@ -393,7 +393,10 @@ 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 := getDesktopFileRules(plug.Snap()) + desktopSnippet, err := getDesktopFileRules(plug.Snap()) + if err != nil { + return err + } spec.AddPrioritizedSnippet(desktopSnippet, prioritizedSnippetDesktopFileAccess, desktopLegacyAndUnity7Priority) return nil diff --git a/interfaces/builtin/export_test.go b/interfaces/builtin/export_test.go index e129d078014..48256b45b6c 100644 --- a/interfaces/builtin/export_test.go +++ b/interfaces/builtin/export_test.go @@ -36,7 +36,6 @@ var ( ResolveSpecialVariable = resolveSpecialVariable ImplicitSystemPermanentSlot = implicitSystemPermanentSlot ImplicitSystemConnectedSlot = implicitSystemConnectedSlot - GetDesktopFileRules = getDesktopFileRules StringListAttribute = stringListAttribute PolkitPoliciesSupported = polkitPoliciesSupported ) diff --git a/interfaces/builtin/unity7.go b/interfaces/builtin/unity7.go index c621dc895c1..92f2fea6735 100644 --- a/interfaces/builtin/unity7.go +++ b/interfaces/builtin/unity7.go @@ -695,7 +695,10 @@ 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 := getDesktopFileRules(plug.Snap()) + desktopSnippet, err := getDesktopFileRules(plug.Snap()) + if err != nil { + return err + } spec.AddPrioritizedSnippet(desktopSnippet, prioritizedSnippetDesktopFileAccess, desktopLegacyAndUnity7Priority) return nil } diff --git a/interfaces/builtin/utils.go b/interfaces/builtin/utils.go index c0a5fee8c7b..536f6c094ab 100644 --- a/interfaces/builtin/utils.go +++ b/interfaces/builtin/utils.go @@ -108,7 +108,7 @@ var desktopFilesFromInstalledSnap = func(s *snap.Info) ([]string, error) { // 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 { +func getDesktopFileRules(s *snap.Info) (string, error) { var b strings.Builder b.WriteString("# Support applications which use the unity messaging menu, xdg-mime, etc\n") @@ -130,7 +130,7 @@ func getDesktopFileRules(s *snap.Info) string { desktopFileIDs, err := s.DesktopPlugFileIDs() if err != nil { logger.Noticef("cannot list desktop plug file IDs: %v", err) - return getDesktopFileRulesFallback() + return getDesktopFileRulesFallback(), nil } for _, desktopFileID := range desktopFileIDs { // Validate IDs, This check should never be triggered because @@ -138,8 +138,7 @@ func getDesktopFileRules(s *snap.Info) string { // But still it is better to play it safe and check AARE characters anyway. if err := apparmor.ValidateNoAppArmorRegexp(desktopFileID); err != nil { // Unexpected, should have failed in BeforePreparePlug - logger.Noticef("internal error: invalid desktop file ID %q found in snap %q: %v", desktopFileID, s.InstanceName(), err) - return getDesktopFileRulesFallback() + return "", fmt.Errorf("internal error: invalid desktop file ID %q found in snap %q: %v", desktopFileID, s.InstanceName(), err) } fmt.Fprintf(&b, "%s/%s r,\n", dirs.SnapDesktopFilesDir, desktopFileID+".desktop") } @@ -149,11 +148,11 @@ func getDesktopFileRules(s *snap.Info) string { desktopFiles, err := desktopFilesFromInstalledSnap(s) if err != nil { logger.Noticef("failed to collect desktop files from snap %q: %v", s.InstanceName(), err) - return getDesktopFileRulesFallback() + return getDesktopFileRulesFallback(), nil } if len(desktopFiles) == 0 { // Nothing to do - return getDesktopFileRulesFallback() + return getDesktopFileRulesFallback(), nil } excludeOpts := &apparmor.AAREExclusionPatternsOptions{ Prefix: fmt.Sprintf("deny %s", dirs.SnapDesktopFilesDir), @@ -170,19 +169,18 @@ func getDesktopFileRules(s *snap.Info) string { // Unexpected, should have been validated/sanitized earlier in: // - Desktop interface's BeforePreparePlug for desktop file ids // - MangleDesktopFileName for prefixed desktop files - logger.Noticef("internal error: invalid desktop file name %q found in snap %q: %v", desktopFile, s.InstanceName(), err) - return getDesktopFileRulesFallback() + return "", fmt.Errorf("internal error: invalid desktop file name %q found in snap %q: %v", desktopFile, s.InstanceName(), err) } excludePatterns = append(excludePatterns, "/"+strings.TrimSuffix(filepath.Base(desktopFile), ".desktop")) } excludeRules, err := apparmorGenerateAAREExclusionPatterns(excludePatterns, excludeOpts) if err != nil { logger.Noticef("internal error: failed to generate deny rules for snap %q: %v", s.InstanceName(), err) - return getDesktopFileRulesFallback() + return getDesktopFileRulesFallback(), nil } b.WriteString(excludeRules) - return b.String() + return b.String(), nil } // stringListAttribute returns a list of strings for the given attribute key if the attribute exists. diff --git a/interfaces/builtin/utils_test.go b/interfaces/builtin/utils_test.go index 663130e5957..4d347d568b3 100644 --- a/interfaces/builtin/utils_test.go +++ b/interfaces/builtin/utils_test.go @@ -200,6 +200,7 @@ type testDesktopFileRulesOptions struct { desktopFileIDs []string isInstance bool expectedRules []string + expectedErr string } func (s *desktopFileRulesBaseSuite) testDesktopFileRules(c *C, opts testDesktopFileRulesOptions) { @@ -242,6 +243,10 @@ plugs: // connected plugs have a non-nil security snippet for apparmor apparmorSpec := apparmor.NewSpecification(plug.AppSet()) err := apparmorSpec.AddConnectedPlug(iface, plug, slot) + if opts.expectedErr != "" { + c.Assert(err.Error(), Equals, opts.expectedErr) + return + } 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,`) @@ -390,12 +395,9 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesSanitizedDesktopFileName } func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileName(c *C) { - logbuf, restore := logger.MockLogger() - defer restore() - // Stress the case where a snap file name skipped sanitization somehow // This should never happen because snap.MangleDesktopFileName is called - restore = builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { + restore := builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { return []string{"foo**?$.desktop"}, nil }) defer restore() @@ -405,19 +407,15 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileName(c *C) desktopFiles: []string{"foo**?$.desktop"}, isInstance: false, expectedRules: s.fallbackRules, + expectedErr: `internal error: invalid desktop file name "foo**?$.desktop" found in snap "some-snap": "foo**?$.desktop" contains a reserved apparmor char from ` + "?*[]{}^\"\x00", } s.testDesktopFileRules(c, opts) - - c.Check(logbuf.String(), testutil.Contains, `internal error: invalid desktop file name "foo**?$.desktop" found in snap "some-snap": "foo**?$.desktop" contains a reserved apparmor char`) } func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileIDs(c *C) { - logbuf, restore := logger.MockLogger() - defer restore() - // Stress the case where a desktop file ids attribute skipped validation during // installation somehow - restore = builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { + restore := builtin.MockDesktopFilesFromInstalledSnap(func(s *snap.Info) ([]string, error) { return []string{"org.*.example.desktop"}, nil }) defer restore() @@ -427,8 +425,7 @@ func (s *desktopFileRulesBaseSuite) TestDesktopFileRulesBadDesktopFileIDs(c *C) desktopFiles: []string{"org.*.example.desktop"}, desktopFileIDs: []string{"org.*.example"}, expectedRules: s.fallbackRules, + expectedErr: `internal error: invalid desktop file ID "org.*.example" found in snap "some-snap": "org.*.example" contains a reserved apparmor char from ` + "?*[]{}^\"\x00", } s.testDesktopFileRules(c, opts) - - c.Check(logbuf.String(), testutil.Contains, `internal error: invalid desktop file ID "org.*.example" found in snap "some-snap": "org.*.example" contains a reserved apparmor char`) }