flathub / org.chromium.Chromium

https://flathub.org/apps/details/org.chromium.Chromium
43 stars 25 forks source link

Upstream the Flatpak sandbox code #337

Open DemiMarie opened 10 months ago

DemiMarie commented 10 months ago

The Chromium team is probably in a better shape than it was when https://github.com/flathub/org.chromium.Chromium/issues/43#issuecomment-748543603 was written. Furthermore, this would allow Electron apps to work normally under flatpak without requiring Zypack. I suspect this is what would be needed for apps like Element to ship an official flatpak.

cjao commented 3 months ago

Here's an interesting thread about Chromium's sandbox strength when modified to work with flatpak. Note in particular:

Pls check the namespace structure of Flatpak’s Chromium vs the official native Chrome with lsns -T. You will see a noticable difference and weakened site isolation. The native Chrome namespace structure is more fine-grained. Thus leading to better horizontal isolation.

Is this accurate? If so, is it a limitation of flatpak or just of this particular adaptation of Chromium to Flatpak?

@refi64

user1-github commented 1 month ago

I'm also all in favor of upstreaming the Flatpak sandbox code.

However, since we're started talking about the Flatpak sandbox quality, here is another interesting comment by Cromite browser dev questioning the sandbox quality of the patch after comparing it to the non Flatpak version of the browser. The next few comments are also interesting.

Basically, their main finding is that the browser's renderer process isn't properly sandboxed with the Flatpak sandbox patch. Is this a limitation of running Chromium browsers in Flatpak, or are they interpreting their findings wrong and the renderer process is actually properly sandboxed?

DemiMarie commented 1 month ago

Chromium’s sandbox is designed for Chromium and so is stronger than what Flatpak currently offers. This could be fixed on the Flatpak side.

cjao commented 1 month ago

Is it enough to provide a flatpak option to allow user namespace creation?

cjao commented 1 month ago

Basically, their main finding is that the browser's renderer process isn't properly sandboxed with the Flatpak sandbox patch. Is this a limitation of running Chromium browsers in Flatpak, or are they interpreting their findings wrong and the renderer process is actually properly sandboxed?

Here is another interesting thread discussing how putting Chromium in flatpak may weaken its sandbox.

DemiMarie commented 1 month ago

Is it enough to provide a flatpak option to allow user namespace creation?

This would be a security vulnerability in Flatpak. Flatpak portals check /proc/$PID/root to determine which flatpak made a D-Bus API call. It would also massively increase kernel attack surface.

The proper solution is for Flatpak to provide a sandbox that is as tight as Chromium wants it to be. This will need to be opt-in on the part of applications, but also only very few applications can run with a sandbox that is as tight as that of the Chromium render process.

DemiMarie commented 1 month ago

Further thought: Could Chromium use Landlock to sandbox itself adequately? Landlock requires no privileges to use at all, nor does it rely on namespaces. It is much more similar to how macOS sandboxing works.

cjao commented 1 month ago

Is it enough to provide a flatpak option to allow user namespace creation?

This would be a security vulnerability in Flatpak. Flatpak portals check /proc/$PID/root to determine which flatpak made a D-Bus API call. It would also massively increase kernel attack surface.

The proper solution is for Flatpak to provide a sandbox that is as tight as Chromium wants it to be. This will need to be opt-in on the part of applications, but also only very few applications can run with a sandbox that is as tight as that of the Chromium render process.

Would it be a greater security vulnerability than --filesystem=host or the other holes that one can already poke in the sandbox? Those options support major use cases:

An option like --allow-userns would cater to a fourth category:

For comparison, Ubuntu snap provides a custom security profile specifically designed to support browsers that bring their own sandbox. In particular, this profile whitelists user namespaces. This allows them to ship Chromium using snap without modifying Chromium's battle-tested sandbox.

DemiMarie commented 1 month ago

Is it enough to provide a flatpak option to allow user namespace creation?

This would be a security vulnerability in Flatpak. Flatpak portals check /proc/$PID/root to determine which flatpak made a D-Bus API call. It would also massively increase kernel attack surface. The proper solution is for Flatpak to provide a sandbox that is as tight as Chromium wants it to be. This will need to be opt-in on the part of applications, but also only very few applications can run with a sandbox that is as tight as that of the Chromium render process.

Would it be a greater security vulnerability than --filesystem=host or the other holes that one can already poke in the sandbox? Those options support major use cases:

  • Apps that can but have yet to opt into the portal APIs
  • Apps for which the portal APIs are insufficient
  • Apps that are not practical to be confined, such as IDEs which generally expect full access to the host system (especially if e.g. one is doing low-level development).

I would argue that IDEs can practically be confined, and that Qubes OS does this all the time. Of course, it probably helps that that the confinement is implemented by giving the IDE a whole VM.

An option like --allow-userns would cater to a fourth category:

  • Apps that integrate their own custom sandboxes.

For comparison, Ubuntu snap provides a custom security profile specifically designed to support browsers that bring their own sandbox. In particular, this profile whitelists user namespaces. This allows them to ship Chromium using snap without modifying Chromium's battle-tested sandbox.

This is an acceptable stopgap, but it means that the browser process is either unconfined or much less confined, which means that the system is exactly as secure as if a flatpak was not used. Providing a sandbox that is as strong as Chromium’s is the best solution, but is also the most work.

cjao commented 1 month ago

