interfaces: deny connected x11 plugs access to ICE #9542
Conversation
The ICE protocol allows the application to tell the session manager how to restart an application from previously saved state, along with the entire command line to execute. This can be used to craft a sandbox escape, assuming a compatible session manager is used. Fixes: https://bugs.launchpad.net/snapd/+bug/1901489 Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
| @@ -138,6 +139,13 @@ owner /run/user/[0-9]*/mutter/Xauthority r, | |||
| network netlink raw, | |||
| /run/udev/data/c13:[0-9]* r, | |||
| /run/udev/data/+input:* r, | |||
|
|
|||
| # Deny access to ICE granted by abstractions/X | |||
There was a problem hiding this comment.
What is the risk of regressions here?
There was a problem hiding this comment.
Unclear, it depends on how apps react. ICE is optional so I suspect most apps won't care or notice. It's a dead part of the spec and GNOME removed support for it a long while ago.
There was a problem hiding this comment.
I think the risk is very low. Very few applications implement the protocol, and those that do should be able to run on desktops without an XSMP session manager.
In addition, it is not clear that any snaps can successfully communicate via ICE on current distros. libICE 1.0.10 changed where it stores the authentication cookies to $XDG_RUNTIME_DIR/ICEauthority:
https://gitlab.freedesktop.org/xorg/lib/libice/-/commit/b484311c929a1b64966d89da92fafce7263006e1
This change was first included in Ubuntu 19.10, after which none of my desktop sessions have written a cookie to the old ~/.ICEauthority location. The <abstractions/X> AppArmor fragment does not grant access to the new location, so it is unlikely that a snap would be able to successfully connect.
There was a problem hiding this comment.
On my focal system I do get ~/.ICEauthority, created on the 22nd of January 2020.
There was a problem hiding this comment.
On a fresh install of focal I only see $XDG_RUNTIME_DIR/ICEauthority - there is ~/.ICEauthority so perhaps @zyga yours is left from a previous install (especially since it was created before focal was released).
I also agree the risk of regressions is very low, looks good to me.
There was a problem hiding this comment.
The XSMP server should be creating a new auth cookie each time you log in. Assuming you're not still running a login session from January, it is unlikely that there are any valid cookies in ~/.ICEauthority. You can compare the contents with the following commands:
iceauth -f $XDG_RUNTIME_DIR/ICEauthority list
iceauth -f ~/.ICEauthority list
If the file in your home directory does not contain the cookies from the $XDG_RUNTIME_DIR file, then it can't be used to talk to the current gnome-session instance.
There was a problem hiding this comment.
@alexmurray yes, I removed the file and it did not come back on the subsequent login. Must have been a result of keeping HOME across many releases and upgrades.
|
|
||
| # Deny access to ICE granted by abstractions/X | ||
| # See: https://bugs.launchpad.net/snapd/+bug/1901489 | ||
| deny owner @{HOME}/.ICEauthority r, |
There was a problem hiding this comment.
should this be just deny ${HOME}/... ? I'm not familiar with how deny interacts with owner? The only other place I see for deny owner is in a commented out snippet from Jamie 4 years ago in the common template, and in tests.
There was a problem hiding this comment.
deny owner @{HOME}/.ICEauthority r, is fine - owner says that to allow r to only files that are owned by the accessing user (since @{HOME} is just a regex which provides access to all /home not /home/$USER since apparmor doesn't know the real $HOME so by using owner in the original rule in the X abstraction this ensures the user is only allowed access to their own file as they are the owner of that) - and deny says to make this denied (so that we override the existing allowed access provided by the X abstraction).
There was a problem hiding this comment.
You can see examples of existing deny owner combinations at say https://gitlab.com/apparmor/apparmor/-/wikis/Profiling_with_tools - or even audit deny owner in /etc/apparmor.d/abstractions/ubuntu-helpers on Ubuntu systems.
There was a problem hiding this comment.
I wasn't sure myself so I went with the simplest initial choice, just deny the originally granted rule. Based on Alex's description it seem this is fine, though I would not mind reducing this to just deny ..., that is without the owner field.
There was a problem hiding this comment.
The deny owner is fine, I just wanted clarity on that, I didn't find the example upstream, thanks @alexmurray for the link.
There was a problem hiding this comment.
Sorry about this being after the fact, but I think that 'deny' would've been better than 'deny owner' since you're trying to undo something from somewhere else and I'd just assume block accesses to all ICEauthority files, whether they are owned by the user or not. What is committed is not 'wrong' since the X abstraction only allows owner match, but if something else allowed access to all ICEauthority files, then the 'deny owner' rule would allow reads to other users' ICEauthority files, only denying the ones owned by the user.
mvo5
added
the
Needs security review
This is the more modern location of the ~/.ICEauthority file. We should deny access to both to prevent the usage of the X session management protocol. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
|
FWIW, LGTM too 👍 |
| # Deny access to ICE granted by abstractions/X | ||
| # See: https://bugs.launchpad.net/snapd/+bug/1901489 | ||
| deny owner @{HOME}/.ICEauthority r, | ||
| deny owner /run/user/*/ICEauthority r, |
There was a problem hiding this comment.
FYI, when we embed apparmor3, we'll want to use the @{run} tunable.
The ICE protocol allows the application to tell the session manager how
to restart an application from previously saved state, along with the
entire command line to execute. This can be used to craft a sandbox
escape, assuming a compatible session manager is used.
Fixes: https://bugs.launchpad.net/snapd/+bug/1901489
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com