Adjust apparmor policy for compatibility with snap-exec #133

Merged
merged 7 commits into from Sep 16, 2016

Conversation

Projects
None yet
3 participants
Collaborator

zyga commented Sep 12, 2016

No description provided.

zyga added some commits Sep 12, 2016

Allow snap-confine to read the auxiliary vector
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Allow for unsafe profile transitions
This patch was proposed by the Ubuntu security team to allow
snap-confine to perform the snap-run -> snap-confine -> snap-exec
transition.

Fixes: https://bugs.launchpad.net/snap-confine/+bug/1621127
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Collaborator

zyga commented Sep 12, 2016

This causes tests to fail. I had a look at the profile but I have no idea why it happens. I'll wait until I can sync with the security team.

@zyga zyga added the Blocked label Sep 12, 2016

Collaborator

zyga commented Sep 12, 2016

@tyhicks could you please look at the diff and offer some insight into why this might affect tests the way it does?

src/snap-confine.apparmor.in
@@ -21,6 +21,8 @@
/usr/lib/@{multiarch}/libseccomp.so* mr,
/lib/@{multiarch}/libseccomp.so* mr,
+ @{PROC}/@{pid}/auxv r,
+
@jdstrand

jdstrand Sep 12, 2016

Contributor

I thought we said this was not needed. IIRC, the debdiff didn't include it. It's fine if it is needed, I'd just like confirmation that it is.

@zyga

zyga Sep 12, 2016

Collaborator

The debdiff I initially pasted did include this. If it is not required I will gladly remove it from this branch.

@jdstrand

jdstrand Sep 12, 2016

Contributor

1.0.38-0ubuntu0.16.04.10+ppa2 in debian/usr.lib.snapd.snap-confine does not have this rule.

@zyga

zyga Sep 12, 2016

Collaborator

I believe this was made by mvo later. I any case, I'm proposing this because I recall you asked for a review on the eventual upstream inclusion.

@jdstrand

jdstrand Sep 12, 2016

Contributor

The updated rules should absolutely be in upstream and thank you for proposing them! My only question is about the particular auxv rule. I also don't see it in 1.0.38-0ubuntu0.16.04.10 from xenial-proposed. Can you compare this PR with the one that you said was uploaded? At this point I checked the image ppa, xenial-proposed and an image mvo gave me that has the unsafe rules and none of them have the auxv rule (which, again, is consistent with what I thought would be uploaded).

src/snap-confine.apparmor.in
@@ -56,23 +58,24 @@
# don't allow changing profile to unconfined or profiles that start with
# '/'
@jdstrand

jdstrand Sep 12, 2016

Contributor

Can you adjust this comment to be:

# Don't allow changing profile to unconfined or profiles that start with
# '/'. Use 'unsafe' to support snap-exec on armhf and its reliance on
# the environment for determining the capabilities of the architecture.
# 'unsafe' is ok here because the kernel will have already cleared the
# environment as part of launching snap-confine with
# CAP_SYS_ADMIN.
@zyga

zyga Sep 14, 2016

Collaborator

Done

Collaborator

tyhicks commented Sep 12, 2016

@zyga the mount-profiles-bin-snap-{destination,source} test failures are not related to his PR. There are no rules in the profile that allow bind mounts from /snap/snapd-hacker-toolbelt/*/mnt/ to /snap/bin/ or the other way around. I didn't go back through the snap-confine tree to see what has changed to cause this but it I am confident because I can undo the changes made in this PR, reload the profile, and then use sudo aa-exec -p /usr/local/libexec/snap-confine -- mount -o rw,bind /snap/snapd-hacker-toolbelt/10/mnt /snap/bin to see that the mounts are denied without the "unsafe" changes.

I'm also doubting that the lp-1599891 failure is caused by these profile changes. It is simply checking to see if the profile is loaded which is the case since AppArmor is denying things in the mount-profiles-bin-snap-{destination,source} tests.

Collaborator

zyga commented Sep 12, 2016

If you run spread test on master they still pass. I agree with your analysis except that we must be missing smoothing. I will look at this with fresh eyes tomorrow.

@zyga zyga removed the Blocked label Sep 14, 2016

Adjust comment as requested by jdstrand
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Collaborator

zyga commented Sep 14, 2016

This now passes in qemu for me. Let's see if linode is happy as well.

Collaborator

zyga commented Sep 14, 2016

So for whatever reason this still fails in Linode. Ideas welcome :/

Collaborator

zyga commented Sep 16, 2016

This doesn't fail in emu for me, There's one failing test in linode: linode:ubuntu-16.04-64-grub:spread-tests/main/cgroup-used

  • snapd-hacker-toolbelt.busybox echo 'Hello World'
    cannot change apparmor hat of the support process for mount namespace capture. errmsg: Operation not permitted
    support process for mount namespace capture exited abnormally

I don't know what the apparmor denial is though. I will investigate this but I suspect it is caused by out-of-date apparmor that is used there (my machine was built just a few days ago)

@zyga zyga merged commit f927a04 into master Sep 16, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@zyga zyga deleted the new-snap-exec-policy branch Sep 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment