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. I would prefer this is done in There was a problem hiding this comment. No snaps on the store currently should have desktop-file-ids yet, I think we could add a check for them in snap.Validate() and fail hard there during install/pack. I was confused at first and thought this was for existing desktop file names as well. There was a problem hiding this comment. I got confused a bit, I don't think we can this check to Lines 241 to 242 in 848d437 I don't see a clean way of force validating $ snap pack --check-skeleton squashfs-root build
snap "hello" has bad plugs or slots: desktop (desktop-file-ids entry "org.he&&llo.Example" is not a valid D-Bus well-known name)Maybe I am confused about what is required here, but I think adding validation on the container level There was a problem hiding this comment. So it looks like an error here is unexpected and we should return early rather than simply ignore it. There was a problem hiding this comment. I updated that part to return and error instead of doing the fallback as this indicates something dangerous that the early validation and sanitization was bypassed by the snap. |
||
| logger.Noticef("invalid desktop file id %q found in %q: %v", desktopFileID, s.InstanceName(), err) | ||
|
There was a problem hiding this comment. I think my point is if we prefer not to error here ok, but this message should make clear that this situation is an internal error, as the desktop-ids shouldn't have made this far There was a problem hiding this comment. Updated, and also updated the tests to check that errors are logged properly |
||
| 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("error: %v", err) | ||
ZeyadYasser marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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. There was a problem hiding this comment. Filed a ticket to track this idea https://warthogs.atlassian.net/browse/SNAPDENG-32333 There was a problem hiding this comment. I just have one comment, I think the desktop file names check should be on the container level, so I think |
||
| 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")) | ||
| } | ||
| excludeRules, err := apparmorGenerateAAREExclusionPatterns(excludePatterns, excludeOpts) | ||
| if err != nil { | ||
| logger.Noticef("error: %v", 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