cmd/snap-exec: don't fail on some try mode snaps #6048
Conversation
The snap-{run,confine,exec} pipeline culminates with snap-exec reading
the info of the snap that needs to be executed and running the appropriate
command. If the snap in question is a try mode snap in /tmp or other path
that is not accessible from the execution environment the code in ReadInfo
falls over inability to compute the utterly useless Size field.
Perhaps the code in snap-exec should be more optimized to be as minimal
as needed but for now let's introduce a new ReadInfo variant that skips
the size computation.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
A try-mode snap is represented by a bind mount from whatever the original location was, to /snap/$SNAP_NAME/$SNAP_REVISION as well as a symlink from /var/lib/snapd/snap/$SNAP_NAME_$SNAP_REVISION.snap to whatever the original location was. In the execution environment the symlink cannot work, in general. The code loading snap.Info was using the fully generic snap.ReadInfo which, as one of the features, tried to stat and compute the size of the snap. Since reading the size of a snap is not very useful for snap-exec and since it can fail if the symlink is not reachable in the execution environment, switch to the more limited helper. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| mountFile := MountFile(name, si.Revision) | ||
| st, err := os.Stat(mountFile) | ||
| if os.IsNotExist(err) { |
There was a problem hiding this comment.
Am I being silly, or is the only reason this works today because of the bug in apparmor that causes it to leaks stats? Consider: if the snap I'm trying doesn't use the home plug, but the source code for the snap is in ~/src/snaps/, then the symlink that is mountFile is pointing to a location inaccessible by snap-exec. However, because appamor leaks stats, we happen to have the ability to determine that the symlink is pointing to something that exists. Should this be an Lstat to avoid such things?
There was a problem hiding this comment.
Apparmor doesn't check stat, that's correct. As for Lstat - perhaps, cc @chipaca for opinion.
In any case inside the mount namespace we cannot even attempt to follow the symlink as someone may craft it to be malicious.
|
Yeah, that size is wrong for 'try' snaps, info should also not show it. |
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
In snapcore#6048 we added ReadInfoExceptSize(). This PR adds a small comment in the code about why its used instead of the regular ReadInfo().
Snaps installed via "snap try" may have a dangling symlink from
/var/lib/snapd/snaps/snap_rev since the source location is unconstrained. In
the execution environment that path may not exist or may point to unrelated
data, in a malicious case. The snap-exec command used snap.ReadInfo to obtain
information about the application or hook to exec. That function tries to stat
the snap file to compute the size of the snap. Since snap-exec does not need
the size and since this cannot be achieved in the execution environment we
should not attempt that.
This branch adds and uses a new helper that is exactly like ReadInfo but
doesn't compute the size of the snap.
Fixes: https://bugs.launchpad.net/snapd/+bug/1800004
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com