i/builtin: support desktop-file-ids in desktop files rule generation #14444
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,9 +24,12 @@ | |
| "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,108 @@ | |
| 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, error) { | ||
| var b strings.Builder | ||
|
|
||
| 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 | ||
| 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 | ||
| // 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(), nil | ||
| } | ||
| 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 { | ||
| // Unexpected, should have failed in BeforePreparePlug | ||
| 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") | ||
| } | ||
|
|
||
| // Generate deny rules to suppress apparmor warnings | ||
| 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) | ||
| return getDesktopFileRulesFallback(), nil | ||
| } | ||
| if len(desktopFiles) == 0 { | ||
| // Nothing to do | ||
| return getDesktopFileRulesFallback(), nil | ||
| } | ||
| 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 |
||
| // Unexpected, should have been validated/sanitized earlier in: | ||
| // - Desktop interface's BeforePreparePlug for desktop file ids | ||
| // - MangleDesktopFileName for prefixed desktop files | ||
| 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(), nil | ||
| } | ||
| b.WriteString(excludeRules) | ||
|
|
||
| return b.String(), nil | ||
| } | ||
|
|
||
| // stringListAttribute returns a list of strings for the given attribute key if the attribute exists. | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
wonder if this should perhaps be done in snap.Validate()
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do validation in the desktop interface's
BeforePreparePlugsnapd/interfaces/builtin/desktop.go
Lines 623 to 633 in b605298
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer this is done in
snap.Validate()since any existing snaps should be updated. However, if we are concerned about breaking things too much, we should ensure that review-tools implements similar validation logic fordesktop-file-idsso we can avoid snaps being uploaded with anything that might try and abuse this (also recall this needs a snap declaration as well which adds additional protection too).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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got confused a bit, I don't think we can this check to
snap.Validate(). As Samuele mentioned the bad plug will not be there to validate since the snap plugs and slots are sanitized before getting validated so when we reachsnap.Validatea baddesktop-file-idsattribute won't be there.snapd/snap/info_snap_yaml.go
Lines 241 to 242 in 848d437
I don't see a clean way of force validating
desktop-file-idsbecause they are sanitized (and disappear if bad) just before validation. Only thing left of them is an error message underinfo.BadInterfacesthat is shown when runningsnap pack --check-skeleton.Maybe I am confused about what is required here, but I think adding validation on the container level
snap.validateContainer()for desktop file names on pack/build as @bboozzoo suggested #14444 (comment) is a great idea + checks from review tools to prevent new revision with weird desktop file names.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.
So it looks like an error here is unexpected and we should return early rather than simply ignore it.
snap pack --check-skeletonraises an error with bad plugs/slots, but actual pack does not, which I think was intentional.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.
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.