wrappers: allow snaps to install icon theme icons #6767

jhenstridge · 2019-04-24T04:09:02Z

This branch adds support for snaps to install icon theme icons, as described in the following forum post:

https://forum.snapcraft.io/t/proposal-support-the-icon-theme-spec-for-desktop-icons/10676?u=jamesh

In short, it will copy files from meta/gui/icons to corresponding locations in /var/lib/snapd/desktop/icons, and remove them again when the snap is removed. At the moment, the only files considered are:

files that match the glob snap.$SNAP_NAME.*
no directory components match snap.* (since that could match the file name glob).
the file extension must be .png or .svg.

Some missing features include:

Support for icons without the snap. prefix. I don't think this belongs in the initial implementation, since it opens up namespacing issues.
investigate generating icon-theme.cache files with gtk-update-icon-cache. Need to check whether GTK is actually looking for it first with supplemental icon directories.
No validation of icon files (although there is currently no validation for other places snaps currently provide icons).

chipaca · 2019-04-24T08:02:28Z

I'm not too happy about copying binary blobs out of a snap and into a place where unconfined apps will read them. There have been in the past a whole family of attacks around crafting malicious images. What makes this safe from that kind of attack?
Cc @jdstrand

zyga · 2019-04-24T09:36:32Z

Formatting wrong in following files:
/home/travis/gopath/src/github.com/snapcore/snapd/osutil/synctree.go

zyga · 2019-04-24T09:37:58Z

@chipaca when I was considering how to allow snaps to ship wallpapers that would be indeed copied out of the snap (at least partially) I was thinking about defining a content-filter that would be coming from a snap, running confined, and processing a file to the point where the output was considered sanitised. The interface policy would constrain access to ship content filters and for manual pages and various image types the filter would simply re-format and re-compress them.

jhenstridge · 2019-04-24T11:25:18Z

@chipaca: I considered that, but we are already doing that (minus the copying). When installing .desktop files for a snap, the Icon line will be munged to point at an image file distributed by the snap. This would seem to have exactly the same security concerns as copying image files to a shared location.

I'm not really sure what the best validation strategy would be, since there isn't necessarily a single canonical form for PNGs or SVGs. We could have snapcraft run a particular version of optipng with a particular zlib on icons, and then have review-tools do the same to verify that the output is the same. For SVG files, checking that they are valid XML, and don't include any unexpected elements might be enough.

@zyga: I'm looking into that. Locally go fmt seems happy with that file, so maybe there has been some changes to the algorithm?

jhenstridge · 2019-05-14T09:09:14Z

All tests are passing now, with both unit test and spread test coverage. What's supported right now:

copy icons from $SNAP/meta/gui/icons to /var/lib/snapd/desktop/icons on package install/upgrade, provided the icon name is correctly namespaced.
Remove icons plus empty directories in /var/lib/snapd/desktop/icons on package removal.
Rewrite icon file names when in parallel install mode.

What's not supported:

validation of image file content. I'm not sure whether this is something to do in snapd or on the store side, and it doesn't seem to have been a concern for existing images provided by snaps (the snap icon itself, and icons referenced by desktop files).
rewriting of Icon line of .desktop files when parallel installed snap uses an icon theme icon name.

chipaca · 2019-05-15T18:31:29Z

FWIW, WRT validation, you're right that if it is a concern it needs addressing separately, and this work is orthogonal to that.

jdstrand · 2019-05-23T15:08:42Z

I'm not too happy about copying binary blobs out of a snap and into a place where unconfined apps will read them. There have been in the past a whole family of attacks around crafting malicious images. What makes this safe from that kind of attack?
Cc @jdstrand

@jhenstridge is correct, we already have this problem now with desktop files referencing icons that are typically snap provided. When we first introduced the desktop file feature we discussed this was suboptimal with little we could do within the context of snapd itself but that it was conceivable that external scans/checks could be performed (in the vein of what has been suggested here; note man pages are in a different category because man historically might be installed setuid/setgid and there was a tractable path forward for keeping the system safe, unlike with images (no setuid and no tractable path forward)). Distros would simply patch the bugs in the affected image libraries (like they would anyway). This PR expands the use of images to beyond what is listed in the desktop file, but these icons will be rendered by the same libraries as the shell where CVEs in these libraries would be fixed with urgency by the distros, regardless of snap-provided content.

