xdg-desktop-portal
xdg-desktop-portal copied to clipboard
flatpak
Allow portal access non-flatpak sandboxes
Currently, the portal tries to check the process filesystem to inspect its flatpak metadata, but this is not possible for non-flatpak sandboxes, for which D-Bus access is out-of-scope here.
In particular, this affects firejail applications which have been
granted full d-bus access (e.g.: d-feet).
Fixes https://github.com/flatpak/xdg-desktop-portal/issues/737
Looking at the diff in github, and there's a severe indentation problem. There's also typos in the comments.
I added braces to make the block clear (the else was a bit ambiguous). Does that cover the indentation problem you mention? I believe it fits the style of the surrounding code.
Fixed two typos in the comment.
The indentation is still wrong. See line 1731 in xdp-utils.c for an example of an else if block. I think it would make sense to move your comment into the else if block and use curly braces even though there's only one line of code.
Yeah the indentation looks correct. I don't know whether the code changes are correct.
I'm not 100% sure this is safe. Can we guarantee that there is no way a flatpak:ed app can create a request such when the portal calls statfs(/proc/$pid/root) it returns EACCES. Because if it can, then with this change the portal consider the app fully privileged.
The comment below mentions one way to trigger this error codepath:
/* Otherwise, we should be able to open the root dir. Probably the app died and we're failing due to /proc/$pid not existing. In that case fail instead of treating this as privileged. */
It doesn't obviously look like a pid-exit race like the above would get an EACCES. But on the other hand, I can't convince myself it is impossible to cause that somehow. The statfs() was added to the EACCES error especially to catch this risk, because it is impossible for a flatpak sandbox to have a fuse rootfs.
Can we maybe use some other way than statfs() failing with EACCES to catch the case of "app is a firejail sandbox" that can't be faked by flatpak:ed apps?
Can we guarantee that there is no way a flatpak:ed app can create a request such when the portal calls statfs(/proc/$pid/root) it returns EACCES.
I'm not 100% sure, but I believe you need either fuse or setuid to trigger this, neither of which should be possible inside Flatpaks.
Because if it can, then with this change the portal consider the app fully privileged.
I admit I'm a bit confused about this check though -- doesn't the xdg-dbus-portal deal with restricting D-Bus access? Why does the portal need to check this again?
It doesn't obviously look like a pid-exit race like the above would get an EACCES. But on the other hand, I can't convince myself it is impossible to cause that somehow. The statfs() was added to the EACCES error especially to catch this risk, because it is impossible for a flatpak sandbox to have a fuse rootfs.
Proving a negative is hard (if even possible). We should mostly care that Flatpaks cannot trigger this.
Can we maybe use some other way than statfs() failing with EACCES to catch the case of "app is a firejail sandbox" that can't be faked by flatpak:ed apps?
I've though about checking whether the parent is firejail itself and is not in a namespace... do you think this could potentially be spoofed somehow? The approach sounds like it could work, but it honestly sounds like too much of a hack.
One downside of this other approach is that it fixes Firejail'd applications, but not other scenarios, for example, non-flatpak bubblewrap sandboxes.
Can we guarantee that there is no way a flatpak:ed app can create a request such when the portal calls statfs(/proc/$pid/root) it returns EACCES.
I'm not 100% sure, but I believe you need either fuse or setuid to trigger this, neither of which should be possible inside Flatpaks.
I don't think you need setuid. For example, if you can trigger pid reuse by some other uid, then the /proc/$pid/root for that process might be inaccessible to the portal running as the current user. That's not necessarily easy to do, but maybe not impossible.
Because if it can, then with this change the portal consider the app fully privileged.
I admit I'm a bit confused about this check though -- doesn't the
xdg-dbus-portaldeal with restricting D-Bus access? Why does the portal need to check this again?
Flatpak implements outbound dbus filtering using a per-app dbus proxy, not via the portal. But that has nothing to do with this.
The aim of the portal is to control access to sensitive operations, as such its primary function is to securely verify the credentials (like the app id) of the calling process so that it can decide what it should have access too. That is what this function is doing. If it returns NULL (without error), it means that we decided that the calling process is a "host app", and thus has unbounded access to all sensitive APIs.
So, If a flatpak app can in any way make this function return NULL, then all security granted by the portal mechanism is worthless.
Can we maybe use some other way than statfs() failing with EACCES to catch the case of "app is a firejail sandbox" that can't be faked by flatpak:ed apps?
I've though about checking whether the parent is firejail itself and is not in a namespace... do you think this could potentially be spoofed somehow? The approach sounds like it could work, but it honestly sounds like too much of a hack.
How do you propose to check that the parent is firejail? Do you have some trusted id for it at the kernel level we can use?
One downside of this other approach is that it fixes Firejail'd applications, but not other scenarios, for example, non-flatpak bubblewrap sandboxes.
If people want to re-implement sandboxes and integrate with other systems like portals they need to do the work and integrate with it. I don't see any way around that. Security is very hard, a single error makes the entire thing fall over, so we can't play loose.
For example, with this patch, applications that use firejail and access the portal will have full access to everything. So it makes the portals "work", but only by making them useless (in terms of security at least).
If people want to re-implement sandboxes and integrate with other systems like portals they need to do the work and integrate with it. I don't see any way around that. Security is very hard, a single error makes the entire thing fall over, so we can't play loose.
I'll never get tired of linking to this: https://gitlab.freedesktop.org/dbus/dbus/-/issues/171
Probing a process to find its identify is very hard, subtle and generally not the greatest idea. Creating and mounting a new socket in each app instance completely removes all of that.
I don't think you need setuid. For example, if you can trigger pid reuse by some other uid, then the /proc/$pid/root for that process might be inaccessible to the portal running as the current user. That's not necessarily easy to do, but maybe not impossible.
If the pid is reused by a process, and that process can either use fuse or suid, then it's already a privileged process. In this case: it's not a Flatpak, should not try to treat is as such.
If it returns NULL (without error), it means that we decided that the calling process is a "host app", and thus has unbounded access to all sensitive APIs.
The xdg settings portal is not a sensitive one. E.g.: Trying to read the GTK theme or the dark mode is not dealing with sensitive data, is it?
How do you propose to check that the parent is firejail? Do you have some trusted id for it at the kernel level we can use?
This one?
https://superuser.com/questions/150117/how-to-get-parent-pid-of-a-given-process-in-gnu-linux-from-command-line
For example, with this patch, applications that use firejail and access the portal will have full access to everything. So it makes the portals "work", but only by making them useless (in terms of security at least).
Firejail itself can filter the d-bus calls. Look for dbus in firejail-profile(5). I'm giving read-only permissions for, and this is mostly for the settings portal to work.
Probing a process to find its identify is very hard, subtle and generally not the greatest idea. Creating and mounting a new socket in each app instance completely removes all of that.
This is exactly what xdg-dbus-proxy achieves, and it's what firejail uses.
I don't think you need setuid. For example, if you can trigger pid reuse by some other uid, then the /proc/$pid/root for that process might be inaccessible to the portal running as the current user. That's not necessarily easy to do, but maybe not impossible.
If the pid is reused by a process, and that process can either use fuse or suid, then it's already a privileged process. In this case: it's not a Flatpak, should not try to treat is as such.
Alex was suggesting that maybe the process reusing the pid doesn't need to use setuid or fuse in order for /proc/$pid/root to be inaccessible, e.g. perhaps it's a process that starts out with a uid other than the one the portal runs under (the user's).
If it returns NULL (without error), it means that we decided that the calling process is a "host app", and thus has unbounded access to all sensitive APIs.
The xdg settings portal is not a sensitive one. E.g.: Trying to read the GTK theme or the dark mode is not dealing with sensitive data, is it?
Right, but your change would also allow access to all the other portal interfaces, not just settings.
How do you propose to check that the parent is firejail? Do you have some trusted id for it at the kernel level we can use?
This one?
https://superuser.com/questions/150117/how-to-get-parent-pid-of-a-given-process-in-gnu-linux-from-command-line
But how to securely determine if the parent pid is firejail?
For example, with this patch, applications that use firejail and access the portal will have full access to everything. So it makes the portals "work", but only by making them useless (in terms of security at least).
Firejail itself can filter the d-bus calls. Look for
dbusin firejail-profile(5). I'm giving read-only permissions for, and this is mostly for the settings portal to work.
You're talking about having firejail restrict the process's access to portal interfaces? Or are you talking about filtering D-Bus calls for other purposes?
The whole point of the portal interfaces is to provide safe access to system resources for sandboxed (or unsandboxed) processes, so wouldn't it make more sense to have x-d-p treat firejailed processes as a new type of sandbox (we already have Flatpak and Snap) rather than host processes?
If people want to re-implement sandboxes and integrate with other systems like portals they need to do the work and integrate with it.
Does this scale? Is there a limit for number of sandbox types x-d-p code can handle before it becomes overly complicated?
If people want to re-implement sandboxes and integrate with other systems like portals they need to do the work and integrate with it. I don't see any way around that. Security is very hard, a single error makes the entire thing fall over, so we can't play loose.
I'll never get tired of linking to this: https://gitlab.freedesktop.org/dbus/dbus/-/issues/171
I'm not unaware of this, this was designed by me and simon in 2017. But I'd like to point that it has gone nowhere in the 5 years since then, while all that time we have a working (hackish or not) solution to the flatpak peer id detection that is in heavy use.
I don't think you need setuid. For example, if you can trigger pid reuse by some other uid, then the /proc/$pid/root for that process might be inaccessible to the portal running as the current user. That's not necessarily easy to do, but maybe not impossible.
If the pid is reused by a process, and that process can either use fuse or suid, then it's already a privileged process. In this case: it's not a Flatpak, should not try to treat is as such.
You don't seem to understand the exploit. It matters not what the other process does or is. All the attacker needs is race condition such that the portal believes the caller has pid X, but by the time it verifies the id, something else is now running as pid X. Maybe a system service triggered by the attacker started this process. If that process is running as a different uid than the portal it may get EACESS trying to look at its root dir in proc. This will cause the portal to make the decision of allowing the request, which it should not have according to the permissions of the initial caller.
If it returns NULL (without error), it means that we decided that the calling process is a "host app", and thus has unbounded access to all sensitive APIs.
The xdg settings portal is not a sensitive one. E.g.: Trying to read the GTK theme or the dark mode is not dealing with sensitive data, is it?
The entire point of portals is to be a service that controls access to sensitive things, designed in such a way that it is safe to give full dbus access to even sandboxed apps, because it does its own security checks. Yes, there are some APIs that are not sensitive, but what is the point of mentioning them while ignoring the rest?
How do you propose to check that the parent is firejail? Do you have some trusted id for it at the kernel level we can use?
This one?
https://superuser.com/questions/150117/how-to-get-parent-pid-of-a-given-process-in-gnu-linux-from-command-line
This is just the parent pid. Lets say it returned 42, how do you then decide this is firejail or not?
For example, with this patch, applications that use firejail and access the portal will have full access to everything. So it makes the portals "work", but only by making them useless (in terms of security at least).
Firejail itself can filter the d-bus calls. Look for
dbusin firejail-profile(5). I'm giving read-only permissions for, and this is mostly for the settings portal to work.
I'm aware of dbus filtering, as I wrote the xdg-dbus-proxy program this firejail option uses.
However, that will only allow you to control whether you can talk to the portal or not. Further limitations on the portal APIs are designed on the idea that the portal can unforgeably identify the caller and apply app-specific permissions.
If people want to re-implement sandboxes and integrate with other systems like portals they need to do the work and integrate with it.
Does this scale? Is there a limit for number of sandbox types x-d-p code can handle before it becomes overly complicated?
What is the alternative? Stop being secure in our security framework?
The current implementation is also vulnerable to the same pid reuse attacks. The current check for buf.f_type == 0x65735546 suffers from all the same race conditions as the new branch that I'm adding here.
The whole point of the portal interfaces is to provide safe access to system resources for sandboxed (or unsandboxed) processes, so wouldn't it make more sense to have x-d-p treat firejailed processes as a new type of sandbox (we already have Flatpak and Snap) rather than host processes?
Trying to restrict the access of unsandboxed applications does not make sense, they are unsandboxed. They can potentially just spawn a toolbox container or use fuse to trigger the buf.f_type == 0x65735546 path that gives them unrestricted access. An unsandboxed process can also spoof a /.flatpak-info file.
Further limitations on the portal APIs are designed on the idea that the portal can unforgeably identify the caller and apply app-specific permissions.
The idea that the portal can "unforgeably identify the caller" is not correct; it cannot. This assumption is not valid. Trying to design around this premise is not possible.
The whole point of the portal interfaces is to provide safe access to system resources for sandboxed (or unsandboxed) processes, so wouldn't it make more sense to have x-d-p treat firejailed processes as a new type of sandbox (we already have Flatpak and Snap) rather than host processes?
So in order to special-case Firejail processes, we'd need to be recognise them in an unambiguous, unforgable way, but I don't believe that to be entirely possible. FWIW, I don't believe it to be possible for flatpaks either.
The implementation in this PR is not vulnerable to new attack angles compared to existing code.
Probing a process to find its identify is very hard, subtle and generally not the greatest idea. Creating and mounting a new socket in each app instance completely removes all of that.
Creating a new socket for each app is exactly what is being done by Firejail. This is what xdg-dbus-portal does. It is the correct thing to do. I don't see the value in the portal itself trying to identify clients, especially since it cannot be done correctly with complete certainty.
The entire point of portals is to be a service that controls access to sensitive things, designed in such a way that it is safe to give full dbus access to even sandboxed apps, because it does its own security checks. Yes, there are some APIs that are not sensitive, but what is the point of mentioning them while ignoring the rest?
Isn't the purpose of xdg-dbus-proxy to do these security checks and fulfil this role? It already implements this security aspect of things.
The entire point of portals is to be a service that controls access to sensitive things, designed in such a way that it is safe to give full dbus access to even sandboxed apps, because it does its own security checks.
If you give full dbus access to an app, its no longer sandboxed.
Yes, there are some APIs that are not sensitive, but what is the point of mentioning them while ignoring the rest?
The current implementation makes the public endpoints also unreachable for well-sandboxed applications.
What is the alternative? Stop being secure in our security framework?
Rely on xdg-dbus-proxy for dbus sandboxing. Don't try to re-implement security inside this portal. The approach taken is not a solid one anyway, it can be spoofed in foreign sandboxes anyway, and blocks some properly-set-up ones.
What is the alternative? Stop being secure in our security framework?
I thought more of defining standard way of connecting sandboxes with the x-d-p which could be used as fallback if checks for flatpak and snap failed. i.e reading metadata from /.sandbox-info file. Each sandbox would be responsible for unforgetability of its contents (if they fail to do so then it's their problem not x-d-p). I think it would be easier to adopt various sandboxes to work with x-d-p than adopting x-d-p to work with every sandbox.
I've given some thought to the initial issue, and maybe things are not as grim as I initially though.
Here is the basic setup:
A process running in a flatpak sandbox talks to the portal, via an xdg-dbus-proxy instance, which grants it (really all flatpaks) access to the portal, but nothing much else. This is safe, because the portal is designed to do further access checks. For example, the settings allow you to control which apps can send notifications, and unless it says org.foo.app is allowed to send notifications the notification API will just return a permission denied error.
For this to work, we can't have the app just claim to be org.foo.app, because we can't trust that it is not lying. So, the portal needs to have a way to identify the app-id of the peer outside of the control of the app. The way flatpak does this, is that each sandbox has a /.flatpak-info file with information, and the sandbox is setup with this file being a read-only bind mount, and the sandbox not having permissions to unmount/overmount/modify it, as well as not having support for sub-sandboxing that could modify the fs layout.
When the app connects to dbus the daemon asks the kernel what the peer pid is, and the portal then can ask the daemon what pid a particular well-known id on the bus has. Then the portal can look in /proc/$pid/root/.flatpak-info, and read information about the app. Since the app can't modify this this information is trustworthy to some degree. For example, it is possible for a non-flatpak to fake to have such a file, but that is not really something we care about as the app would need privileges to do so. However, we can only trust the file if we can actually read it. If there is an error during access to it, we can't be sure what is going on.
The most obvious error that can happen is that the process that sent the request has died and the directory no longer exists. If this happens we need to fail the operation, otherwise a malicious app could send notifications it shouldn't be able to by just sending the notification and exiting. That is what this check is about.
In this particular case however, the directory does exist, but we're getting an access error when reading it. The question is, is it possible that a sandboxed flatpak app can cause this particular error, because if that is so we must treat it as an error rather than not-flatpak, otherwise the app could use this method to make the portal fail to id them, and then send non-allowed notifications (for example).
For an unprivileged process, I can think of only two ways of causing an EACCESS for files in /proc. Either using prctl(PR_SET_DUMPABLE) which affects /proc file ownership, or by using a pid reuse attach and ensure that the new process with that pid is not run by the same uid as the portal.
However, in the case of a flatpaked app, the pid that the dbus daemon sees is actually that of the dbus proxy, and flatpak sets this up so that it is not visible/accessible by the app. In particular, this means that the app cannot change its dumpable state, know the pid of it, or make it die until all the other processes in the sandbox has died.
This later makes pid reuse attacks harder, but not impossible. For example, the app could connect to the bus, then start another sandbox (say a sub-sandbox via the portal) and pass the dbus connection file descriptor to that, and then kill the original app which will kill the proxy. However, after such a pid reuse attack the proxy will no longer proxy stuff and the dbus connection you now have can't be used to do requests.
There is still a window where you could send a request, exit the app and then the portal will see the request with the pid gone. However, this is a very small window, and I don't think it will be possible for a sandboxed (in particular, with a pid namespace) app to do a complex pid reuse attack in the time it takes from sending the message to the portal seeing it.
However, the reasoning above is really the same for the toolbox case, so If we believe this, then there is really no need for the check for a fuse rootfs, and the real fix is to just allow EACCESS errors to pass in general as not-flatpak without any additional checks.
The question is, did I miss some other possible way to make the stat() call return EACESS?
I thought more of defining standard way of connecting sandboxes with the x-d-p which could be used as fallback if checks for flatpak and snap failed. i.e reading metadata from /.sandbox-info file. Each sandbox would be responsible for unforgetability of its contents (if they fail to do so then it's their problem not x-d-p). I think it would be easier to adopt various sandboxes to work with x-d-p than adopting x-d-p to work with every sandbox.
Well, this is more or less what i meant, the other sandboxes need to do the actual hard work of integrating with x-d-p. However, it is true that if you can make everyone agree on just one new way of doing it it will be easier. You still need some per-sandbox-type customization though, because x-d-p needs more integration than just getting the app id. For example, it also needs a way to see what files are accessible to a particular instance. However maybe the flexibility of that could be encoded in the .sandbox-info file.
This would need very careful design to ensure that e.g. a flatpak app can't put a .sandbox-info file in / and work around the flatpak check, or vice versa. And even with all that in place the original issue still stands, i.e. if we can't ensure the .flatpak-info and the .sandbox-info file is missing, we can't know the app is not sandboxed, so it is unsafe to continue.
Given the complexity of the above I think it might make more sense for other sandboxes to just use a /.flatpak-info file, as its really just a simple file with the app id in. We could even add support in the portal for parsing some special keys to say "actually, this is not a flatpak, its a foopak" if we need custom handling for foopaks in the portal.
I'm not unaware of this, this was designed by me and simon in 2017. But I'd like to point that it has gone nowhere in the 5 years since then, while all that time we have a working (hackish or not) solution to the flatpak peer id detection that is in heavy use.
Right, I talked with Simon about this. Apparently progress died because of a lack of reviews. The idea is sound, it "only" needs an implementation and reviews but unfortunately I'm not really in position to review dbus code.
Does this scale? Is there a limit for number of sandbox types x-d-p code can handle before it becomes overly complicated?
What is the alternative? Stop being secure in our security framework?
Well no, the alternative is this mechanism. x-d-p or any other client could just query dbus for the app id. Any sandboxing mechanism only has to request a socket from dbus and make sure it's the only visible dbus socket in the sandbox.
This would finally make it possible again to have clients talk to each other. Right now every interaction has to be mediated by x-d-p somehow which makes issues like this one really annoying.
Well no, the alternative is this mechanism. x-d-p or any other client could just query dbus for the app id. Any sandboxing mechanism only has to request a socket from dbus and make sure it's the only visible dbus socket in the sandbox.
Yes, and for the record i think this is the only right approach. Its just that getting this in and implemented in all dbus daemon implementations is a lot of work. It definitely would be more secure, more usable and overall more sane than what we do currently.
Can this be merged? Discussion diverged onto how to improve the current model, but, AFAIK, there are no remaining blockers for merging this. It's a big annoyance that other applications cannot access the portal.
PID re-use attacks are theoretically possible, but that's also currently the case due to how FUSE_SUPER_MAGIC is special-cased a few lines down on the same function. In reality, no new attack vectors are introduced, and even that existing one is more theoretical than practical.
How useful is this change on its own? For example the document portal would be still unusable for non-flatpak apps.
The settings portal becomes usable, so applications can respect dark/light theme.
Other settings (like fonts and themes) would be readable by applications too, though my main target here is the color-preference one, since it will avoid applications running in light mode in the middle of the night.