Add support for Wayland security context #4920

emersion · 2022-05-31T12:28:21Z

This exposes a reliable way for Wayland compositors to get
identifying information about a client. Compositors can then
apply security policies if desirable.

~~TODO: bwrap --sync-fd can only be specified once (only the last param is taken into account), but D-Bus already uses it. Any ideas how to address this?~~

References: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/68

common/flatpak-run.c

emersion · 2022-06-10T07:25:34Z

Gentle ping

common/flatpak-run.c

swick · 2022-06-10T15:41:19Z

@smcv might also be interested

jadahl · 2023-06-15T10:04:22Z

common/flatpak-run.c

@@ -487,7 +488,8 @@ flatpak_run_add_environment_args (FlatpakBwrap *bwrap,
 flatpak_context_append_bwrap_filesystem (context, bwrap, app_id, app_id_dir,
 exports, xdg_dirs_conf, home_access);

- flatpak_run_add_socket_args_environment (bwrap, context->shares, context->sockets);
+ if (instance_id)
+ flatpak_run_add_socket_args_environment (bwrap, context->shares, context->sockets);
 flatpak_run_add_session_dbus_args (bwrap, proxy_arg_bwrap, context, flags, app_id);
 flatpak_run_add_system_dbus_args (bwrap, proxy_arg_bwrap, context, flags);
 flatpak_run_add_a11y_dbus_args (bwrap, proxy_arg_bwrap, context, flags);


I wonder if any of these needs to be added for apply_extra_data() (which is where instance_id can be NULL).

Mh, I would even go so far and say it also should not get access to IPC, /dev, dri and shared tmp.

Probably makes sense to separate into a new PR.

Generally these are no-ops due to the passed in run_flags (e.g. with FLATPAK_RUN_FLAG_NO_SESSION_BUS_PROXY, etc), and the empty app_context that apply_extra_data uses.

Do you have any particular place where it gets this wrong?

In fact, is this check even needed? I don't see how sockets & FLATPAK_CONTEXT_SOCKET_WAYLAND would be set in apply_extra_data?

In fact, isn't it a bad idea to not call flatpak_run_add_socket_args_environment()?
We actively do some things like cover-mount /tmp/.X11, etc, if the x11 flag is not set. So maybe this will actually limit our security measures?

I see. So instead, would an assert that the instance is not NULL be welcome when setting up Wayland sockets?

In the end, what we don't is setting up Wayland socket when we don't have an instance id, which appears to be the case when we arrive here from apply_extra_data().

I see. So instead, would an assert that the instance is not NULL be welcome when setting up Wayland sockets?

I guess this makes sense. Assert that instance ID is set if sockets & FLATPAK_CONTEXT_SOCKET_WAYLAND.

I looked into this as well on Friday and noticed that the run flags are not what makes this work. The context doesn't load metadata from anywhere so none of the sockets will get set up.

The --sandboxed argument to flatpak run was added after the apply_extra_data sandbox and does almost the same but not entirely. It does add FLATPAK_RUN_FLAG_NO_PROC as well and I can't tell why and we don't call flatpak_context_make_sandboxed which is fine because we don't load anything else in the context but might be a good idea to call anyway.

Wanted to test this but a lot of tests are failing for me on main because of #5027.

--- a/common/flatpak-dir.c +++ b/common/flatpak-dir.c @@ -8328,20 +8328,25 @@ apply_extra_data (FlatpakDir *self, "--cap-drop", "ALL", NULL); - /* Might need multiarch in apply_extra (see e.g. #3742). - * Should be pretty safe in this limited context */ - run_flags = (FLATPAK_RUN_FLAG_MULTIARCH | + /* Run flags which equal --sandboxed */ + run_flags = (FLATPAK_RUN_FLAG_SANDBOX | FLATPAK_RUN_FLAG_NO_SESSION_HELPER | - FLATPAK_RUN_FLAG_NO_PROC | FLATPAK_RUN_FLAG_NO_SESSION_BUS_PROXY | FLATPAK_RUN_FLAG_NO_SYSTEM_BUS_PROXY | FLATPAK_RUN_FLAG_NO_A11Y_BUS_PROXY); + /* Might need multiarch in apply_extra (see e.g. #3742). + * Should be pretty safe in this limited context. + * Removing /proc to tighten sandbox even more I guess? */ + run_flags |= FLATPAK_RUN_FLAG_NO_PROC | + FLATPAK_RUN_FLAG_MULTIARCH; + if (!flatpak_run_setup_base_argv (bwrap, runtime_files, NULL, runtime_arch, run_flags, error)) return FALSE; app_context = flatpak_context_new (); + flatpak_context_make_sandboxed (app_context); if (!flatpak_run_add_environment_args (bwrap, NULL, run_flags, id, app_context, NULL, NULL, -1,

Created #5466 to maybe discuss this further.

Assert that instance ID is set if sockets & FLATPAK_CONTEXT_SOCKET_WAYLAND

Yeah.

We absolutely don't want NO_PROC. That will break any apps that uses /proc/self/fd, etc which is very common (even used by glibc). And /proc access is not a security problem for regular apps. Only for apply_extra_data() when writing to the system repo. See commit message for cd21428.

And flatpak_context_make_sandboxed() is a complete no-op for the newly created (and thus empty) app_context.

emersion · 2023-06-15T10:06:23Z

Rebased, added Meson support, made instance ID mandatory.

davidedmundson · 2023-06-22T15:02:42Z

Can I check that this MR is "acked in principle" and just blocked on the upstream protocol to land?

Obviously it's chicken and egg, but it's also silly to land this in the specs without the most important user of this being officially on-board.

jadahl · 2023-06-22T15:22:17Z

Can I check that this MR is "acked in principle" and just blocked on the upstream protocol to land?

There has been no explicit ack from a maintainer yet, but it got some initial review from @alexlarsson. Something more would be appreciated no doubt.

emersion · 2023-06-28T08:02:26Z

Any news about this?

alexlarsson

LGTM

emersion · 2023-07-03T09:05:16Z

Thank you for having a look, @alexlarsson!

jadahl · 2023-07-03T09:10:47Z

Next steps: release wayland-protocols with the new protocol, and bump the version needed by flatpak if enabled.

alexlarsson · 2023-07-03T09:40:22Z

Actually, as per the comment above. I think not calling flatpak_run_add_socket_args_environment() is a bad idea.

jadahl · 2023-07-03T10:30:09Z

Next steps: release wayland-protocols with the new protocol, and bump the version needed by flatpak if enabled.

Can now depend on 1.32 for this.

emersion · 2023-07-03T14:41:00Z

Added an assert and the wayland-protocols version requirement.

common/flatpak-run-wayland.c

.github/workflows/check.yml

swick · 2023-07-04T16:14:48Z

LGTM

common/flatpak-run-wayland.c

emersion · 2023-07-11T17:57:33Z

@alexlarsson, does this look good to you?

emersion · 2023-07-18T13:55:50Z

@alexlarsson, anything else I need to do or does this look good to you?

emersion · 2023-07-30T08:28:29Z

@alexlarsson, any news about this?

common/flatpak-run-wayland.c

emersion · 2023-08-24T06:30:03Z

@alexlarsson, any chance to get this merged?

alexlarsson

Some very minor indentation issues, but otherwise this looks good to me,

common/flatpak-run-wayland.c

orowith2os · 2023-08-24T08:11:01Z

common/flatpak-run-wayland.c

+#ifdef ENABLE_WAYLAND_SECURITY_CONTEXT
+ gboolean security_context_available = FALSE;
+ if (flatpak_run_add_wayland_security_context_args (bwrap, app_id, instance_id,
+ &security_context_available))
+ return TRUE;
+ if (security_context_available)
+ return FALSE;
+#endif /* ENABLE_WAYLAND_SECURITY_CONTEXT */


I'd appreciate a bit of an explanation on what's going on here.

AIUI, this would mean not running with Wayland at all, if a security_context isn't available, thus running on X11; why is that desired? You'd be breaking things like fractional scaling and mixed monitor scales when doing so. And even a Wayland socket without a security_context is leaps better than an X11 socket on its own.

If security contex is available, we should generally never reach this, only if there is some weird error. And in that case the idea is to prefer not to use wayland.

I'm still not sure on why we'd not prefer to use Wayland. What if a compositor doesn't support the protocol (like an old Mutter) but doesn't provide any iffy protocol extensions? Then this would mean all Flatpak apps run on X11 (if Flatpak is built with security_context support), which would result in a degraded UX.

It seems more appropriate to log the issue and continue on.

If the compositor doesn't support the protocol, the current logic will just set up a regular Wayland connection. No X11 used.

I've added a comment to make it clearer why the check is written this way.

It seems the general flow of the code was confusing me. There's flatpak_run_add_wayland_security_context_args, which does all of the security_context and wayland socket setup, and flatpak_run_add_wayland_args, which does the normal wayland stuff.

flatpak_run_add_wayland_args calls flatpak_run_add_wayland_security_context_args to completely set up the socket from here. If the protocol is available but failed to be set up, it'll bail out, running on X11. I would still prefer that to instead log the error and run on Wayland, but w/e.

But I'm not understanding how the rest of the flatpak_run_add_wayland_args function is called if the compositor doesn't support the protocol. Is the rest of the function that falls back to that behavior done in flatpak_run_add_wayland_security_context_args? If so, would be nice to document that.

The confusion comes from the variable being named security_context_available - the final if check on it makes it sound like it's "if security context is available, fail to set up Wayland", when in reality it's just returning the initial value of the variable, FALSE.

It would be nice if the variable could be set to TRUE initially instead, the security context setup has an else case that sets it to FALSE, and then the final check goes to see if the variable is not true, and returns FALSE in that case. That would make it flow a bit nicer.

Also documenting the functions, their flow, and their behavior would be nice, to make it so conversations like this don't crop up again. I don't want this to be a blocker here, so if I can make it better myself, I can follow up with it in a later PR.

Sorry, I don't have plans to address your comments. I think the current logic is fine, especially with the comment.

The comment does help, and at most, I'd make it a tiny bit more explicit in the event the compositor doesn't support it:

/* If security-context is available but we failed to set it up, bail out. * If the compositor doesn't support security-context, we'll just * set up a plain Wayland socket. */

That's just repeating what the code already say.

It's well known convention in flatpak that returning true means "success", so if something say

if (try_security_context(&was_it_even_there)) return true; if (was_it_even_there) return false;

is very clear. The only question is "why", not "what", which could just as well be in the commit message.

The same logic will be used for Wayland security context.

More complicated setup logic will be added next commit.

emersion · 2023-08-24T08:17:33Z

Indentation now fixed!

This exposes a reliable way for Wayland compositors to get identifying information about a client. Compositors can then apply security policies if desirable. See: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/68

emersion · 2023-08-24T10:24:51Z

Thanks a lot!

alexlarsson reviewed Jun 3, 2022

View reviewed changes

common/flatpak-run.c Outdated Show resolved Hide resolved

alexlarsson reviewed Jun 3, 2022

View reviewed changes

common/flatpak-run.c Outdated Show resolved Hide resolved

emersion force-pushed the wl-security-context branch from c9f8645 to 2a4476a Compare June 3, 2022 12:19

swick reviewed Jun 10, 2022

View reviewed changes

common/flatpak-run.c Outdated Show resolved Hide resolved

emersion force-pushed the wl-security-context branch 2 times, most recently from be03687 to c2823de Compare October 5, 2022 12:57

emersion force-pushed the wl-security-context branch 5 times, most recently from 40a3a0d to f07412d Compare June 15, 2023 10:04

jadahl reviewed Jun 15, 2023

View reviewed changes

emersion force-pushed the wl-security-context branch 2 times, most recently from b41dd86 to 643e6fa Compare June 15, 2023 10:40

alexlarsson approved these changes Jul 3, 2023

View reviewed changes

emersion force-pushed the wl-security-context branch from f3c2aa8 to e817655 Compare July 3, 2023 14:40

swick reviewed Jul 3, 2023

View reviewed changes

common/flatpak-run-wayland.c Outdated Show resolved Hide resolved

alexlarsson mentioned this pull request Jul 3, 2023

dir: Set up the apply_extra sandbox like flatpak-run --sandbox #5466

Merged

swick reviewed Jul 3, 2023

View reviewed changes

.github/workflows/check.yml Outdated Show resolved Hide resolved

emersion force-pushed the wl-security-context branch 2 times, most recently from d48b00d to 6010dc1 Compare July 4, 2023 16:09

emersion force-pushed the wl-security-context branch from 6010dc1 to ee96067 Compare July 4, 2023 16:19

swick reviewed Jul 6, 2023

View reviewed changes

common/flatpak-run-wayland.c Show resolved Hide resolved

emersion mentioned this pull request Jul 11, 2023

Wayland security context support netblue30/firejail#5883

Open

zccrs reviewed Aug 2, 2023

View reviewed changes

common/flatpak-run-wayland.c Show resolved Hide resolved

alexlarsson approved these changes Aug 24, 2023

View reviewed changes

common/flatpak-run-wayland.c Outdated Show resolved Hide resolved

common/flatpak-run-wayland.c Outdated Show resolved Hide resolved

orowith2os reviewed Aug 24, 2023

View reviewed changes

emersion added 2 commits August 24, 2023 10:15

common: introduce flatpak_bwrap_add_sync_fd

7db6276

The same logic will be used for Wayland security context.

common: split Wayland socket setup to separate file

ec77874

More complicated setup logic will be added next commit.

emersion force-pushed the wl-security-context branch from ee96067 to a8192c1 Compare August 24, 2023 08:17

emersion force-pushed the wl-security-context branch from a8192c1 to 6d3188f Compare August 24, 2023 08:21

alexlarsson merged commit f0e626a into flatpak:main Aug 24, 2023
9 checks passed

emersion deleted the wl-security-context branch August 24, 2023 10:24

smcv mentioned this pull request Aug 31, 2023

Clean up build system integration for Wayland security contexts #5507

Merged

Add support for Wayland security context #4920

Add support for Wayland security context #4920

Conversation

emersion commented May 31, 2022 • edited

emersion commented Jun 10, 2022

swick commented Jun 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersion commented Jun 15, 2023

davidedmundson commented Jun 22, 2023

jadahl commented Jun 22, 2023

emersion commented Jun 28, 2023

alexlarsson left a comment

Choose a reason for hiding this comment

emersion commented Jul 3, 2023

jadahl commented Jul 3, 2023

alexlarsson commented Jul 3, 2023

jadahl commented Jul 3, 2023

emersion commented Jul 3, 2023

swick commented Jul 4, 2023

emersion commented Jul 11, 2023

emersion commented Jul 18, 2023

emersion commented Jul 30, 2023

emersion commented Aug 24, 2023

alexlarsson left a comment

Choose a reason for hiding this comment

orowith2os Aug 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersion Aug 24, 2023 • edited

Choose a reason for hiding this comment

orowith2os Aug 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersion commented Aug 24, 2023

emersion commented Aug 24, 2023

emersion commented May 31, 2022 •

edited

orowith2os Aug 24, 2023 •

edited

emersion Aug 24, 2023 •

edited

orowith2os Aug 24, 2023 •

edited