Incorrect permissions in meta/snap.yaml

popey 2017-06-27 11:01:14 UTC #1

Just briefly discussed this with @mvo and @kyrofa who suggested a forum post.

I exported an application out of a 3rd party tool, in order to snap it. The exported directory looks like this:-

drwx------  4 alan alan      4096 Jun 27 11:08 linux64

I created a simple yaml which ingests this and makes a strict snap, which installs correctly. However I can’t run it:-

internal error, please report: running "c3game" failed: open /snap/c3game/x1/meta/snap.yaml: permission denied

Which looks like it’s due to the root fs being 700 rather than the more usual 755.

alan@gort:~/Development/Snappy/construct3template$ unsquashfs -ll c3game_0.1_amd64.snap
Parallel unsquashfs: Using 4 processors
21792 inodes (25320 blocks) to write

drwx------ root/root               400 2017-06-27 11:22 squashfs-root

Should snapd allow me to install this, knowing it’s un-runnable?
Should snapcraft allow me to make this snap, knowing it’s un-runnable?

kyrofa 2017-06-27 11:32:33 UTC #2

I don’t know about this one. Even though it’s a snap, I feel like standard permissions still apply. Is there no situation where a snap developer wouldn’t WANT to use this to make sure specific directories within the snap were unreadable by non-root-users?

I suppose snapcraft could take a look at the path formed by each app though, and try to ensure they actually CAN execute by a non-root user, and warn if not. That would probably only apply to apps that aren’t daemons, since those run as root.

jdstrand 2017-06-27 14:31:59 UTC #3

First off, the contents of the squashfs are not private because someone can just unsquashfs them and examine them, as such directories that are 0700, etc in $SNAP aren’t really hiding anything.

However, in general the permissions within the snap might be set in particular ways because the software expects them to be set that way. Developers may set permissions in very specific ways and IMO we shouldn’t try to second guess what the developer is doing. This will potentially become even more true when snappy uid/gids are implemented. Indeed, as soon as we implement strict checks there will be bugs filed for snaps that are bumping against our restrictions.

All that said, we can try to look for obviously wrong permissions, such as the thing that command points to is not executable or squashfs-root is too restrictive. The review-tools actually will warn for some of these things already and perhaps it is time to start reconsidering having snapcraft use them after the build (even if in an opt-in manner). There is a review-tools snap in the store that hasn’t been thoroughly vetted for production yet (worked is scheduled but behind other work), but it works quite well in my testing. Getting some new users and bugs against it sooner than later will only help get it into shape faster.

mvo 2017-07-04 08:21:42 UTC #4

I agree that snapcraft should not do anything to change the permissions but it seems prudent to warn in e.g. snapcraft snap or via the review-tools if the root dir has no read permission for others.

niemeyer 2017-07-04 19:40:47 UTC #5

I’m curious about how this snap was cooked. That directory is supposed to be put in place by snapcraft itself before squashing the snap, so how come it ended up with the wrong permissions? We can block this in the store, and snapd should have a better error message, but the real problem here is during the build process.

@sergiusens Do you have any ideas here?

sergiusens 2017-07-04 19:49:27 UTC #6

@popey can I see the snapcraft.yaml? Is this uploaded in some repo? This may be related -> https://bugs.launchpad.net/snapcraft/+bug/1515394

We had old PRs (or MPs even) attempting to fix that particular umask bug once but we haven’t found a happy solution. Happy to discuss that here again.

popey 2017-07-12 14:51:54 UTC #7

@sergiusens I’ve put the whole thing on a junk branch here:- https://code.launchpad.net/~popey/+junk/c3template
The contents of the linux64 directory came from a 3rd party game development tool called Construct3. http://construct3.net/
I chmodded the directory which makes it build, install and run (but crash due to some permission missing).

Week 28 of 2017 in snapcraft

chipaca 2017-12-08 09:50:48 UTC #8

I think snapd should check, as part of its validation phase, that the apps listed pass a minimal sanity check:

/ and /meta/ should be searchable, and everything under /meta should be world-readable.
for every hook in the snap, the hook should be executable. Hooks are run as root, so any bit is enough (and the path needn’t be searchable).
for every app in the snap and the app should be readable and executable; for commands (but not necessarily for services) the path should be searchable. For now, the bit on apps needs to be there for owner, group and ‘other’. We can relax this later on if needed. We should check published snaps though, and I’d be happy with something less restrictive if it’s found this is too onerous for existing snaps.

EDIT: to add: snapd in particular cannot, reasonably, check whether the apps are actually executable. If the libraries it depends on are inside a 0700 directory, for example. Snapcraft should check this, and probably the review tools also if possible?