interfaces: remove apparmor downgrade feature #9936
Conversation
Apparmor downgrade was automatically enabled when the running kernel supported some, but not all of the features. Since the complete set was never upstreamed, this effectively meant that users had less features than they otherwise would have. Since apparmor is still reported as "partial", nothing changes from the point of view of not sending any misleading messages. For certain classes of snap packages, this improves the effective confinement on systems such as Debian or openSUSE Leap. Perfect confinement is still way off, this doesn't change that. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
zyga
changed the title
zyga
changed the title
anonymouse64
added
the
Needs security review
|
Fwiw, this is related to https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=923500 - I think we want/need to explore how much confinement we can enable on Debian now that apparor is available there. But AIUI some of the Ubuntu patches are not applied in their kernel. |
| // so the meaning of the template would change across kernel | ||
| // versions and we have not validated that the current template | ||
| // is operational on older kernels. |
There was a problem hiding this comment.
My main concern about this PR is the comment from this snippet, do we know that not downgrading snap confinement will result in working snaps across the set of supported systems? It feels like the answer is yes given that many AppArmor features have in fact been upstreamed, but I don't know for sure.
Said another way, do we understand the fallout from this change? Yes it will make more snaps more secure, but will it make other snaps not work at all on other distros like Debian which are not simply missing AppArmor patches in the kernel but in fact have incompatible AppArmor patches in the kernel which enforce policy in a way our policy does not reflect and thus break some snaps.
Maybe it's okay that some snaps are broken if on the whole more snaps are secure, but I think we should attempt to understand that first.
There was a problem hiding this comment.
I think at the very least our spread tests should give some indication of this, but I don't think our spread tests exercise enough different features of snap confinement to ensure that there aren't snaps out there which we would be breaking by doing this.
There was a problem hiding this comment.
Can't say about Debian, but on Arch and Tumbleweed we haven't really observed any major hiccups so far, with just one exception. The incomplete fine grained Unix socket mediation broke accessing X11 on Tumbleweed after a change to x11 interface landed, but I carry this patch bboozzoo@2a5f97e in in the downstream packaging. If what @anonymouse64 meant by incompatible patches is this functionality, then the Debian package should likely carry that patch too. Note that we do not downgrade the AppArmor snippets for interfaces, even if the base template was simplified.
Looking at the template, I would expect the following rules to be ineffective without the remaining non-upstreamed bits of AppArmor:
unix (bind, listen) addr="@snap.@{SNAP_INSTANCE_NAME}.**",
unix peer=(label=snap.@{SNAP_INSTANCE_NAME}.*),
...
dbus (receive, send) peer=(label=snap.@{SNAP_INSTANCE_NAME}.*),
...
dbus (send)
bus={session,system}
path=/org/freedesktop/DBus
interface=org.freedesktop.DBus.Introspectable
member=Introspect
peer=(label=unconfined),
Where the unix rule probably allows all unix socket access, and dbus is ignored (would also require dbus to be built with apparmor support?).
I would say that the downstream packaging should just disable AppArmor during the build if there's any concern about compatibility, in which case they would need to carry this patch: #9773 (also carried in openSUSE) so that snapd does not do something silly in specific scenarios.
One thing we could probably do is to display a warning that confinement is partial, like we do for cgroups v2. This information is available to via snap debug confinement but it's not intuitive and maybe it's the reason for the Debian bug we have.
It would also be nice if I finally got down to implementing the AppArmor related enhancements in our spread tests. IMO we're finishing confinement tests too early after a simple check "$(snap debug confinement)" != strict, where we could check for a specific apparmor feature(s) and continue executing the test. I believe we would have a better view of what is actually supported.
There was a problem hiding this comment.
@anonymouse64 I understand the concern but I think we need to be realistic.
The strategy to wait for upstreaming of all apparmor bits failed.
Right now users have effectively no security and perhaps sometimes, broken snaps (like nextcloud which can reach to host's mysql socket) CC @kyrofa for context if required. This is a reality check that we need to change direction. This is that change.
Is it secure? No, but neither was the previous version - it had not apparmor at all.
Is it guaranteed to work? No, but neither was the previous version - due to lack of apparmor, apps might load or access content from the host and misbehave.
I think we just need to honestly look at what is the possible outcome from pursuing those distinct paths and see what is better for our users and what is more honest.
There was a problem hiding this comment.
For interested readers, @zyga is referring to LP: #1819734.
Because snaps bundle their dependencies, snap developers often find themselves integrating pieces of software into their snap where they're lacking in-depth familiarity. It's not unusual for these components to try and reach out to the system and then have reasonable fallback behavior when it learns that such a thing is impossible. It took me ages to figure out why mysql was failing on Debian, and then more ages to figure out a way to make it stop trying to grab that file.
Ultimately, I believe that if snapd wants a reasonable cross-distro story, it needs to take ownership of ensuring that snaps run similarly everywhere instead of leaving the onus on:
- The user, who has no idea that they ran
sudo snap install nextcloudand now have a daemon running as unconfined root on their server until it's far too late because they are under the impression that snaps run the same everywhere - The snap developer, who created a snap thinking "snaps run the same everywhere, I'll be able to package once and my users can use it wherever they want without increasing my support load" when in reality, non-Ubuntu snap users can easily become the majority of support requests because snaps run very differently on non-Ubuntu systems
I want to re-emphasize this latter point. I've found myself saying a few times "sorry, I have no idea how snapd on [os] works and I don't have the bandwidth to install it and get to the bottom of it." Essentially, I've found myself slowly moving toward a "I can only support you if you're running Ubuntu" model out of necessity, which really hurts the snap story. I think a more granular approach to security is a step in the right direction. It's significantly more work, though!
There was a problem hiding this comment.
My understanding is that the reason for the current 'fail-open' approach was that we didn't want to appear to users to be offering strict confinement when in fact, due to missing support in the host's environment, various things would not actually be mediated properly (dbus etc) without this fallback. However, I agree with the reasoning in the comments of this PR that this approach has created a lot of difficulty in various situations, and obviously results in a less secure deployment.
As such this PR will create a more secure setup for some users as we will now enforce confinement in these cases - I agree this will likely break some snaps which now get denied things that may have previously worked by chance, and this will obviously have holes in the confinement if various features are not supported - but this should be a better overall result than the status quo where there is no confinement at all.
There was a problem hiding this comment.
We are currently exploring which if any snaps will be broken by this change with some help from the advocacy team. I think I am in general on-board with this change so long as we at least understand who/what we are breaking (though note the spread tests on debian here were all happy).
|
@alexmurray can you clarify if you have fully reviewed this from a security team perspective and remove the label if so? I assume you have not since the label is still present |
|
@anonymouse64 no I have not fully reviewed this from a security perspective. Whilst I am in favor of making this change, it is clearly quite a departure from the way things have operated in the past and I am not sure how to quantify the impact of this since it will change the behaviour of strict snaps across a wide variety of platforms. I don't have time myself at this stage to try and do indepth testing of snaps across the various platforms with this change, but I would be keen to know more about the testing that is being done and what in particular is being looked at for this. |
|
@alexmurray we have conducted our testing the detailed spreadsheet w/ testing results is here: https://docs.google.com/spreadsheets/d/113nvFT6bqJWRR2BQaLgxHA8GOHR5gh--8dES5-E6XHs/edit#gid=0 but the TLDR is that all of the snaps we tested functionally worked fine on Debian 11, 10, and 9, albeit some with AppArmor denials. So I think this PR is good to move forward from a testing perspective, there may still be some small breakages, but overall we think this is safe to pull in. |
|
Worst case we will have to revert but let's hope not. |
|
Woot, thank you for this bold and important move everyone. |
Apparmor downgrade was automatically enabled when the running kernel
supported some, but not all of the features. Since the complete set was
never upstreamed, this effectively meant that users had less features
than they otherwise would have.
Since apparmor is still reported as "partial", nothing changes from the
point of view of not sending any misleading messages. For certain
classes of snap packages, this improves the effective confinement on
systems such as Debian or openSUSE Leap.
Perfect confinement is still way off, this doesn't change that.
Signed-off-by: Zygmunt Krynicki me@zygoon.pl