Allow portal access non-flatpak sandboxes

[WhyNotHugo]

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 #737

[hadess]

Looking at the diff in github, and there's a severe indentation problem. There's also typos in the comments.

[WhyNotHugo]

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.

[hadess]

Does that cover the indentation problem you mention?

No.

[WhyNotHugo]

Fixed. 🤦‍♂️

[WhyNotHugo]

Rebased to main.

[mwleeds]

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.

[WhyNotHugo]

Ah, yes, not I see it. I believe this version should be correct.

[mwleeds]

Yeah the indentation looks correct. I don't know whether the code changes are correct.

[GeorgesStavracas]

I'd really love to hear from @alexlarsson, but meanwhile, here are a few stylistic comments

[WhyNotHugo]

All updated.

[alexlarsson]

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?

[WhyNotHugo]

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.

[alexlarsson]

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-portal deal 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).

[swick]

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.

[WhyNotHugo]

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.

[WhyNotHugo]

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.

[WhyNotHugo]

Being able to use the OpenURL portal for sandboxed applications would also be very useful.

Can this be merged in? The portal fits perfect for handling this kind of situation in sandboxes, but is currently unusable due to this issue.

[hadess]

Can this be merged in? The portal fits perfect for handling this kind of situation in sandboxes, but is currently unusable due to this issue.

It's pretty clear that no one is particularly happy with those changes, and nobody wants to own up to merging it. I guess "it's the same security issue that already exists" isn't a particularly great advert for the changes, and the majority of folks maintaining xdg-desktop-portal probably don't care that much about non-Flatpak sandboxes.

I honestly think you should change adapt your non-Flatpak sandbox to xdg-desktop-portal, rather than the opposite.

[WhyNotHugo]

I guess "it's the same security issue that already exists" isn't a particularly great advert for the changes

This could also be rephrased as "does not introduce any new security issues". It's mostly vulnerable to an existing theoretical one, but fixing an existing [theoretical] attack is out-of-scope for this change.

and the majority of folks maintaining xdg-desktop-portal probably don't care that much about non-Flatpak sandboxes.

I think the maintainers need to voice their position here. If xdp is to be flatpak-and-snap-specific, then please state so, so that applications can understand what their runtime dependencies are.

I honestly think you should change adapt your non-Flatpak sandbox to xdg-desktop-portal, rather than the opposite.

There's no obvious way to adapt other sandboxes "to look like Flatpak". Firejail does this differently, and making it fit into xdp seems like it would require rewriting its entire filesystem management. This also makes it near-impossible to innovate with new sandboxing mechanism that diverge at all from what Flatpak does.

It would honestly be less effort to fork xdp itself, and fix the issues in forks -- but creating a fork comes with its own huge pile of maintenance issues.

---

The biggest part of the conflict here is fulfilling two distinct roles:

1. Provides access to portals in a desktop-agnostic fashion.
2. Attempts to authenticate and sandbox clients via flatpak-specific mechanism.

Even naming them makes the conflict of goals evident here: xdp implements sandboxing-related bits, which really seem out-of-scope for a portal that presents itself as desktop agnostic and supporting "other desktop containment frameworks". AND these sandboxing bits are implemented in a Flatpak-specific (and Snap-specific) way. This PR adds support for Firejail (to a degree), but seems to have been implicitly refused so far).

On top of that, the authentication mechanism is proved to be a terrible security approach (see multiple comments above). Attempting to identify applications at runtime to determine their sandbox level is a known bad approach. If a sanboxed application has an unproxied unfiltered connection to the xdp, then it should considered to be unsandboxed and have unlimited access, rather than try and guess what its sandbox level should be.

As a reference, see how xdg-dbus-proxy or Wayland's security-context WIP protocol work. Sandboxed clients don't get a regular socket to a service (in this case, the dbus-broker), but go through an intermediate that either does the filtering, or yields a new socket with restricted permissions. This is what Flatpak be doing, and the check we're discussing here in the XDP is just a hack.

[Erick555]

I honestly think you should change adapt your non-Flatpak sandbox to xdg-desktop-portal, rather than the opposite.

It was mentioned earlier in discussion that it's not possible in current state as many things in x-d-p are hardcoded around flatpak/snap (i.e. document portal) and don't accept anything else. Some sandbox agnostic changes in portals are necessary and only after that other sandboxes can be adjusted to work with them.

