xdp-utils: inspect sender security label to detect snap packages. #136

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

jhenstridge commented Dec 12, 2017

I've been working towards getting xdg-desktop-portal to work with snap packages (https://snapcraft.io/). I don't yet have everything functioning, but thought I'd open a PR as a starting point to see if there is interest in accepting this upstream. If there is interest in this, I've got a similar patch for xdg-document-portal.

It is possible to detect that we're talking to a snap confined application by checking that it's AppArmor security profile starts with "snap.". I've switched over to using GetConnectionCredentials, which provides both the security label (for detecting snaps) and the process ID (for detecting flatpaks), so this doesn't impose additional round trips.

I am using libapparmor to parse the profile out of the security label, but the functions used are so trivial that we could reimplement them to avoid the dependency if desired.

There is a bit of an impedance mismatch because snap package names do not match Flatpak's application ID format. As a work around, I am prepending "snap.pkg." to the snap package name to fulfil the requirement for three dot separated components. This isn't perfect, since I think there are some valid snap package names that aren't valid for the last component of the application ID. I think the idea of including some kind of prefix has merit though, since it should help avoid namespace conflicts.

Owner

alexlarsson commented Dec 14, 2017

Hmm, this sounds interesting, and i think we should take something like this. However, i'd like to figure out some details first.

First of all, yes, ideally drop the libapparmour dependency, its just some string ops anyway.

Secondly, the long term approach to app authentication in flatpak is to add proper container ids to the dbus daemon itself, which is happening here: https://bugs.freedesktop.org/show_bug.cgi?id=100344
It is still work in progress, but I think any new design for things like application identifiers etc need to be done with that in mind. So, rather than modifying the app id we should maybe have a container type + app-id identifier in the code.

What are the rules for snap names? Allowable names, names for different versions of the same app etc.

And last, xdg-document-portal is currently part of flatpak, and has some (minor) entaglements with it. I guess this would be a problem for snap? Maybe we have to look at untangling that?

Contributor

jhenstridge commented Dec 14, 2017

Reading through that bug, it sounds like it depends on having a supervisor process (either per-instance, or per-app-per-user) to manage the restricted D-Bus socket. Our current mediation system exposes the normal D-bus sockets to the sandbox and relies on dbus-daemon to query the kernel security policy to make decisions.

For what is is worth, I don't think the linux security label has the same race condition caused by looking up /proc/$PID/root/.flatpak-info, since dbus-daemon is querying the open client socket:

https://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-sysdeps-unix.c#n1765

As for snap package names, they need to match the regexp "^([a-z0-9]+-?)a-z$", which basically means they're made up of lower case letters, digits, and dashes, but can't start or end with a dash, have two consecutive dashes, and must have at least one letter.

We don't have separate security policies for different versions of the app: when a new version is installed or the user/admin adjusts the interfaces of a snap, the same security label is used. And if the user is installing the app via the store (rather than side-loading via "snap install --dangerous filename.snap"), only the registered owner(s) will be able to push new versions out to users.

As for xdg-document-portal, I put together a matching patch when I was initially putting together the prototype, but haven't tried updating it for the current state of master yet:

jhenstridge/flatpak@726ae80

My initial thought was to just create a new binary package containing xdg-document-portal and xdg-permission-store to a new binary package built from the existing flatpak source package. Having those components collocated with xdg-desktop-portal might have some benefits, such as sharing the logic for identifying the D-Bus peer, but isn't essential.

Owner

alexlarsson commented Dec 14, 2017

Not a supervisor in the sense that it hangs around, its just part of spawning the app. The idea is that the spawner talks to the (real) dbus-daemon and creates it an app-specific listening socket, then it gives the app access to that. In flatpak this would be done by bind-mounting the socket into the app sandbox as /run/user/1000/bus. Any client that connects to the bus via that socket will automatically have the container identifier and data hanged off it.

Not only does this fix the race (which, yes, security label doesn't have as long as your app can't change label or exchange fds with anything in a different label), but it also allows you to hang various other metadata on the client, such as information about various permissions. In the case of flatpak we basically serialize the metadata file there.

