Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't require TALK permission for broadcast rules #61

Conversation

GeorgesStavracas
Copy link
Member

@GeorgesStavracas GeorgesStavracas commented May 25, 2024

The context is to allow for AT-SPI broadcast signals (specifically EventListenerRegistered and EventListenerDeregistered from the org.a11y.atspi.Registry interface on the registry object) to be allowed through, without giving apps a full talk permission to it.

CC @ebassi @smcv

@smcv I appreciate this goes directly against https://gitlab.freedesktop.org/dbus/dbus/-/issues/185#note_48128, but I don't understand that restriction and would appreciate some ellucidation as to why that's been proposed. I'm not particularly sold on this approach so any advice will be highly welcomed!

Edit: see #61 (comment) for a more up-to-date understanding of the D-Bus issue.

The context is to allow for AT-SPI broadcast signals (specifically
`EventListenerRegistered` and `EventListenerDeregistered` from the
`org.a11y.atspi.Registry` interface on the registry object) to be
allowed through, without giving apps a full talk permission to it.
@ebassi
Copy link

ebassi commented May 27, 2024

I do wonder if the simplest solution would be for flatpak to always set up the a11y bus proxy with:

--broadcast=org.a11y.atspi.Registry=@/org/a11y/atspi/registry

After all, there's already a:

"--call=org.a11y.atspi.Registry=org.a11y.atspi.Registry.GetRegisteredEvents@/org/a11y/atspi/registry"

in the proxy set up function. Would that work too?

@GeorgesStavracas
Copy link
Member Author

Basically --broadcast doesn't have any effect without --talk too. This PR would make your suggestion possible 🙂

@GeorgesStavracas
Copy link
Member Author

I'm going to try and be a bit more helpful and expand on my previous comment. According to this comment in the D-Bus bugtracker:

Receiving broadcast signals from outside the container instance checks for TALK access to the sender.

This is implemented by xdg-dbus-proxy in the following lines:

xdg-dbus-proxy/flatpak-proxy.c

Lines 2746 to 2752 in 1bcfaea

if (policy == FLATPAK_POLICY_OWN ||
(policy == FLATPAK_POLICY_TALK &&
any_filter_matches (filters, FILTER_TYPE_BROADCAST,
header->path,
header->interface,
header->member)))
filtered = FALSE;

As a consequence, in order to pass --broadcast=org.a11y.atspi.Registry.*=@/org/a11y/atspi/registry, we also need --talk=org.a11y.atspi.Registry. But this talk permission is pretty massive, and will expose unwanted objects.

What I'm proposing here in this PR is, specifically, to allow --broadcast to work without --talk.

@ebassi
Copy link

ebassi commented May 27, 2024

Right that makes sense.

But this talk permission is pretty massive, and will expose unwanted objects.

What kind of objects? Can we limit that to the /org/a11y/atspi/registry object path?

@GeorgesStavracas
Copy link
Member Author

What kind of objects?

I was thinking of /org/a11y/atspi/accessible/root, the Accessible and Component interfaces contain quite a bit of information, I think. It may or may not be a problem, I don't know.

Can we limit that to the /org/a11y/atspi/registry object path?

Not with today's xdg-dbus-proxy. Right now, --talk only accepts a bus name, it doesn't have object path granularity

@GeorgesStavracas
Copy link
Member Author

Thanks to Philip's review, and after a better read at https://gitlab.freedesktop.org/dbus/dbus/-/issues/185, I actually think this PR implements the exact proposed behaviour described in that issue.

Comment dbus/dbus#185#note_48129 reads:

The equivalent of --talk=NAME is (flags=SEE|CALL_METHODS|SEND_UNICAST_SIGNALS|RECEIVE_BROADCASTS|SEND_UNIX_FDS|RECEIVE_UNIX_FDS|OBJECT_PATH_IS_SUBTREE|ACTIVATE, bus_name=NAME, object_path=/, interface="").

and comment dbus/dbus#185#note_48130 reads:

Comment 5 proposes RECEIVE_BROADCASTS.

So to the best of my understanding, in practice these two comments propose that:

  • TALK permission includes RECEIVE_BROADCASTS permission
  • RECEIVE_BROADCASTS can be allowlisted without TALK permission

Which is exactly what this PR implements.

@GeorgesStavracas GeorgesStavracas marked this pull request as ready for review May 28, 2024 09:38
GeorgesStavracas added a commit to GeorgesStavracas/flatpak that referenced this pull request May 28, 2024
These signals can be used by apps to monitor whether they need to emit
signals on the a11y bus or not. This can very significantly reduce
chattery on the a11y bus, and at least WebKit relies on these signals
to be broadcasted in.

The PR flatpak/xdg-dbus-proxy#61 is required
for this changeset to work as expected, but it can land independently
as `--broadcast` is supported by xdg-dbus-proxy.
@pwithnall
Copy link

OK LGTM! Sorry for the delay in taking another look at this

@GeorgesStavracas GeorgesStavracas merged commit 0297845 into flatpak:main Jul 9, 2024
5 checks passed
@GeorgesStavracas GeorgesStavracas deleted the gbsneto/broadcast-doesnt-require-talk branch July 9, 2024 12:14
GeorgesStavracas added a commit to GeorgesStavracas/flatpak that referenced this pull request Jul 9, 2024
These signals can be used by apps to monitor whether they need to emit
signals on the a11y bus or not. This can very significantly reduce
chattery on the a11y bus, and at least WebKit relies on these signals
to be broadcasted in.

The PR flatpak/xdg-dbus-proxy#61 is required
for this changeset to work as expected, but it can land independently
as `--broadcast` is supported by xdg-dbus-proxy.
GeorgesStavracas added a commit to flatpak/flatpak that referenced this pull request Jul 9, 2024
These signals can be used by apps to monitor whether they need to emit
signals on the a11y bus or not. This can very significantly reduce
chattery on the a11y bus, and at least WebKit relies on these signals
to be broadcasted in.

The PR flatpak/xdg-dbus-proxy#61 is required
for this changeset to work as expected, but it can land independently
as `--broadcast` is supported by xdg-dbus-proxy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants