netblue30 / firejail

Linux namespaces and seccomp-bpf sandbox
https://firejail.wordpress.com
GNU General Public License v2.0
5.69k stars 557 forks source link

Wayland security context support #5883

Open WhyNotHugo opened 1 year ago

WhyNotHugo commented 1 year ago

Is your feature request related to a problem? Please describe.

security-context is a wayland protocol that can be used by clients to create a new wayland socket. The same compositor instance will listen for connections on this sockets, but clients that connect to it cannot perform privileged operations.

Describe the solution you'd like

Firejail should use this protocol to further sandbox clients and prevent them from screenscrapping, creating transparent fullscreen overlays and permanent keyboard hijacking, other attacks. All these attack rely on "priviledged" protocols and are unavailable on sockets created via security-context.

Describe alternatives you've considered

So far there hasn't been a way to do this. This protocol was introduced in wayland-protocols 1.32 which was released yesterday.

sway already has a patch with a working server implementations. I expect other wlroots compositors to follow suit.

Additional context

Shameless plug: I wrote a tiny cli client that creates a new socket at given path: https://git.sr.ht/~whynothugo/way-secure

This could be called on the host to create a socket inside the sandbox's $XDG_RUNTIME_DIR.

emersion commented 1 year ago

FWIW, the Flatpak impl: https://github.com/flatpak/flatpak/pull/4920

FelixPehla commented 1 month ago

This protocol is already implemented by Kwin and sway.

I am interested in trying to implement this but before I start I would like to ask some general questions about how this should be implemented and configured.

By looking at how some mechanisms for x11 are implemented:

Do you have any preferences for this or other things I should keep in mind?

FelixPehla commented 1 month ago

One more point I thought of: much like the @x11 group including not just x11 files but also stuff related to GTK, Qt, vulkan, etc. there should probably be a group like @wayland that includes those too. Or alternatively you separate the common graphics stack from x11 and have something like @gui.

What are your thoughts on this @glitsj16 @kmk3 @rusty-snake ?

glitsj16 commented 1 month ago

create a disable-wayland.inc

This seems very doable indeed. I don't think there's anything special here besides implementing and editing the affected profiles and the profile template.

There exists an x11 filter to enable x11 sandboxing, a wayland filter could be used to only allow access to unprivileged protocols.

Personally I like this proposal.

There is a HAVE_X11 flag for compilation with or without x11 support. The same could be done to compile with or without wayland security context support.

Yes, I wouldn't expect anything unusual here. But @kmk3 is much better placed to advise on the particulars involved/potential implementation details to follow etcetera.

If the wayland compositor in use does not implement the protocol, despite wayland-filtering being enabled, should firejail only emit a warning or refuse to start?

This is probably the trickier part of your proposal. I mean, there are pro's and con's here to consider for both suggestions and that's always very difficult to judge. No personal preference.

On the topic of private-etc groups I'll need a bit more time to go over the suggested @wayland or @gui groups. But I can see the importance/gains of adding/improving support for it.

All in all a generous offer to work on this :-)

Regards

rusty-snake commented 1 month ago

If the wayland compositor in use does not implement the protocol, despite wayland-filtering being enabled, should firejail only emit a warning or refuse to start?

This is probably the trickier part of your proposal. I mean, there are pro's and con's here to consider for both suggestions and that's always very difficult to judge. No personal preference.

Consequences of fail is that we can not add it to any profile, hence "normal" users will not benefit from it, only power users who create .locals or add it to globals.local.

If we go with warning/info (I prefer) we can add it to profiles upstream as long as someone tested it with a supporting compositor and normal users can benefit.

For the most features we don't do a hard-fail if the platform requirements are not satisfied. And the feature that have a hard-fail (x11 none) had caused problems and ugly workarounds like disable-X11.inc.

Some features do hard-fail, but only when requested on command-line not when requested by a profile.

disable-wayland.inc

I do not like the include cluttering, we need snippets.

Nevermind, how about

emersion commented 1 month ago

If the wayland compositor in use does not implement the protocol, despite wayland-filtering being enabled, should firejail only emit a warning or refuse to start?