This is an acceptable stopgap, but it means that the browser process is either unconfined or much less confined, which means that the system is exactly as secure as if a flatpak was not used.

This is true. However, Chromium is a bit of a special case because it's basically the gold standard for application sandboxing, whereas the builtin Flatpak sandbox is designed for programs that don't already have internal security mechanisms.

Even if Flatpak does not add a security boundary, the ability to bundle dependencies would still have independent value. In fact, some blog posts early in the development of flatpak suggest that bundling was the initial focus.

Erick555 commented 1 month ago

Chromium has per-process sandboxing which protect processes from each other. Flatpak sandbox protects host from the whole app environment. They're different layers and orthogonal which is why flatpak will never replace chromium sandboxing. The problem is flatpak also blocks creating of chromium own sandbox which needs workaround.

DemiMarie commented 1 month ago

Chromium has per-process sandboxing which protect processes from each other. Flatpak sandbox protects host from the whole app environment. They're different layers and orthogonal which is why flatpak will never replace chromium sandboxing. The problem is flatpak also blocks creating of chromium own sandbox which needs workaround.

On Linux, a sandboxed process will not generally have the privileges to create sub-sandboxes. That’s why I think it is better for Flatpak to have a special case that creates an ultra-strict sandbox, even if Chromium is the only one to use it.

Leaving the browser process unsandboxed is reasonable (if suboptimal) for Chromium itself, but not for Electron apps.

Erick555 commented 1 month ago

For chromium you need many sandboxes not one and there is no one ultra-strict sandbox that will work for all chromium process (i.e. some require network access and some don't).

Without flatpak chromium parent process creates sandboxes for its child processes. In flatpak chromium parent process must ask flatpak to crate sandbox for child on its behalf.

Flatpak can't manage chromium processes and chromium parent can't create subsandboxes so they need to cooperate i.e. through flatpak-spawn api which is done atm. There is no replacement one by the other in cards.

DemiMarie commented 1 month ago

Can flatpak-spawn be made flexible enough for Chromium’s needs?

Erick555 commented 1 month ago

I don't know. Maybe. This is something to delve into when someone starts upstreaming.

cjao commented 2 weeks ago

The lead developer of cromite recently investigated the Flatpak sandbox adapter used by Chromium and concluded that

that patch is highly insecure and should not be used.

Of course, flatpak is effective in the process and disk sandbox. but, somehow, it seems to reduce the rendering process sandbox, the most important one in chromium. the objective of chromium is to reduce the possibilities for websites, in the event of security holes, to tamper with the user's browsing, or to steal it. this is why it is important that the sandbox remain as defined by the chromium security team, or improve it. From what it seems to me, but mind you, it seems to me, flatpak is fine for isolating applications that do not have a sandbox of their own, but perhaps not suitable for use in more sophisticated applications.

user1-github commented 2 weeks ago

The lead developer of cromite recently investigated the Flatpak sandbox adapter used by Chromium and concluded that

That's the comment I linked to in my previous post about a month ago.

I agree with rany2's comment in that thread - as you can see, different devs interpret the results differently. That's why I think it's worth trying to submit upstream the Flatpak sandbox patch, so that Chromium devs who actually understand how browser sandboxing should work will properly review and scrutinize the sandbox patch.

rusty-snake commented 2 weeks ago

that patch is highly insecure and should not be used.

I concluded that over two years ago, https://github.com/madaidans-insecurities/madaidans-insecurities.github.io/issues/62, so it is really something we know and life with.

Also see https://github.com/flatpak/flatpak/issues/5879#issuecomment-2255568180

DemiMarie commented 2 weeks ago

Could Landlock be a solution to this? “Running in a filesystem namespace with no access to anything” should be equivalent to “running with a Landlock policy that blocks all filesystem operations.” The latter requires no privileges at all. Landlock cannot currently reject all filesystem operations.

rusty-snake commented 2 weeks ago

Well, the difference between an empty mount-namespace (what I guess you mean with filesystem namespace) and blocking all filesystem related syscalls (seccomp) is not huge.

DemiMarie commented 2 weeks ago

@rusty-snake Indeed so. Flatpak imposes no restrictions on seccomp or Landlock, and if Chromium was able to adequately sandbox itself without namespaces the problem would go away.

rusty-snake commented 2 weeks ago

To me this sounds like adding a --add-seccomp-fd argument to flatpak-spawn (flatpaks sub-sandbox interface/tool) like flatpak-spawn --sandbox --no-network --add-seccomp-fd={FD} would significantly increase the sub-sandboxing capabilities.

refi64 commented 2 weeks ago

I posted a reply on the cromite thread. With regard to the comments here...most of this literally already exists, as-is Chromium's layer 2 sandbox uses seccomp, and there is work towards using landlock.

Basically everything outlined under --add-seccomp-fd is also already a thing that's actively used.

rusty-snake commented 2 weeks ago

So what's missing then in flatpaked chromium browsers?

DemiMarie commented 2 weeks ago

I posted a reply on the cromite thread. With regard to the comments here...most of this literally already exists, as-is Chromium's layer 2 sandbox uses seccomp, and there is work towards using landlock.

Did you mean to link to something else? The link you posted points back at this issue.

refi64 commented 2 weeks ago

once again I prove to myself that writing issue comments at 9pm is a mistake, correct link to the landlock support issue.

robertwthomson commented 1 week ago

there is work towards using landlock.

More like the start of a discussion than work, though