-
Notifications
You must be signed in to change notification settings - Fork 574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
i/builtin: support desktop-file-ids in desktop files rule generation #14444
i/builtin: support desktop-file-ids in desktop files rule generation #14444
Conversation
79dfd14
to
8080492
Compare
8080492
to
ad2bce2
Compare
I just realized that incoming desktop file names must be sanitized before passing to |
ad2bce2
to
ec8468d
Compare
I fixed this in multiple ways, each adding a layer of protection on its own:
|
Signed-off-by: Zeyad Gouda <[email protected]>
ec8468d
to
a315bf8
Compare
interfaces/builtin/utils.go
Outdated
// 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) |
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.
also why not fail and let AppArmorConnectedPlug return an error?
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.
Good point, Since desktop-file-ids is a new concept, I believe we can just fail. but for existing prefixed desktop files we could break some snaps but it would be very rare. A snap with weird desktop file names + unity7/desktop-legacy plugs.
// 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.
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 BeforePreparePlug
snapd/interfaces/builtin/desktop.go
Lines 623 to 633 in b605298
// https://specifications.freedesktop.org/desktop-entry-spec/latest/file-naming.html | |
// Desktop file id must be a valid D-Bus name: | |
// - A sequence of non-empty elements separated by dots | |
// - None of which starts with a digit | |
// - Each of which contains only characters from the set [A-Za-z0-9-_] | |
// | |
// XXX: dashes "-" are not recommended but supported, should they be removed? | |
// https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-names | |
var desktopFileIDRegexp = regexp.MustCompile(`^([A-Za-z_-][\w-]*)(\.[A-Za-z_-][\w-]*)*$`) | |
func (iface *desktopInterface) validateDesktopFileIDs(attribs interfaces.Attrer) error { |
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 for desktop-file-ids
so 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 reach snap.Validate
a bad desktop-file-ids
attribute won't be there.
Lines 241 to 242 in 848d437
snap.BadInterfaces = make(map[string]string) | |
SanitizePlugsSlots(snap) |
I don't see a clean way of force validating desktop-file-ids
because they are sanitized (and disappear if bad) just before validation. Only thing left of them is an error message under info.BadInterfaces
that is shown when running snap pack --check-skeleton
.
$ 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 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-skeleton
raises 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.
// - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. A warning there wouldn't break existing snaps.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed a ticket to track this idea https://warthogs.atlassian.net/browse/SNAPDENG-32333
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 just have one comment, I think the desktop file names check should be on the container level, so I think snap.validateContainer()
would be more appropriate to add the additional scenario parameter.
Signed-off-by: Zeyad Gouda <[email protected]>
5aff77e
to
4c93ce8
Compare
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.
One question. I'll re-review this later today.
@@ -0,0 +1,6 @@ | |||
[Desktop Entry] |
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 wish there was a way to comment on the file name in GitHub. This is such a nice name :)
interfaces/builtin/utils.go
Outdated
} | ||
if len(desktopFiles) == 0 { | ||
// Nothing to do | ||
return getDesktopFileRulesFallback() |
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.
Why do we switch to the fallback in this case?
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.
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
func getDesktopFileRules(snapInstanceName string) []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), | |
"# 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), | |
} | |
for _, t := range aareExclusivePatterns(snapInstanceName) { | |
rules = append(rules, fmt.Sprintf("deny %s/%s r,", baseDir, t)) | |
} | |
return rules | |
} |
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?
Signed-off-by: Zeyad Gouda <[email protected]>
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 did a quick first pass, seems ok. One remark about the already landed helper and one about desktop-ids validation
// 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.
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?
Signed-off-by: Zeyad Gouda <[email protected]>
interfaces/builtin/utils.go
Outdated
// 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) |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, and also updated the tests to check that errors are logged properly
Signed-off-by: Zeyad Gouda <[email protected]>
interfaces/builtin/utils.go
Outdated
@@ -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) |
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'm not sure this one and the one below are internal errors though, do we check desktop file names somewhere else?
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.
Good point, the snap mount could have been unmounted due to external reasoning which would cause failure. Updated.
Regarding the ones below:
internal error: invalid desktop file name %q found in snap %q which should have been validated earlier
- My reasoning here was that if we got so far and also got a bad desktop file name then snapd have been tricked somehow by the snap either when validating desktop file ids or when sanitizing the desktop file names during install/refresh.
internal error: failed to generate deny rules for snap %q: %v
- My reasoning here was that there is a case we didn't account for that breaks deny rules generation
Signed-off-by: Zeyad Gouda <[email protected]>
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.
thanks, lgtm, please iterate on this further with other reviewers as needed
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.
looks quite good
// - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
interfaces/builtin/utils.go
Outdated
// | ||
// The snap must be mounted. | ||
func getDesktopFileRules(s *snap.Info) string { | ||
const template = ` |
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?
diff --git a/interfaces/builtin/utils.go b/interfaces/builtin/utils.go
index 026f7668f5..2475d2f7f1 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")
+ b.WriteString(fmt.Sprintf("%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")
+ b.WriteString(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
@@ -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")
+ b.WriteString(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"
+ 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.
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 appropriate
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.
updated
Signed-off-by: Zeyad Gouda <[email protected]>
Signed-off-by: Zeyad Gouda <[email protected]>
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.
In general this looks quite good - I really like the defence in depth approach and multiple levels of validation. However, I would prefer the validation is done in snap.Validate()
to fail-hard earlier to avoid any chance of possible abuse, but will leave this decision up to @pedronis - from a security point of view, this looks really good.
I have also filed a bug for review-tools as well to make sure we implement similar logic there https://bugs.launchpad.net/review-tools/+bug/2081240.
// 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.
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 for desktop-file-ids
so 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).
// - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This sounds like a great idea.
And we should also extend review-tools to implement similar checks against desktop file names etc.
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 <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14444 +/- ##
==========================================
+ Coverage 78.77% 78.86% +0.09%
==========================================
Files 1073 1079 +6
Lines 143693 145697 +2004
==========================================
+ Hits 113191 114910 +1719
- Misses 23401 23601 +200
- Partials 7101 7186 +85
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
LGTM
Failing tests are known to fail:
|
This PR adds support for
desktop-file-ids
for apparmor rules generated when thedesktop-legacy
andunity7
interfaces are connected because they provide the ability for the snap to access it own desktop files.This adds a bit of complexity due to the deny rules that needs to be generated to suppress warnings.
GenerateAAREExclusionPatterns
is used with care to generate the deny rules for multiple patterns.Now, since the rules generated depend on the snap content itself, more aggressive validation and sanitization is required:
[A-Za-z0-9-_.]
so they can't contain special AARE charactersdesktop-file-ids
as is without sanitization because they should already be validated during installunity7
anddesktop-legacy
) that non of the names used in rule generation contain any special AARE characters.allow
rules doesn't depend on desktop files found in the snap but instead depends on the desktop prefix and desktop file ids.deny
rules depend desktop file names inside the snap mount, so if a snap somehow escaped all this validation, it can only shoot itself in the foot and deny itself more thingsAny single one of the methods used above should have been enough, but since this is generating apparmor rules from user input basically it is better to play it safe here.
Please be as skeptical as possible when reviewing and let me know if you found an edge case that could break the rule generation.