FWIW, Flatpak just carries on without the protocol if the compositor doesn't support it. Do note that Flatpak makes the difference between missing compositor support (global not advertised) and all other kind of errors when setting up the Wayland security context (these are hard failures).

rusty-snake commented 1 month ago

Just FTR: GNOME/Mutter does not implement this because it is secure by default. The Wayland security context idea comes from the Kwin and sway edges where security is opt-in.

emersion commented 1 month ago

Not exactly sure what this comment is about tbh. The security-context protocol allows authenticating clients, and GNOME also needs to do so. Support for the protocol is still WIP in mutter: https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3107

rusty-snake commented 1 month ago

Authentication alone is boring, you need to do something with it. Usually you will do authorisation.

In the context of wl-security-context everybody talks about screen capturing, which can then be deny/allow listed for some clients. However GNOME does not allow you to screen capture with third-party tools.

WhyNotHugo commented 1 month ago

Authentication alone is boring, you need to do something with it. Usually you will do authorisation.

wl-security-context isn't just about authentication. On sway (and likely other compositors too), clients in a security context don't have access to any privileged protocol by default.

If firejail puts its sandboxed clients in a security context, they won't have access to any privileged protocols on compositors that expose them.

While screen capture is the typical example, the following are also considered privileged and not suitable for sandboxed clients:

Plus the following wlroots protocols:

rusty-snake commented 1 month ago

wl-security-context isn't just about authentication. On sway (and likely other compositors too), clients in a security context don't have access to any privileged protocol by default.

While screen capture is the typical example, the following are also considered privileged and not suitable for sandboxed clients

Non of the is implemented by mutter according to https://wayland.app/.

Going back to my comment above. Kwin/sway implement privileged protocols. Hence they need security contexts for sandboxed clients. Mutter does not implement privileged protocol, security contexts are less important.

WhyNotHugo commented 1 month ago

is_privileged() ? deny : allow is authorisation.

Correct, it is.

However instead of this hardcoded(?) policy a compositor could also open a pop-up that asks the user "MyProgram tries to capture your screen. [ALLOW] [DENY]", use polkit for configurable policies.

A compositor could choose to do any of the above if they wanted to. I'm not sure if polkit is the right tool though; it is a system service configured by root. It is usually used to authorise users to run system-wide privileged operations, not for sandboxes to run user-wide privileged operations.

Nevertheless, no compositor does this (or has any plans to do this, AFAIK), so can we agree that it is out-of-scope here?

The specification says only "This is intended to be used by sandboxes. Sandbox engines attach a security context to all connections coming from inside the sandbox. The compositor can then restrict the features that the sandboxed connections can use."

You make a good point here: the protocol specification itself doesn't state "compositors will restrict access to all privileged protocols". In reality, current implementation do restrict them.

There is currently no mechanism to allow a client to access some privileged protocols. AFAIK, there is an intent of implementing one such protocol, but there seems to be no consensus on what it will look like.

Going back to my comment above. Kwin/sway implement privileged protocols. Hence they need security contexts for sandboxed clients. Mutter does not implement privileged protocol, security contexts are less important.

I think it's fair to say "almost every compositor except Mutter", rather than "Kwin/sway".

Mutter handles this out-of-band via D-Bus, so instead of having to sandbox the Wayland connection, you need to sandbox D-Bus instead. This is already implemented in firejail, so you don't need to worry about Mutter.

emersion commented 1 month ago

Note that mutter implements e.g. the global keyboard shortcut inhibitor and this one is more privileged as well.

FelixPehla commented 1 month ago

There is currently no mechanism to allow a client to access some privileged protocols. AFAIK, there is an intent of implementing one such protocol, but there seems to be no consensus on what it will look like.

I believe Weston and Kwin create a new socket with access to a subset of privileged protocols and pass them to an application using $WAYLAND_SOCKET, as per the discussion here.

This should also (optionally) be allowed here I think, maybe with something like wayland socket? I'm not sure about the argument, but that can be hashed out later.

FelixPehla commented 4 weeks ago

I didn't want to discuss the implementation details in an issue, so I've created a new draft here. Feel free to drop by.