[WhyNotHugo]

Flatpak uses the xdg-dbus-proxy, right? The proxy allows filtering to which objects a sand-boxed application can talk. Why does Flatpak not rely on this, instead of having the Flatpak-internal permission check built in?

E.g.: assume that all connections come via a portal; anything else is a privileged/unsandboxed client.

[rusty-snake]

The proxy allows filtering to which objects a sand-boxed application can talk.

Flatpak only exposes the functionality to filter the names you can talk to but no filtering of objects/interfaces.

[rusty-snake]

Flatpak uses the xdg-dbus-proxy, right?

Well, you can give it --socket=session-bus permission.

[WhyNotHugo]

Well, you can give it --socket=session-bus permission.

Yeah, but if you do, you should assume the sandboxing game is over (e.g.: like filesystem=host). I don't think the xdp should try and shield this scenario TBH.

[WhyNotHugo]

This issue continues to be super annoying. Sandboxed applications can't talk to the portal at all.

Things like chromium now attempt to use the portal, so can't even shot a file picker because of this bug.

It's ridiculous how this PR has been stalled for no good reason, while the issue just spills all over so many other applications.

I wish we had an xdg-desktop-portal implementation that wasn't broken for Linux desktop, that would unbreak SO many things.

[Erick555]

I'm curious: for what chromium attempts to use the portal?

[rusty-snake]

@Erick555 native file-chooser I guess. Try Ctrl-O.

[WhyNotHugo]

native file-chooser I guess. Try Ctrl-O.

Exactly this

[Erick555]

But this is only for better looking file dialog not actual file permissions (as those only work in flatpak or snap), right?

[mwleeds]

Posting regular comments asking for this to be merged is not going to be as productive as working on solving the hard problems here.

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.

I think you're right about pid re-use to a fuse rootfs process being an existing possible attack, though it might not necessarily be as easy to pull off as a pid re-use to a process that returns EACCES from a root statfs. More importantly, there's still a question of whether pid re-use is the only possible way a Flatpak/Snap could trigger the EACCES; see:

The question is, did I miss some other possible way to make the stat() call return EACESS?

If there is another way then this patch would be opening a new security hole.

A few more comments:

1. Both your commit message and code comments seem to imply that x-d-p's job is to do D-Bus filtering which is not true. At the very least those need rewording.
2. If I understand correctly, with this patch, firejailed processes are treated as unsandboxed processes by x-d-p which means they are granted access to resources that sandboxed processes wouldn't necessarily have. Is there a chance they would be given access to something by x-d-p that firejail has restricted them from? If so, that would seem to be undesirable.
3. With this patch, what is the behavior of firejailed processes that try to use the document portal? Does it function correctly?

[rusty-snake]

Does it function correctly?

No because ${XDG_RUNTIME_DIR}/doc

• is blacklisted for the most programs and for programs Luke chromium it is hidden. (This can be change if there is the need for it but ATM we blacklist all flatpak related directories in XRD).
• is a FUSE filesystem AFAIK which means it must be mounted with allow_root/allow_other in order to work with firejail.

Maybe you can get it to work, but I don't see it to work out-of-the-box any time soon. Or am I miss something here.

[WhyNotHugo]

I think you're right about pid re-use to a fuse rootfs process being an existing possible attack, though it might not necessarily be as easy to pull off as a pid re-use to a process that returns EACCES from a root statfs. More importantly, there's still a question of whether pid re-use is the only possible way a Flatpak/Snap could trigger the EACCES; see:

This keeps being brought up over and over again here. The issue exists on master and current releases. This patch does not extend this attack surface, it merely let it continue being there.

I suggest opening a separate issue to discuss it. I don't see the point in blocking this PR because of this issue. Also, the issue can be addressed entirely separately, though, as described above, is more of a design issue than anything else.

[mwleeds]

@WhyNotHugo your comment refers to "this issue" which I assume is about pid re-use with a fuse root filesystem, but my comment was also a discussion of two other issues: pid re-use that results in statfs EACCES, and potentially other ways to cause statfs EACCES other than pid re-use. It seems like you're not acknowledging those issues, and they are new with this PR, not pre-existing.

