Update apparmor profile for snap-confine

[zyga]

During the recent move of snap-confine of $libexecdir from /usr/lib to
/usr/lib/snapd the apparmor profile effectively did nothing. This patch
corrects the path and adds rules required for additonal functionality added
since the rename.

Thanks to Jamie Strandboge for identifying the problem.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

[mvo5]

Will an attacker be able to mount parts of the classic system RW with that? Which makes most of the rest of the confinement uneffective(?)

[zyga]

The idea is that he or she won't be because this happens after pivot_root and after unshare(CLONE_NEWNS) and all the bind mounts earlier are setup in a way that makes changes here not propagate outside.

Still I think we should let the security team comment on this.

[jdstrand]

First, this rule could be rewritten as: mount options=(rw bind),

Second, I don't like this rule. Because snap-confine is setuid we are trying to be fanatical about it's profile which is why we don't, for example, even use the base abstraction. I prefer updating the profile with each newly added mount point. Is there a reason why we can't continue to do this? If not, is there a way to limit this to /snap/ubuntu-core/** or something?

[jdstrand]

As for it happening after the pivot_root, that is assuming that the code is correct before the pivot_root and can't be attacked there. The apparmor profile is attempting to limit the functionality to precisely what the launcher needs, no more, no less. The profile already grants a lot because of the nature of the launcher, but a carte blanche rule like this will provide easier privilege escalation. For example, because of the existing profile, the recent execl bug was considerably harder to expoit for privilege escalation. We want to make sure we are continuing to do this.

[zyga]

I don't think we can confine it any further because both the source and the destination come from snaps.

[jdstrand]

The comment as it stands is unfortunate. However, I'd like to challenge that assessment. :)

Doesn't snapd control what the mount points are via interfaces? As such, we are still in control of the mountpoints within the snapcore project.

Even if not, aren't the mount for things to be mounted inside the snap? As such, we should be able to limit the target directory at the very least. For example, it would be exceedingly unfortunate if we mounted /var as rw in the snap writable area since then the snap has control to modify /var/lib/snapd/....

With that in mind, I'm now looking at these mount rules and wondering why many of them are 'rw'-- eg, everything under 'when --enable-rootfs-is-core-snap is NOT used' and nvidia.

[jdstrand]

I'd like to see a spread test for this. The existing spread tests will show when the profile needs updating when snap-confine is running under confinement. However, we need to make sure it is running under confinement. I suggest:

1. install hello-world
2. cp /etc/apparmor.d/usr.bin.snap-confine /tmp
3. add 'audit deny file,' to /tmp/usr.bin.snap-confine
4. apparmor_parser -r /tmp/usr.bin.snap-confine
5. if hello-world.echo ; then exit 1 ; fi

restore consists of:

1. apparmor_parser -r /etc/apparmor.d/usr.bin.snap-confine
2. rm -f /tmp/usr.bin.snap-confine

In this manner, we copy the profile to a temporary location, add a rule that is guaranteed to make the launcher fail to launch, then try to run hello-world. If it succeeds we know the binary attachment is wrong.

[zyga]

Yes, a spread test is on my TODO list

[zyga]

I've added a spread test for this, much easier, have a look please.

[jdstrand]

debian/usr.bin.snap-confine doesn't represent the correct path now. This doesn't matter to apparmor but can lead to user confusion. Please rename to usr.lib.snapd.snap-confine. This will require changes to dh_apparmor in debian/rules and debian/snap-confine.install

[zyga]

Ack

(this will require some changes to packaging that is somewhat harder to coordinate)

[zyga]

This is now bug https://bugs.launchpad.net/snap-confine/+bug/1599891

[jdstrand]

This one is not as bad as the rw one, but fine-tuning would apply here as well.

[zyga]

I've simplified the rules, please tell me if there's anything more you want me to do here.

[zyga]

As a small side note, I'd like to move the apparmor profile to src/, have auto tools generate it from an .in file so $libexecdir can be not baked in and finally make it clear that this is something packages should be responsible for handling.

[jdstrand]

This test is fine but and makes sure there is an enforce mode profile but is a bit brittle in the face of renames or snapd/snap-confine/snap-run miscoordinated updates. I'd still prefer we are testing enforcement in the manner I described since it will ensure that snap-confine is running under confinement when invoked from the snapd/snap-run that is installed.

[jdstrand]

With the added /snap/bin test you no longer need to add anything in spread-tests/regression/lp-1599891/task.yaml.

[zyga]

Right, thanks for confirming that. I was about to add something but I realized we're already testing denials

[jdstrand]

CC @tyhicks
I very much do not like these mount rules. The problem is not about who is writing the mount profiles, it is about attacks against snap-confine and privilege escalation. These rules as written provide virtually no protection against privilege escalation if an attacker can gain control of snap-confine. Can you give specific examples why these rules are required and not more-specific rules that are coordinated between snap-confine and snapd?

Also, you didn't address my questions in this comment:
"Doesn't snapd control what the mount points are via interfaces? As such, we are still in control of the mountpoints within the snapcore project.

Even if not, aren't the mount for things to be mounted inside the snap? As such, we should be able to limit the target directory at the very least. For example, it would be exceedingly unfortunate if we mounted /var as rw in the snap writable area since then the snap has control to modify /var/lib/snapd/....

With that in mind, I'm now looking at these mount rules and wondering why many of them are 'rw'-- eg, everything under 'when --enable-rootfs-is-core-snap is NOT used' and nvidia."

[zyga]

We control mount points via interfaces but we allow any values to be used AFAIR

I'd like to make --enable-rootfs-is-core-snap used everywhere with this upload. The other option should be entirely removed as it just adds code that isn't used or tested.

As for mounting things on /var, isn't that something that will only be visible to the process? The outside world won't see the /var bind mount, will it?

Perhaps we should write a few spread tests, if you give me a few things that you'd like to ensure are working correctly I can make that happen.

[tyhicks]

I agree with @jdstrand that the mount rules are way too permissive. In addition, they make most of the other mount rules useless because those two permissive mount rules are a superset of the other mount rules.

I'm confused by "We control mount points via interfaces but we allow any values to be used AFAIR". As the maintainers of interfaces in snappy, aren't we in control of what values are used for the source and destination mount points? If so, we can add specific mount rules, as needed, instead of granting permission to bind mount anything to anywhere.

[jdstrand]

@zyga - the /var example is an example of an attack. The rules you've written allow snap-confine to mount pretty much anything. An attacker who can subvert snap-confine due to some undiscovered bug is then in a position to make snap-confine mount anything into anywhere and this can be used to escalate privileges among other things.

[zyga]

FYI:

20:36 < jdstrand> zyga (cc tyhicks): the next step is to give examples of mount profiles that snapd 
                  currently implements
20:36 < zyga> jdstrand: only the content interface uses them
20:36 < zyga> jdstrand: and the read/write paths are decided by plug/slot attributes
20:36 < jdstrand> zyga (cc tyhicks): and then understand precisely what mount rules are needed
20:36 < zyga> jdstrand: any value is allowed right now
20:37 < zyga> jdstrand: that fact implies that any mount rules might be allowed
20:37 < jdstrand> zyga: yes, but the source mount point is inside a snap, not /**
20:37 < jdstrand> zyga: and the target mount point is inside a snap, not /**
20:37 < zyga> jdstrand: one useful use case is /var/lib/snapd/hostfs/usr/share/fonts 
20:37 < zyga> jdstrand: as for the target directory, let me check
20:38 < jdstrand> zyga: but the fonts directory is an implicit slot, not the content interface
20:38 < tyhicks> zyga: sorry to distract from the discussion but does the snap itself define the 
                 plug/slot attributes?
20:38 < zyga> jdstrand: it would be a slot on the core snap (it doesn't exist yet)
20:38 < zyga> tyhicks: yes
20:38 < zyga> small clarification, currently both source and destination are rooted at $SNAP
20:38 < jdstrand> zyga: right, so we add a rules for /usr/share/fonts like we do for /etc/alternatives
20:39 < zyga> so I guess we can constrain those profiles to /snap/*/**
20:39 < jdstrand> zyga: and then a different glob rule for content
20:39 < jdstrand> yes! :)
20:39 < zyga> I missed those two lines, I just read the content interface

[jdstrand]

Please change this to:

Support mount profiles via the content interface

[zyga]

Sorry for missing this earlier, changing now

[jdstrand]

From IRC discussion (thanks @zyga for remembering the deny rules):

# but don't share /snap/bin
audit deny mount /snap/bin/** -> /**,
audit deny mount /** -> /snap/bin/**,

[zyga]

Done

[jdstrand]

If you:

• reword the comments as suggested
• add the audit deny rules as suggested
• adjust the spread test as recommended

+1

[jdstrand]

This grep will be brittle in the long run since the kernel audit subsystem changes its output periodically, I suggest something like grep 'apparmor="DENIED" .* srcname="/snap/bin/" and leave it at that.

[jdstrand]

With the recent updates, LGTM (though updating the comments as requested would be nice).