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

Whitelist properties and peer calls #60

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JakobDev
Copy link

It is expected that this is available

@swick
Copy link
Contributor

swick commented May 27, 2024

This seems fine in general but maybe explicitly list all the methods/props/signals that should become available in case dbus later gets some feature that we really shouldn't expose to sandboxes.

@JakobDev
Copy link
Author

How should GetAll be handled?

@smcv
Copy link
Contributor

smcv commented Aug 22, 2024

Whitelist properties and peer calls

This is a misleading commit message: what it immediately made me think was "what? we shouldn't allow getting the properties of arbitrary peers, they might be secret!"

What I think you mean is more like: "Allow Properties, Peer and Introspection calls to the message bus itself" and that's a more reasonable thing to allow.

But, as @swick suggested, it's entirely possible that the message bus could gain properties that we will sometimes want to hide from bus clients (for example, imagine if the list of all owned names was a property), so I think this needs to be more nuanced than "allow everything".

Allowing Peer.Ping against the message bus is certainly fine. If that's something you want for your use-case, special-casing it to be allowed would make sense.

Allowing Peer.GetMachineId I'm not so sure - Flatpak doesn't make any attempt to keep the machine ID secret from apps, but in principle another user of xdg-dbus-proxy might want to do that?

Allowing Introspectable against the message bus also seems OK, the scope of that interface is quite narrowly-defined; but generally it should only be used by debugging/development tools anyway, so I'm unsure of its value.

I am uneasy about allowing Properties globally. For the Properties of interface org.freedesktop.DBus, Features seems OK, and Interfaces seems OK... but is it going to be a problem if Features or Interfaces advertises a feature/interface that the message bus allows, but xdg-dbus-proxy does not?

Perhaps it would be better for xdg-dbus-proxy to special-case Properties and rewrite the reply:

  • GetAll("org.freedesktop.DBus") → call GetAll("org.freedesktop.DBus") on the real message bus, but then parse the response, take the known-safe subset of it that xdg-dbus-proxy understands, and send that back as if it had been the reply
  • Get("org.freedesktop.DBus") → only allow specific properties, and filter the reply as above if necessary
  • anything else → deny

I don't think we should allow other properties or GetAll, at least not without first adding some wording to the D-Bus Specification about it being unsafe to add new Properties on org.freedesktop.DBus that would be things you could reasonably want to keep hidden from sandboxed apps. I also don't think getting the Properties of an unknown interface (for example org.freedesktop.DBus.SomeExtensions or io.github.bus1.dbus_broker.SomeOtherExtension) should generally be allowed, because we can't know what the other extension does or how it works.

@smcv
Copy link
Contributor

smcv commented Aug 22, 2024

It is expected that this is available

Sorry, no, that's not a use-case. What's an example (or examples) of a real application needing to get this information about the message bus?

I think this is an example of something that I often find myself saying: instead of jumping directly to proposing a solution in a PR and never actually stating the problem, it's often better to start with a solution-neutral problem statement reported as an issue, so that potential solutions can be assessed against whether they solve it (without causing other bad effects).

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.

4 participants