-
-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RemoteDesktop: Pass the creator's pid when creating a session #913
base: main
Are you sure you want to change the base?
Conversation
The portal implementation can use the extra information to decide to which extent the request should be trusted. This can be done by checking the process namespace or its origin in the filesystem.
I wonder if it would make sense to pass the XdpAppInfoKind too. |
The process id of the process that is creating the session. | ||
</para></listitem> | ||
</varlistentry> | ||
</variablelist> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this couldn't instead be something that could be put on the org.freedesktop.portal.Session
API instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make sense too, as long as it's xdp setting it. It doesn't need to be part of the client's API as they already know their pid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Erm, yes, I meant org.freedesktop.impl.portal.Session
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, If you all agree that this is the way to go, I'll implement this instead
Any time you're passing around a pid in Flatpak-adjacent space, please document it as something like "the process ID of the creator in the root pid namespace" to be unambiguous about whose view of the process hierarchy we're dealing with. |
When a Flatpak app creates a RemoteDesktop session, is this really the pid of the Flatpak app, or is it the pid of the xdg-dbus-proxy that is talking to us on the app's behalf? The distinction is particularly important because if it's the pid of the Flatpak app itself, then there are potentially attacks that it can do by exploiting pid reuse to fool us into thinking it's someone else (https://bugs.freedesktop.org/show_bug.cgi?id=83499 - and in fact Maemo/MeeGo had its own out-of-tree LSM which was vulnerable to a similar attack). If it's the pid of xdg-dbus-proxy, then we can trust xdg-dbus-proxy to behave more reasonably. When adding information that would be reasonable to use for authorization decisions, we should try to be explicit about whether that information can safely be trusted, and whether other information that's derived from it (in this case by looking up the pid in /proc or similar) can safely be trusted. |
The only reasonable thing I assume would to ensure that any communicated pid is the pid of the application itself, not any dbus proxy, remapped to the host pid namespace, so that it can be consumed by the portal backend for any custom stuff it wants to do. |
Like @smcv said, you cannot trust any pid except the pid of the xdg-dbus-proxy. No amount of remapping and trickery will get rid of the fundamental races. If you want to make security decisions based on the pid, it must be the pid of the xdg-dbus-proxy. If we ever move away from xdg-dbus-proxy and move authentication to the dbus server there won't be a single pid that can be used. So I would even argue that passing any pid to anywhere should not happen. The pid should only be used to authenticate the app and the resulting XdpAppInfo must be used for all further security decisions (without adding a pid to that struct). I don't know why you want the PID in the RemoteDesktop backend but this is a bad idea. |
I'm not so sure that's a good idea, for two reasons. If you don't trust the application, then using its pid for any trust decisions is not particularly safe. This is because the pid is just a number, and not directly meaningful: whenever you say "I want to know the pid", what you really mean is usually something more like "I want to know the pid so I can use it to find out the SELinux context" or "I want to know the pid so I can correlate that with all the windows it has created" or "I want to know the pid so I can use it to look up the Flatpak app ID", often by looking up extra information in /proc. However, because the application controls its own lifetime, it can do sneaky things to trick the caller into getting the wrong information. If it doesn't have The second reason is that for Flatpak apps, what you say is not necessarily going to be implementable, because neither the message bus (dbus-daemon or dbus-broker) nor the xdg-desktop-portal have access to the pid of the actual application in an unforgeable way. This is because the message bus is only directly talking to the xdg-dbus-proxy, and the xdg-desktop-portal finds out the pid of the xdg-dbus-proxy by asking the trusted message bus for that information. This is partly so that the xdg-dbus-proxy can filter D-Bus messages to apply finer-grained permissions to them, but it's also partly so that the xdg-dbus-proxy can mitigate the attacks I described above, by being a component that can be trusted not to exit at a precisely attacker-chosen time or execve() an attacker-chosen binary. |
How? What is the concrete use-case for this? |
I agree pids are problematic, and I don't know the intended use case, but I imagine if it was the pid pointing to the dbus proxy, it wouldn't be very useful, for whatever the intentions are. |
We want to be able to use the portal to control the system without needing to interact with a dialog on specific cases, like processes that come from the OS and are supposed to have such permission granted. In this case, RemoteDesktop is special because you only use it when you are not in front of the system. That is use cases like kde connect or krfb (which we use for remote connections). Both these cases already have systems in place (much more sophisticated than the dialog) that tell us it's fine to use them to control the system. If we have the PID, we can put together infrastructure to identify them. Note that right now, apps outside of snap and flatpak don't even offer an app_id, and even then, we'll need to somehow identify where it comes from. In this case, knowing that it comes from the distro binaries (i.e. It's in /usr as meant by your distributor and owned by root) should be enough to say "yes, go ahead, you can control the system". Regarding the xdg-dbus-proxy, I don't really mind right now either way. Nevertheless, if we can implement something that can go beyond the /usr use-case, I am happy to look into it too. Offering the desktop file path instead of the PID, could be another interesting approach. |
If the system component that is supposed to have permissions by default runs on the host you can give all host apps that permission by checking for If the system component is not an app the appinfo is still of the type |
In the case of kde connect I'm seeing "" as an app_id because it's launched by the dbus daemon. I guess we should extend the app_id-detection capabilities? There's apps that will be registering as host (e.g. AppImage) that we will also be unable to filter out or plain out trust. I was going for /usr as means to identify that its level of trust is equivalent to the current process's. |
Please, yes. If dbus activation doesn't follow the cgroup naming convention mentioned above this could definitely happen and we should look into a way to fix that. I also think we should add a portal to set the app id that is only used for host apps if all other app id detection fails. |
Here's the support for app_id on dbus services: #921 Now this does give us some more information as we know what we are running exactly. I wonder if we should also offer a XDP_APP_INFO_KIND_DBUS in XdpAppInfoKind or similar. This would allow us to infer which service file this is coming from. |
The biggest problem I see with using the unit name is that it applies to the process itself but also all of its children. |
If you spawn another program, it should also be moved to its own cgroup/systemd scope. At least gnome has code that spawns apps and that should take care of it. |
@aleixpol do you still have problems with this in KDE? |
The portal implementation can use the extra information to decide to which extent the request should be trusted. This can be done by checking the process namespace or its origin in the filesystem.