flatpak / xdg-dbus-proxy

GNU Lesser General Public License v2.1
53 stars 21 forks source link

Don't require TALK permission for broadcast rules #61

Closed GeorgesStavracas closed 1 week ago

GeorgesStavracas commented 1 month ago

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 https://github.com/flatpak/xdg-dbus-proxy/pull/61#issuecomment-2134772967 for a more up-to-date understanding of the D-Bus issue.

ebassi commented 1 month ago

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 commented 1 month ago

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

GeorgesStavracas commented 1 month ago

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:

https://github.com/flatpak/xdg-dbus-proxy/blob/1bcfaea06f6504aee39e0fff19d8156681dafe9c/flatpak-proxy.c#L2746-L2752

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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:

Which is exactly what this PR implements.

pwithnall commented 1 week ago

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