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.

The same logic will be used for Wayland security context.

More complicated setup logic will be added next commit.

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

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

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?

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 added 2 commits July 3, 2023 16:34

common: introduce flatpak_bwrap_add_sync_fd

1c6f350

The same logic will be used for Wayland security context.

common: split Wayland socket setup to separate file

532f36d

More complicated setup logic will be added next commit.

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

Add support for Wayland security context #4920

Add support for Wayland security context #4920

emersion commented May 31, 2022 •

edited

emersion commented Jun 10, 2022

swick commented Jun 10, 2022

jadahl Jun 15, 2023

swick Jun 15, 2023

alexlarsson Jul 3, 2023

alexlarsson Jul 3, 2023

emersion Jul 3, 2023

jadahl Jul 3, 2023

jadahl Jul 3, 2023

swick Jul 3, 2023

swick Jul 3, 2023

alexlarsson Jul 3, 2023

emersion commented Jun 15, 2023

davidedmundson commented Jun 22, 2023

jadahl commented Jun 22, 2023

emersion commented Jun 28, 2023

alexlarsson left a 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

Add support for Wayland security context #4920

Are you sure you want to change the base?

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 May 31, 2022 •

edited