i/builtin: support desktop-file-ids in desktop files rule generation #14444
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,63 +79,114 @@ func verifySlotPathAttribute(slotRef *interfaces.SlotRef, attrs interfaces.Attre | |
| return cleanPath, nil | ||
| } | ||
|
|
||
| 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 | ||
| 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 | ||
| // 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 { | ||
| const template = ` | ||
| # Support applications which use the unity messaging menu, xdg-mime, etc | ||
| # This leaks the names of snaps with desktop files | ||
| %s/ r, | ||
|
|
||
| # Allow rules: | ||
| %s | ||
|
|
||
| # Deny rules: | ||
| %s | ||
| ` | ||
|
|
||
| // 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) | ||
| // 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 fail, but still it is better to play it safe with allow rules. | ||
| desktopFileIDs, err := s.DesktopPlugFileIDs() | ||
| if err != nil { | ||
| logger.Noticef("cannot list desktop plug file IDs: %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 { | ||
|
There was a problem hiding this comment. wonder if this should perhaps be done in snap.Validate() There was a problem hiding this comment. Ideally, Yes. But we don't know if there are snaps already in the store with desktop files with undesired names. adding the check in snap.Validate() could break existing installs (but maybe for such snaps, we should?). There was a problem hiding this comment. don't we do any validation on the ids? I'm sure there are some checks that we can do just based on dbus rules, no? There was a problem hiding this comment. We do validation in the desktop interface's snapd/interfaces/builtin/desktop.go Lines 623 to 633 in b605298 There was a problem hiding this comment. I added the check here to avoid depending on other parts working as expected and having the checks validate input standalone without any assumptions. There was a problem hiding this comment. but BeforePreparePlug is used by Sanitize* code which means if it fails the plug will not even be there, so it's not clear it's a good idea to ignore the error here There was a problem hiding this comment. I would prefer this is done in |
||
| 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) | ||
ZeyadYasser marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return getDesktopFileRulesFallback() | ||
| } | ||
| 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 := desktopFilesFromInstalledSnap(s) | ||
| if err != nil { | ||
| logger.Noticef("failed to collect desktop files from snap %q: %v", s.InstanceName(), err) | ||
| return getDesktopFileRulesFallback() | ||
| } | ||
| if len(desktopFiles) == 0 { | ||
| // Nothing to do | ||
| return getDesktopFileRulesFallback() | ||
|
There was a problem hiding this comment. Why do we switch to the fallback in this case? There was a problem hiding this comment. This is to be consistent with the original implementation, which didn't care about existing desktop files. snapd/interfaces/builtin/utils.go Lines 116 to 136 in f3d0682 I know a snap depending on the undocumented behavior that unity7 and desktop-legacy gives it access to list desktop files should not be supported, but who knows what snap will break if that changes? |
||
| } | ||
| excludeOpts := &apparmor.AAREExclusionPatternsOptions{ | ||
| Prefix: fmt.Sprintf("deny %s", dirs.SnapDesktopFilesDir), | ||
| 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 { | ||
|
There was a problem hiding this comment. snap.Validate() is also used when packing a snap, maybe it'd be useful to issue a warning if the there's anything wrong with the desktop files. Hm I don't recall why we don't do any sanity checks of desktop files in validation code, and rather let them fail during install. There was a problem hiding this comment. Yes, I agree. A warning there wouldn't break existing snaps.
No idea, I recently landed changes there to validate their file types, but there weren't any other validations. I think without epochs we cannot do better on the container validation side. There was a problem hiding this comment. A followup material, but we could look into tweaking snap.Validate() interface to pass additional parameter describing the scenario in which it is invoked, and in case of pack/build-time validation fail hard on some new checks we introduce. There was a problem hiding this comment.
This sounds like a great idea. And we should also extend review-tools to implement similar checks against desktop file names etc. |
||
| logger.Noticef("internal error: invalid desktop file name %q found in snap %q which should have been validated earlier: %v", desktopFile, s.InstanceName(), err) | ||
ZeyadYasser marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return getDesktopFileRulesFallback() | ||
| } | ||
| 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() | ||
| } | ||
| denyRules += excludeRules | ||
|
|
||
| return fmt.Sprintf(template, dirs.SnapDesktopFilesDir, allowRules[1:], denyRules) | ||
| } | ||
|
|
||
| // stringListAttribute returns a list of strings for the given attribute key if the attribute exists. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this could use strings.Builder as well now. Not essential, but give it a try locally, and if the changes look good maybe it's worth pushing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more less, builder implements Writer, so you can use
fmt.Fprintf(&b, ...)where appropriateThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated