interfaces/many: deny arbitrary desktop files and misc from /usr/share #8301
Conversation
Currently, the desktop interface allows access to all of /usr/share/applications. The files in /usr/share/applications come from the core snaps, and with the exception of files related to xdg-open, snaps are unable to do anything useful with these files (since they are unable to execute the application). As such, limit the access to: /usr/share/applications/ r, /usr/share/applications/mimeapps.list r, /usr/share/applications/xdg-open.desktop r, and suppress noisy denials by adding explicit deny rules for the desktop files known to be shipped in the core, core18 and core20 snaps. References: https://bugs.launchpad.net/snapd/+bug/1868051 (related)
Currently, the unity7 interface allows access to all of
/usr/share/applications. The files in /usr/share/applications come from
the core snaps, and with the exception of files related to xdg-open,
snaps are unable to do anything useful with these files (since they are
unable to execute the application). As such, limit the access to:
/usr/share/applications/ r,
/usr/share/applications/mimeapps.list r,
/usr/share/applications/xdg-open.desktop r,
and suppress noisy denials by adding explicit deny rules for the desktop
files known to be shipped in the core, core18 and core20 snaps.
In addition, to avoid confusion, remove legacy rules inherited from
snappy 15.04 that do not exist in the core, core18 and core20 snaps:
/usr/share/gnome/applications/
/usr/share/applications/mimeinfo.cache
/usr/share/unity/icons/**
/usr/share/thumbnailer/icons/**
/usr/share/themes/**
/usr/share/gnome/glib*/schemas/{,*}
/usr/share/ubuntu/glib*/schemas/{,*}
References:
https://bugs.launchpad.net/snapd/+bug/1868051 (related)
Currently, for historical reasons the browser-support interface allows access to /usr/share/applications and /var/lib/snapd/desktop/applications when allow-sandbox: true. With the exception of files related to xdg-open from the core snaps for /usr/share/applications and the snap's own desktop file in /var/lib/snapd/desktop/applications, browser snaps are unable to do anything useful with these files. This has led to situations where browsers are suggesting things they shouldn't and broken GLib portals support. While the desktop interface has been updated to limit access to /usr/share/applications and all applications that plugs browser-support should typically also plugs 'desktop', there is an existing snap that plugs browser-support with allow-sandbox: true without 'desktop' or 'unity7', so we duplicate those accesses here. While the desktop-legacy interface allows access to the directory read of /var/lib/snapd/desktop/applications and the snap's own desktop file in this directory, desktop-legacy is a transitional interface that shouldn't be required for snaps plugging browser-support, so those rules are duplicated here. References: https://bugs.launchpad.net/snapd/+bug/1868051
|
Marking as blocked until @jhenstridge (and optionally @kenvandine) can comment. |
Codecov Report
@@ Coverage Diff @@
## master #8301 +/- ##
==========================================
- Coverage 80.68% 80.66% -0.03%
==========================================
Files 723 723
Lines 57550 57602 +52
==========================================
+ Hits 46435 46464 +29
- Misses 7493 7508 +15
- Partials 3622 3630 +8
Continue to review full report at Codecov.
|
|
In general this looks good, but I think it should be safe to deny access to If you're worried about the number of snaps we'd have to test, perhaps just deny access in the browser-support/allow-sandbox case for now, and we can work from there. |
Reading /var/lib/snapd/desktop/applications is required by at least unity messaging and xdg-mime. Update the comments for desktop, desktop-legacy and browser-support to reflect this (and no longer reference BAMF_DESKTOP_FILE_HINT since that comment was wrong). Explicitly deny /var/lib/snapd/desktop/applications/mimeinfo.cache since snaps are unable to use the data in this file (since they can't execute the returned desktop file themselves). unity messaging menu doesn't require mimeinfo.cache and xdg-mime will fallback to reading the desktop files directly to look for MimeType. Since reading the snap's own desktop files is allowed, we can safely deny access to mimeinfo.cache (eg, xdg-mime will either return one of the snap's mimetypes, or none).
Ok, yes, thank you for mentioning BAMF_DESKTOP_FILE_HINT. I did some digging and figured out that the reason why this is actually needed is to support the unity messaging menu (confirmed via the hello-unity snap) and also xdg-mime (I dug up that the denial that initially prompted the addition of these rules was the discord snap calling xdg-mime). I just pushed a commit that adjusts the comment to mention unity messaging and xdg-mime. I confirmed that neither need access to mimeinfo.cache so I created a deny rule for it. I also confirmed that @{SNAP_INSTANCE_NAME}_*.desktop is required for unity messaging so I'm leaving that in browser-support so as not to break snaps that use allow-sandbox: true without these desktop interfaces (there are production snaps in the store that do this) and for those that choose to use the messaging menu (eg, thunderbird would require allow-sandbox: true and could use this feature). @jhenstridge - can you take another look? I believe this is still compatible with fixing portals since an application that specifies MimeType in its own desktop file is likely to just act on that mime type and not reach out to portals. Eg, firefox isn't going to ask portals to find an application to act on text/html since it will do it itself (and if it does, that is a corner case?). If this is not the case, we'll have to discuss how to handle this situation since we cannot remove the rules without regressing applications (also note, removing from browser-support only doesn't really help since most of the browsers are also plugging desktop-legacy and/or unity7 (confirmed be looking at brave, chromium, firefox, opera{-beta,-developer}, thunderbird, and webbrowser-app |
Actually, thinking through this more, browser-support can remove the access after all without regression because an application that uses the unity messaging menu necessarily must also plugs one of desktop-legacy or unity7 for the dbus accesses. I'll remove this now, but note that it doesn't change the fact that the browsers that are going to use portals are already plugging desktop-legacy and/or unity7 and getting the accesses that way. |
In thinking through why /var/lib/snapd/desktop/applications is needed (for not regressing unity messaging menu), the snap necessarily must plugs desktop-legacy and/or unity7 in order to have the dbus accesses. Snaps that currently plugs browser-support with allow-sandbox: true cannot regress this functionality because they already do not have it. As such, remove the accesses to /var/lib/snapd/desktop/applications.
|
@jdstrand can't the follow-up be done as part of those changes? As I understand this alone will result in dozens of of denials every time app is launched. |
I had missed the messaging menu's separate client library. I had thought libunity was the client library for all these APIs, but that's obviously not the case. The code in libmessaging-menu is a bit weird: it creates a For the Discord snap, the use of the https://github.com/snapcrafters/discord/blob/master/snap/snapcraft.yaml#L49 So rather than calling https://bugs.launchpad.net/snapd/+bug/1863625 So it isn't clear that the Discord snap is a good argument for providing this access: if anything, providing access masks the fact that the snap is not functioning correctly.
I've been testing Firefox's portal support, and it will use its own internal handlers before calling out to the glib APIs that check desktop files. So xdg-open.desktop claiming the http and https schemes isn't a problem. |
|
Thanks @jhenstridge. Marking this as unblocked now. Note to other reviewers: the xdg-mime and unity messaging changes could happen in a follow-up PR, however, as @Erick555 mentioned, we should probably either in this PR or a followup (but both for 2.45) implement option 'E' from https://bugs.launchpad.net/snapd/+bug/1868051/comments/1 (which is this PR plus allowing reading the snap's desktop files with explicit denies for everything else), which happens to address the xdg-mime comment and @Erick555's concern. I will work on that. If it is done before others review this PR, I will add it to this one, else, create another. |
…sktop-files-and-misc-from-usr-share
getDesktopFileRules(<snap instance name>) 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 log noise. Reference: https://launchpad.net/bugs/1868051
desktop-legacy allowed reads on /var/lib/snapd/desktop/applications/ and
/var/lib/snapd/desktop/applications/@{SNAP_INSTANCE_NAME}_*.desktop.
Now, in addition to those accesses, explicitly deny access to other
snap's desktop files.
unity7 allowed reads on /var/lib/snapd/desktop/applications/ and
/var/lib/snapd/desktop/applications/@{SNAP_INSTANCE_NAME}_*.desktop.
Now, in addition to those accesses, explicitly deny access to other
snap's desktop files.
|
Ok, everything is implemented. This should fix the browser issue, not regress anything plugging desktop-legacy/unity7 and suppress noisy denials. @mvo5 - this was one of the things that I was hoping to have in 2.45.1 for policy updates. @kenvandine in particular is wanting it. I believe there is very little chance of regression since browsers are currently all plugging desktop-legacy in addition to browser-support with 'allow-sandbox: true', so they'll still have read access to /var/lib/snapd/desktop/applications and their own desktop file(s), but not other snaps (which is what the fix is for). Snaps only plugging desktop-legacy and/or unity7 have the same policy as before, except with some explict deny rules to silence noisy denials. As such, I changed the milestone back to 2.45. Please let me know if this is in error. |
|
I am very keen to see this land. The Firefox snap revision that would benefit from this is already in the candidate channel. In fact I think the Firefox experience slightly regresses with the current snapd. This would be a big improvement. |
|
FYI, the failures were unrelated so I restarted the jobs. It sure would be nice to just re-run the failing group of tests.... |
These days desktop files are written as name+instance_command.desktop instead of name_instance_command.desktop. wrappers/desktop.go was calculating the desktop file correctly, but other parts of the code base are not. By moving to a SnapInfo method, these other parts can migrate with everything using the same code to calculate the desktop file name. References: - canonical#8301 (comment)
These days desktop files are written as name+instance_command.desktop instead of name_instance_command.desktop. wrappers/desktop.go was calculating the desktop file correctly, but other parts of the code base are not. By moving to a SnapInfo method, these other parts can migrate with everything using the same code to calculate the desktop file name. References: - canonical#8301 (comment)
The desktop-legacy and unity7 policy was incorrectly using
@{SNAP_INSTANCE_NAME}_*.desktop which is name_instance_app.desktop, but
instance desktop files use name+instance_app.desktop now. Add a new
template var, @{SNAP_INSTANCE_DESKTOP}, and use it instead.
With standard hello-unity snap, we can see via dbus-monitor that indicator-messages uses for the hello-unity_hello-unity.desktop file: /com/canonical/indicator/messages/hello_unity_hello_unity_desktop and modifying hello-unity to understand hello-unity_foo (with the desktop file written as hello-unity+foo_hello-unity.desktop) it uses: /com/canonical/indicator/messages/hello_unity_foo_hello_unity_desktop Adjust the policy and add tests for this.
|
@jhenstridge and @anonymouse64 - I believe I've addressed all feedback. This PR includes #9167 so you may want to review that one first. Thanks! |
|
FYI, I re-tested this with:
In each I also ran 'snap run --shell ' and verified I could |
…sktop-files-and-misc-from-usr-share
…sktop-files-and-misc-from-usr-share
| @@ -80,22 +78,29 @@ owner @{HOME}/.local/share/fonts/{,**} r, | |||
| /usr/share/icons/*/index.theme rk, | |||
| /usr/share/pixmaps/ r, | |||
| /usr/share/pixmaps/** r, | |||
| /usr/share/unity/icons/** r, | |||
There was a problem hiding this comment.
This access is removed but not added anywhere back AFAICT - is there a regression risk here?
There was a problem hiding this comment.
No. This path does not exist in core images (notice is is in /usr, which comes from core* snaps). This is explained in commit 105b47f:
interfaces/unity7: limit access to /usr/share
Currently, the unity7 interface allows access to all of
/usr/share/applications. The files in /usr/share/applications come from
the core snaps, and with the exception of files related to xdg-open,
snaps are unable to do anything useful with these files (since they are
unable to execute the application). As such, limit the access to:
/usr/share/applications/ r,
/usr/share/applications/mimeapps.list r,
/usr/share/applications/xdg-open.desktop r,
and suppress noisy denials by adding explicit deny rules for the desktop
files known to be shipped in the core, core18 and core20 snaps.
In addition, to avoid confusion, remove legacy rules inherited from
snappy 15.04 that do not exist in the core, core18 and core20 snaps:
/usr/share/gnome/applications/
/usr/share/applications/mimeinfo.cache
/usr/share/unity/icons/**
/usr/share/thumbnailer/icons/**
/usr/share/themes/**
/usr/share/gnome/glib*/schemas/{,*}
/usr/share/ubuntu/glib*/schemas/{,*}
References:
https://bugs.launchpad.net/snapd/+bug/1868051 (related)
…-arbitrary-desktop-files-and-misc-from-usr-share
|
@mvo5 - the spread tests are failing due to this unrelated failure: This is ready for review. |
These days desktop files are written as name+instance_command.desktop instead of name_instance_command.desktop. wrappers/desktop.go was calculating the desktop file correctly, but other parts of the code base are not. By moving to a SnapInfo method, these other parts can migrate with everything using the same code to calculate the desktop file name. References: - canonical#8301 (comment)
This implements option 'E' from
https://bugs.launchpad.net/snapd/+bug/1868051/comments/1.
Please see individual commit comments for more details.