For example, the plan is for sandboxed dconf to add access permissions there so that the dconf daemon knows which parts of the tree the app is allowed to see.

Owner

alexlarsson commented Dec 14, 2017

Another advantage is that it gives a unique id per instance of the app

Owner

alexlarsson commented Dec 14, 2017

So, in the snap model, you can safely get the snap app name, but what if you want to know the security settings for that snap. Is there some lookaside service for that?

Contributor

jhenstridge commented Dec 15, 2017

Looking at the proposed API in that bug, you'd need to have some process holding the other end of the close_notification file descriptor open for as long as dbus-daemon should accept connections from the app container's socket. I suppose you could leave the file descriptor open when exec'ing the sandboxed app, but that seems a bit unreliable since it relies on the app ignoring the open file.

As for getting more information about the package, that is possible by querying snapd. We have a snapd-glib library that is probably the best choice for this: the protocol is HTTP over a unix domain socket, which libsoup doesn't quite support correctly yet.

Owner

alexlarsson commented Dec 15, 2017

In the case of flatpak we always have a minimal pid1 (because we use pid namespaces) so we have a place to hang that on. In the case of snap, could you not handle that fd in snapd? Or does that not monitor container lifetimes?

I was hoping you could use this too, and hang the snap permissions of the dbus container metadata. I mean, portals wanting to do decisions based on app permissions would still have to have per-containement-system code (because the permissions are different), but they would use the same mechanism to get it.

Contributor

jhenstridge commented Dec 16, 2017

We don't currently have a similar supervisor process. Snapd is a system wide daemon (snaps aren't just targeting the desktop use case), so I'm not sure it is in a good position to handle something like this.

It is quite possible we'll need some kind of session level supervisor for some other desktop related improvements I want to make, so it isn't something I'd say is impossible: just not something I can easily plug into our existing infrastructure.

Owner

alexlarsson commented Dec 18, 2017

@smcv I was hoping snap would also be able to use the new dbus container id thing. Maybe we need to tweak it a bit?

Owner

alexlarsson commented Dec 18, 2017

@jhenstridge: @desrt was going to use the dbus container id + extra metadata for sandboxing dconf, including for snap, although she is obviously not working on canonical stuff anymore...

Contributor

smcv commented Dec 18, 2017

Looking at the proposed API in that bug, you'd need to have some process holding the other end of the close_notification file descriptor open for as long as dbus-daemon should accept connections from the app container's socket.

Yes. If we want apps to have some sort of defined lifetime then something needs to know what the lifetime of the app is/was - either a supervisor/init process for a new pid namespace holding the fd open until all of its child processes die, or a centralized daemon watching until a cgroup becomes empty, or anything else suitable.

The notification doesn't have to get to dbus-daemon by closing a fd, it can also be an explicit D-Bus method call (and we can have multiple ways to notify about the same fact if necessary) - in fact the fd-closing thing isn't implemented yet, because I can't work on the containers interface all the time, whereas the explicit D-Bus method call is already in place. However, for any API that needs to know how long an app is still running (which the private D-Bus servers plan does), it does need to exist.

I was hoping snap would also be able to use the new dbus container id thing

The D-Bus container IDs were intended to be container-technology-agnostic, with Snap as the second potential user after Flatpak.

I don't really want the dbus-daemon to have an increasing number of security-technology-specific code paths for different forms of metadata, because, to be blunt, dbus just doesn't have enough maintainers for it to be OK to duplicate effort - we already have trouble finding reviewers for the SELinux code that's already there. I don't think it's a good idea to have more duplication than there absolutely needs to be in xdg-desktop-portal either.

Contributor

jhenstridge commented Dec 19, 2017

I've started a discussion on the snapd forum about D-Bus app containers. I don't know whether I'll get much of a response before Christmas, but I'll make sure to poke the relevant people.

https://forum.snapcraft.io/t/interacting-with-dbus-daemons-app-container-feature/3245?u=jamesh

I'll be in the same room with one of the snapd security guys at the end of January, so if we don't get much traction before then I'll make sure it goes on the agenda.

Owner

alexlarsson commented Dec 20, 2017

You wont get much response here either for a bit. I'm off for xmas and then skiing...

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