[WhyNotHugo]

The whole mechanism of "try and guess what application is talking to the server" after the connection has been established is flawed and broken, as discussed widely above on this thread. The race condition you mention is just a variation of an existing vector for an already broken check. Do you see the else on that if? It has an extremely similar condition that can also be exploited the same way. But it seems that almost identical security issue isn't important, since nobody's cared to address it in the five months since this PR was opened (even though it's been mentioned a few times).

On top of that, why would the XDP even care about this kind of check? An application that's managed to talk to it directly has already broken out of the sandbox (or bypassed the xdg-dbus-proxy, or been given unrestricted access).

In the meantime, a sandboxed chromium can't even render a file picker dialog because of this bug. So many basic things continue breaking due to this that it's ridiculous.

[mwleeds]

On top of that, why would the XDP even care about this kind of check? An application that's managed to talk to it directly has already broken out of the sandbox (or bypassed the xdg-dbus-proxy, or been given unrestricted access).

I just checked experimentally with gdb and the pid that gets passed to xdp_get_app_info_from_pid() by xdp_connection_lookup_app_info_sync() is the pid of the sandboxed Flatpak app process, not xdg-dbus-proxy. So that's why this security check matters.

[WhyNotHugo]

On Sun, 11 Sep 2022, at 03:29, Phaedrus Leeds wrote: > On top of that, why would the XDP even care about this kind of check? An application that's managed to talk to it directly has already broken out of the sandbox (or bypassed the xdg-dbus-proxy, or been given unrestricted access). > Incorrect. All Flatpak or Snap applications are allowed to talk to x-d-p over D-Bus. There is no sandbox escape required.

This sounds like a security bug in itself already. You want to sandbox applications first, and only allow communicating via filtered channels. Especially to sensitive services like XDP. Exposing a sandboxing application openly to XDP and having XDP do the Flatpak-specific checks is a hack and mixes responsibilities (XDP is suddenly both the portal implementation AND part of the Flatpak sandboxing implementation). The Flatpak/proxy integration seems to need a lot of work still, but pushing responsibility into XDP isn't a good idea; an already-very-complex tool which has this many privileges shouldn't be also doing some unrelated sandboxing. Perhaps we should start opening individual tickets for all the various security issues that we continue to enumerate in this issue.

[swick]

I'm not sure what you're trying to achieve here. We know that this isn't the ideal way but it does work for flatpak and snap right now. You also know what has to happen to improve the situation in a fundamental way (#741 (comment)).

You have exactly two options: figure out how to authenticate connections from your sanboxing mechanism in xdp without breaking it for the existing mechanisms or work on dbus so that it handles authentication and we can remove the sandbox-specific authentication code from xdp.

Complaining about how this is all bad and horrible doesn't help.

[Erick555]

I think the point is xdp is essentially blocklisting firejail and similar sandbox types. There is no workaround for that on firejail side.

[rusty-snake]

There is no workaround for that on firejail side.

It works with firejail if you do not use noroot (i.e. new userns). The reason for this is that firejail creates the userns as root ATM which makes it owned by root. As a consequence can x-d-p (running as user) not read/dereference /proc/<pid>/root.
TL;DR: The fix on firejail side is to use user owned userns.

I'm not saying that there is no need for structural improvements in x-d-p (which in part require a better Linux) and it does not fix it for other sandboxes, but there are workarounds.

[mwleeds]

You have exactly two options: figure out how to authenticate connections from your sanboxing mechanism in xdp without breaking it for the existing mechanisms or work on dbus so that it handles authentication and we can remove the sandbox-specific authentication code from xdp.

To expand on the first option a bit, I think the paths forward are either (1) make a convincing argument that the attack surface area available to sandboxed processes is not widened by treating EACCES from statfs as unsandboxed, in which case this patch could be merged as is, or (2) find a more robust mechanism for detecting firejail'd processes, perhaps with changes to firejail.

[uriesk]

I tried to use xdg-desktop-portal with an application that is simply running as a different user and got given dbus access via policies, and it's not working.

[WhyNotHugo]

@uriesk please open a separate issue, you situation may or may not be related to the issue at hand. You'll need to explain how you granted access "via policies" and what "not working" means.