Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aleixpol
Copy link
Contributor

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.

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.
@aleixpol
Copy link
Contributor Author

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>
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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

@smcv
Copy link
Collaborator

smcv commented Nov 22, 2022

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.

@smcv
Copy link
Collaborator

smcv commented Nov 22, 2022

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.

@jadahl
Copy link
Collaborator

jadahl commented Nov 22, 2022

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.

@swick
Copy link
Contributor

swick commented Nov 22, 2022

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.

@smcv
Copy link
Collaborator

smcv commented Nov 22, 2022

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.

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 NO_NEW_PRIVS (Flatpak apps do, but Snap apps might not), it can execve() a setuid executable, to get an extremely privileged process that has reused its pid, in the hope that you will look up the information of that much-more-privileged process and think "oh good, the request came from uid 0". Or, as a less reliable timing-dependent attack, it can exit just after sending its request, after arranging for lots of other processes to be started, in order to arrange for its pid to be reused for a more-privileged process, in the hope that you will look up the information of that more-privileged process and think that the request came from it. Flatpak apps are not vulnerable to the execve() thing, only the pid-reuse-based version, which is going to be a lot harder to exploit reliably, but it's still something that concerns me.

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.

@smcv
Copy link
Collaborator

smcv commented Nov 22, 2022

The portal implementation can use the extra information to decide to which extent the request should be trusted

How? What is the concrete use-case for this?

@jadahl
Copy link
Collaborator

jadahl commented Nov 22, 2022

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.

@aleixpol
Copy link
Contributor Author

How? What is the concrete use-case for this?

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.

@swick
Copy link
Contributor

swick commented Nov 23, 2022

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 XDP_APP_INFO_KIND_HOST. They do have an app id when they follow the standard and run in a specifically named cgroup (https://systemd.io/DESKTOP_ENVIRONMENTS/) so you could even allowlist apps which are of type XDP_APP_INFO_KIND_HOST and match a specific app id.

If the system component is not an app the appinfo is still of the type XDP_APP_INFO_KIND_HOST but at that point you might as well call the impl directly with a flag to skip the UI.

@aleixpol
Copy link
Contributor Author

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.

@swick
Copy link
Contributor

swick commented Nov 24, 2022

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?

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.

@aleixpol
Copy link
Contributor Author

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.

@aleixpol
Copy link
Contributor Author

The biggest problem I see with using the unit name is that it applies to the process itself but also all of its children.

@swick
Copy link
Contributor

swick commented Oct 9, 2024

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.

@swick
Copy link
Contributor

swick commented Oct 9, 2024

@aleixpol do you still have problems with this in KDE?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

4 participants