Of course, svg files are xml files which may contain javascript/ecmascript, animations and interactive features like mouse events which could be problematic, but librsvg doesn't and won't support this (https://github.com/GNOME/librsvg/blob/master/CONTRIBUTING.md#feature-requests) and since fixing CVE-2013-1881, it is not vulnerable to XXE-style attacks. QtSVG supports more than librsvg, but only "supports the static features of SVG 1.2 Tiny. ECMA scripts and DOM manipulation are currently not supported." (https://doc.qt.io/qt-5/svgrendering.html) and we can expect that future XXE issues in qtsvg/desktop environments would get CVEs and be fixed.

Put another way, I agree with @chipaca and am also not too happy about this, but I wasn't happy with desktop file icons and IME this doesn't appreciably change things (the attack surface is a bit broader for reaching the affected libraries/shell but not in a way that should block this PR).

pedronis · 2019-05-29T12:28:22Z

@jhenstridge I don't have fundamental objections to this.

No rewriting of Icon field in desktop file for parallel installed snaps.

that seems something that needs addressing sooner rather than later?

synctree.go should be split out to a prereq PR I think, and also EnsureTreeState should have a sufficiently extensive doc comment.

jhenstridge · 2019-07-24T08:00:55Z

I've updated the branch now that the EnsureTreeState helper has been merged separately. I've also addressed rewriting of the Icon field in .desktop files to handle parallel install correctly (along with updates to the spread tests to verify this works).

jhenstridge · 2019-07-24T09:28:13Z

Spread test failure is on the tests/main/mount-ns test, which is unrelated to the changes in this branch. I think this is all ready to review.

pedronis

thank you, did a pass, some questions/comments

pedronis · 2019-07-25T11:54:24Z

wrappers/desktop.go

+	// to the instance name.
+	snapIconPrefix := fmt.Sprintf("snap.%s.", s.SnapName())
+	if strings.HasPrefix(icon, snapIconPrefix) {
+		return fmt.Sprintf("Icon=snap.%s.%s", s.InstanceName(), icon[len(snapIconPrefix):]), nil


silly question, what happens if the app refers to the icons from code, or is that not expected? or would it in that case use them from the snap, and not the system?

There are (currently) no interfaces that grant a snap access to /var/lib/snapd/desktop/icons, so the naming should be irrelevant to the snap. As with the exported desktop files, this is for the benefit of the host system.

From within the snap sandbox, the app can access un-munged versions of its own icons in ${SNAP}/meta/gui/icons.

wrappers/icons.go

pedronis · 2019-07-25T12:03:39Z

wrappers/icons.go

+			return nil, err
+		}
+		// rename icons to match snap instance name
+		if strings.HasPrefix(base, snapPrefix) {


shouldn't this if always be true? because of the Match in findIcons ?

Yes. I guess I was thinking about one of the possibilities brought up in the forum thread to allow installing icons matching a well known name. There's nothing else in this branch handling that though, so I've converted this to an error when the prefix doesn't match.

pedronis · 2019-07-25T12:04:50Z

wrappers/icons.go

+			dirContent = make(map[string]*osutil.FileState)
+			content[dir] = dirContent
+		}
+		data, err := ioutil.ReadFile(filepath.Join(rootDir, iconFile))


should we check at least that icons haven't an unreasonable size?

Probably a good idea. I wonder what we should consider unreasonable?

Looking at the icons in /usr/share/icons on my system, the largest ones seem to be Xcursor format files at about 4MB, which seem to be animated mouse cursors. For actual application icons, Handbrake seems to be an outlier with 3.5MB SVG icon (which seems to contain a large embedded base64 encoded PNG file).

Ignoring those outliers, there are a number of PNG and SVG icons between 100KB-140KB, so "reasonable" needs to include those sizes.

If we put a limit (8MB ?) it probably needs to be checked already in snap/validate.go so that is not a surprise only at installation.

Another option is to have Source path field in FileState, if it's set and the source is over some threshold we do a buffered file comparison and copy instead of the whole to memory approach.

Both these seems more follow up material than something to do here though.

Yeah, I'd like to second this request. I think it can merge but with my virtual-seciruty-hat on I'd say this is a way to DOS snapd. Just put up a large enough .png file full of noise and snapd goes down.

CC @jdstrand for possible validation angle at the store.

As a separate idea, we can extend EnsureDirState to support streaming. Instead of handing out bytes we could hand out bytes or file references that it can then efficiently use to avoid holding the entire file in memory.

Yeah, I'd like to second this request. I think it can merge but with my virtual-seciruty-hat on I'd say this is a way to DOS snapd. Just put up a large enough .png file full of noise and snapd goes down.

CC @jdstrand for possible validation angle at the store.

I'm working on this in the review-tools now. Please don't block this PR on that.

zyga

Partial pass, just nitpicks.

overlord/snapstate/backend/link.go

tests/lib/snaps/test-snapd-icon-theme/bin/echo

zyga · 2019-07-25T10:50:47Z

tests/main/parallel-install-snap-icons/task.yaml

@@ -0,0 +1,37 @@
+summary: Parallel installed snaps have non-conflicting icons


This is not related to icons but I think it would be neat if our parallel installed desktop files had a way to name the instance automatically.

zyga · 2019-07-25T10:51:15Z

wrappers/desktop.go

@@ -142,6 +142,30 @@ func rewriteExecLine(s *snap.Info, desktopFile, line string) (string, error) {
 	return "", fmt.Errorf("invalid exec command: %q", cmd)
 }

+func rewriteIconLine(s *snap.Info, line string) (string, error) {
+	icon := strings.SplitN(line, "=", 2)[1]


What if line has no =? Then this will panic.

The calling code reads as:

if bytes.HasPrefix(bline, []byte("Icon=")) { line, err := rewriteIconLine(s, string(bline))

So there will always be an equals sign. I followed the pattern of rewriteExecLine here.

This is something for a follow-up, CC @mvo5 but I'd like to refactor it so that the caller splits and passes key, value arguments so that the code is both correct and obviously correct.

…Info

jdstrand

In general I'm happy with the PR wrt code and tests. I'm not thrilled about copying these blobs onto the system where the Desktop entry can refer to them and various shells parse them, but it isn't any different from what we are doing today where a single desktop file can reference a specific snap-provided icon.

I've requested an additional check for absolute paths for this PR (even though I understand the issue I brought up existed previously). Since you are making that change, please add a comment over AddSnapIcons() that references #6767 (comment) (or summarizes the thought process).

jdstrand · 2019-08-13T19:31:25Z

wrappers/desktop.go

+	// If there is a path separator, assume the icon is a path name
+	if strings.ContainsRune(icon, filepath.Separator) {
+		return line, nil
+	}


This means that the desktop file can specify Icon=/path/to/anything. This seems overly broad; why wouldn't we limit this in some manner?

I just checked and it is snapcraft that is doing the munging of the Icon file, not snapd. Eg, I used Icon=/etc/passwd, installed the snap and then found it was passed all the way through to /var/lib/snapd/desktop/applications/*.desktop after install. I'll add a review-tools test to make sure it is compliant with this PR, but it seems snapd could do more here. I suspect before the variable expansion if it starts with '/' or if the normalized path is different from the specified path, then failing would be sufficient.

I've added some additional checks to RewriteIconLine that should cover this:

ensure that the icon begins with ${SNAP}/

ensure that filepath.Clean(icon) == icon

The second part catches escapes like ${SNAP}/../other/icon.png. It won't cover the case of symlinks within the snap that point outside. Presumably that's something the review tools already check for?

And just so there's no confusion: errors in this function are logged and cause the Icon= line to be omitted from the sanitised desktop file. The change @jdstrand requested will not prevent the install or upgrade of a snap.

I've added some additional checks to RewriteIconLine that should cover this:

1. ensure that the icon begins with `${SNAP}/` 2. ensure that `filepath.Clean(icon) == icon`

The second part catches escapes like ${SNAP}/../other/icon.png. It won't cover the case of symlinks within the snap that point outside. Presumably that's something the review tools already check for?

Thanks! I've adjusted the review-tools to be inline with this PR and so if '/' is in the filename. As such, if the Icon contains '/', it must start with ${SNAP}/ (and end with .png and .svg and the path is the same as the normalized path, but that isn't relevant for what I'm describing), so the existing review-tools check for external symlinks would catch this.

jdstrand

Wrt the direction, handling and checks, LGTM, thanks!

pedronis

thank you, it would be good to address the issue about big icons in a follow up

zyga · 2019-08-23T09:46:45Z

I'll do a pass now, please don't merge meanwhile.

zyga

I did a complete pass now.

I left a few comments. Let's discuss if you want to land this like this or we should do a follow-up pass for some of those. Ideally please also merge master to see if this passes before landing.

dirs/dirs.go

overlord/snapstate/backend/link.go

tests/main/parallel-install-snap-icons/task.yaml

zyga · 2019-09-04T09:45:04Z

wrappers/desktop.go

@@ -142,6 +142,30 @@ func rewriteExecLine(s *snap.Info, desktopFile, line string) (string, error) {
 	return "", fmt.Errorf("invalid exec command: %q", cmd)
 }

+func rewriteIconLine(s *snap.Info, line string) (string, error) {
+	icon := strings.SplitN(line, "=", 2)[1]


This is something for a follow-up, CC @mvo5 but I'd like to refactor it so that the caller splits and passes key, value arguments so that the code is both correct and obviously correct.

zyga · 2019-09-04T09:46:57Z

wrappers/desktop.go

+			return "", fmt.Errorf("icon path %v is not part of the snap", icon)
+		}
+		if filepath.Clean(icon) != icon {
+			return "", fmt.Errorf("icon path %v is not canonical", icon)


I think it's pretty hard to guess the correct value just from the word canonical, how about we just tell what we mean?

Suggested change

return "", fmt.Errorf("icon path %v is not canonical", icon)

return "", fmt.Errorf("icon path %q is not canonical, expected %q", icon, filepath.Clean(icon))

The main problem here is that filepath.Clean(icon) may not be something that we will accept. For example, the canonical version of ${SNAP}/foo/../icon.pngis acceptable, but the canonical version of${SNAP}/../icon.png` is not. We know the value is wrong, but we don't necessarily know what the right value is.

I've changed it to icon path %q is not canonicalized, which might be easier to search for (e.g. this is the terminology used in the realpath(3) man page).

wrappers/desktop_test.go

zyga · 2019-09-04T09:55:40Z

wrappers/icons.go

+				return err
+			} else if ok {
+				ext := filepath.Ext(base)
+				if ext == ".png" || ext == ".svg" {


Can we put up a documentation page somewhere that explains that only those two extensions are supported.

CC @degville to inspect later on.

Should we also validate that only those two extensions are present in the icon directory?

I guess I was following the lead of the desktop file parsing code in ignoring things that don't pass validation.

As a new feature, I suppose we could be more strict in what we accept. I don't think there is anything currently stopping someone shipping a snap containing a meta/gui/icons directory, but it is unlikely anyone has done so.

Let's just document the properties of the meta/gui/icons directory that we enforce here https://snapcraft.io/docs/snap-format

zyga · 2019-09-04T09:57:03Z

wrappers/icons.go

+			return err
+		}
+		base := filepath.Base(path)
+		if info.IsDir() {


Should this be caught by the validation layer earlier on (at installation time)?

We discussed this locally and agreed to move this to validation layer in the next release. It's not a blocker for this PR.

zyga · 2019-09-04T09:59:19Z

wrappers/icons.go

+			dirContent = make(map[string]*osutil.FileState)
+			content[dir] = dirContent
+		}
+		data, err := ioutil.ReadFile(filepath.Join(rootDir, iconFile))


Yeah, I'd like to second this request. I think it can merge but with my virtual-seciruty-hat on I'd say this is a way to DOS snapd. Just put up a large enough .png file full of noise and snapd goes down.

CC @jdstrand for possible validation angle at the store.

wrappers/icons.go

pedronis · 2019-09-10T07:30:27Z

@jhenstridge could you do a pass of applying comments that seems relevant from @zyga's pass, we would like to merge this? also indeed we should do something about too big icons in a follow up

thank you

wrappers/icons.go

jhenstridge force-pushed the icon-theme-support branch 2 times, most recently from 3d2d0a1 to e5fc0e7 Compare April 30, 2019 11:25

jhenstridge force-pushed the icon-theme-support branch 5 times, most recently from 5b2d8cc to d5133ea Compare May 9, 2019 07:19

jhenstridge force-pushed the icon-theme-support branch 3 times, most recently from f11a6b3 to a25d922 Compare May 14, 2019 06:00

jhenstridge changed the title ~~WIP: wrappers: allow snaps to install icon theme icons~~ wrappers: allow snaps to install icon theme icons May 14, 2019

pedronis requested review from pedronis and jdstrand May 20, 2019 14:20

pedronis removed their request for review May 29, 2019 12:28

jhenstridge mentioned this pull request Jun 6, 2019

osutil: add EnsureTreeState helper #6959

Merged

jhenstridge force-pushed the icon-theme-support branch 7 times, most recently from 8c66e39 to b065863 Compare June 12, 2019 05:10

wrappers: fix unnecessary assignment in test

4379d61

pedronis self-requested a review July 24, 2019 12:11

pedronis reviewed Jul 25, 2019

View reviewed changes

zyga reviewed Jul 25, 2019

View reviewed changes

jhenstridge added 2 commits July 26, 2019 16:28

wrappers: pass snap name or instance name to helpers instead of snap.…

9d22252

…Info

overlord, tests: changes suggested by @zyga in review

4328fcf

pedronis self-requested a review August 13, 2019 08:32

pedronis added this to the 2.41 milestone Aug 13, 2019

jdstrand suggested changes Aug 13, 2019

View reviewed changes

mvo5 modified the milestones: 2.41, 2.42 Aug 14, 2019

wrappers: perform some sanity checks on icon paths in desktop files

eef96c1

pedronis requested a review from jdstrand August 19, 2019 07:26

jdstrand approved these changes Aug 19, 2019

View reviewed changes

pedronis approved these changes Aug 20, 2019

View reviewed changes

zyga self-requested a review August 23, 2019 09:46

joaomoreno mentioned this pull request Sep 2, 2019

Snap package fails Store validation checks microsoft/vscode#80205

Closed

zyga reviewed Sep 4, 2019

View reviewed changes

ppd mentioned this pull request Sep 10, 2019

Linux: add support for building snap packages solvespace/solvespace#436

Merged

jhenstridge added 2 commits September 10, 2019 22:24

wrappers: apply changes suggested by @zyga

a001514

Merge remote-tracking branch 'upstream/master' into icon-theme-support

6918530

chipaca reviewed Sep 17, 2019

View reviewed changes

wrappers/icons.go Show resolved Hide resolved

wrappers: update error message for non-canonicalized icon path

d22b27a

mvo5 merged commit 97e2fb0 into snapcore:master Sep 17, 2019

jhenstridge mentioned this pull request Jul 1, 2020

interfaces/desktop-launch: support confined snaps launching other snaps #8699

Merged

JGCarroll mentioned this pull request Jan 29, 2021

Themable icon JGCarroll/joplin-desktop-snap#11

Merged

		@@ -0,0 +1,37 @@
		summary: Parallel installed snaps have non-conflicting icons

	return "", fmt.Errorf("icon path %v is not canonical", icon)
	return "", fmt.Errorf("icon path %q is not canonical, expected %q", icon, filepath.Clean(icon))

wrappers: allow snaps to install icon theme icons #6767

wrappers: allow snaps to install icon theme icons #6767

Conversation

jhenstridge commented Apr 24, 2019 • edited

chipaca commented Apr 24, 2019

zyga commented Apr 24, 2019

zyga commented Apr 24, 2019

jhenstridge commented Apr 24, 2019

jhenstridge commented May 14, 2019

chipaca commented May 15, 2019

jdstrand commented May 23, 2019 • edited

pedronis commented May 29, 2019

jhenstridge commented Jul 24, 2019

jhenstridge commented Jul 24, 2019

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand left a comment

Choose a reason for hiding this comment

jdstrand Aug 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

zyga commented Aug 23, 2019

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis commented Sep 10, 2019

jhenstridge commented Apr 24, 2019 •

edited

jdstrand commented May 23, 2019 •

edited

jdstrand Aug 13, 2019 •

edited