Bug 101256 - Document bus driver object path, and only use it for older methods
Summary: Document bus driver object path, and only use it for older methods
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL: https://github.com/smcv/dbus/commits/...
Whiteboard: review-
Keywords: patch
Depends on:
Blocks: 101257
  Show dependency treegraph
 
Reported: 2017-05-31 17:39 UTC by Simon McVittie
Modified: 2017-06-02 12:49 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
[1/4] test/monitor: Assert that BecomeMonitor() on wrong object path fails (2.06 KB, patch)
2017-05-31 17:39 UTC, Simon McVittie
Details | Splinter Review
[2/4] bus/driver: Make non-core interfaces unavailable on most object paths (7.26 KB, patch)
2017-05-31 17:40 UTC, Simon McVittie
Details | Splinter Review
[3/4] bus/driver: Only allow specific methods to be called at wrong paths (10.01 KB, patch)
2017-05-31 17:40 UTC, Simon McVittie
Details | Splinter Review
[4/4] spec: Document the canonical object path for the bus driver (1.87 KB, patch)
2017-05-31 17:41 UTC, Simon McVittie
Details | Splinter Review
[3/4] bus/driver: Only allow specific methods to be called at wrong paths (10.59 KB, patch)
2017-05-31 19:37 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2017-05-31 17:39:29 UTC
The reference implementation of the message bus has the weird historical quirk that it accepts most method calls on arbitrary object paths. This is silly: the D-Bus object model is that interfaces only exist within specific objects, identified by their object paths.

The message bus emits signals from /org/freedesktop/DBus, so we can treat that path as the canonical one.

For some method calls, since 1.8.14 we already reject attempts to call the method on the wrong object path. This is security hardening against D-Bus policy XML that (inappropriately) allows arbitrary method calls limited only by object path.

We have to keep this strange arrangement for the o.fd.DBus interface, but we can mitigate it by having an opt-in flag that is set on the methods that are older than the current D-Bus Specification and are not security-sensitive.

For other interfaces, we can just omit the interface from other object paths' introspection and method call dispatching.
Comment 1 Simon McVittie 2017-05-31 17:39:55 UTC
Created attachment 131610 [details] [review]
[1/4] test/monitor: Assert that BecomeMonitor() on wrong object  path fails
Comment 2 Simon McVittie 2017-05-31 17:40:29 UTC
Created attachment 131611 [details] [review]
[2/4] bus/driver: Make non-core interfaces unavailable on most  object paths

The o.fd.DBus interface needs to remain available on arbitrary object
paths for backwards compatibility, and the Introspectable interface
is genuinely useful, but everything else can be skipped.
 
This is arguably an incompatible change for the undocumented Verbose
interface, and for the GetAllMatchRules method on the undocumented
Stats interface: previously those were available at all object paths.

---

In case it isn't obvious: I think that arguable incompatible change is harmless.
Comment 3 Simon McVittie 2017-05-31 17:40:59 UTC
Created attachment 131612 [details] [review]
[3/4] bus/driver: Only allow specific methods to be called at  wrong paths

The default for the future should be that new functionality should
only be available at /org/freedesktop/DBus, which is the canonical
path of the "bus driver" object that represents the message bus.
 
The disallowed methods are still in Introspect() output, and 
raise AccessDenied instead of UnknownMethod, to preserve the invariant
that the o.fd.DBus interface has constant contents. 

test/dbus-daemon.c already asserts that UpdateActivationEnvironment()
still raises AccessDenied when invoked on a non-canonical path;
this has been in place since 1.8.14.
Comment 4 Simon McVittie 2017-05-31 17:41:17 UTC
Created attachment 131613 [details] [review]
[4/4] spec: Document the canonical object path for the bus   driver
Comment 5 Simon McVittie 2017-05-31 19:37:40 UTC
Created attachment 131616 [details] [review]
[3/4] bus/driver: Only allow specific methods to be called at  wrong paths

The default for the future should be that new functionality should
only be available at /org/freedesktop/DBus, which is the canonical
path of the "bus driver" object that represents the message bus.

The disallowed methods are still in Introspect() output, and
raise AccessDenied instead of UnknownMethod, to preserve the invariant
that the o.fd.DBus interface has constant contents.

test/dbus-daemon.c already asserts that UpdateActivationEnvironment()
still raises AccessDenied when invoked on a non-canonical path;
this has been in place since 1.8.14.

---

Remove the declaration of bus_driver_check_message_is_for_us()
Comment 6 Philip Withnall 2017-06-01 10:59:08 UTC
Comment on attachment 131610 [details] [review]
[1/4] test/monitor: Assert that BecomeMonitor() on wrong object  path fails

Review of attachment 131610 [details] [review]:
-----------------------------------------------------------------

++
Comment 7 Philip Withnall 2017-06-01 11:03:12 UTC
Comment on attachment 131611 [details] [review]
[2/4] bus/driver: Make non-core interfaces unavailable on most  object paths

Review of attachment 131611 [details] [review]:
-----------------------------------------------------------------

Looks good apart from a few nitpicks.

::: bus/driver.c
@@ +2420,5 @@
> +  INTERFACE_FLAG_NONE = 0,
> +  /* Various older interfaces were available at every object path. We have to
> +   * preserve that behaviour for backwards compatibility, but we can at least
> +   * stop doing that for newly added interfaces. */
> +  INTERFACE_FLAG_ANY_PATH = (1 << 0)

Maybe add a reference to this bug in the comment. Probably also want to mention (or at least not imply to the contrary) that it’s actually useful for o.fd.D.Introspectable.

Nitpick: Add a trailing comma after the final enum value.

@@ +2557,4 @@
>    DBusString xml;
>    DBusMessage *reply;
>    const char *v_STRING;
> +  dbus_bool_t canonical_path;

Nitpick: Maybe call this `is_canonical_path` to make it more obvious it’s a boolean (rather than an object path)?

@@ +2655,4 @@
>    const InterfaceHandler *ih;
>    const MessageHandler *mh;
>    dbus_bool_t found_interface = FALSE;
> +  dbus_bool_t canonical_path;

Nitpick: similarly.
Comment 8 Philip Withnall 2017-06-01 11:05:33 UTC
Comment on attachment 131616 [details] [review]
[3/4] bus/driver: Only allow specific methods to be called at  wrong paths

Review of attachment 131616 [details] [review]:
-----------------------------------------------------------------

++ with nitpicks

::: bus/driver.c
@@ +2293,5 @@
> +{
> +  METHOD_FLAG_NONE = 0,
> +  /* Various older methods were available at every object path. We have to
> +   * preserve that behaviour for backwards compatibility, but we can at least
> +   * stop doing that for newly added methods.. */

Nitpick: Double full-stop at the end of the comment. Maybe cross-reference this bug report in the comment.

@@ +2294,5 @@
> +  METHOD_FLAG_NONE = 0,
> +  /* Various older methods were available at every object path. We have to
> +   * preserve that behaviour for backwards compatibility, but we can at least
> +   * stop doing that for newly added methods.. */
> +  METHOD_FLAG_ANY_PATH = (1 << 0)

Nitpick: Add a trailing comma.
Comment 9 Philip Withnall 2017-06-01 11:06:31 UTC
Comment on attachment 131613 [details] [review]
[4/4] spec: Document the canonical object path for the bus   driver

Review of attachment 131613 [details] [review]:
-----------------------------------------------------------------

++
Comment 10 Simon McVittie 2017-06-01 16:13:19 UTC
(In reply to Philip Withnall from comment #7)
> Maybe add a reference to this bug in the comment. Probably also want to
> mention (or at least not imply to the contrary) that it’s actually useful
> for o.fd.D.Introspectable.

Good point - Introspectable is not just there for compatibility.

> Nitpick: Add a trailing comma after the final enum value.

Sadly that isn't Standard C (although gcc, clang and most real compilers accept it).

Perhaps I could move the NONE sentinels to the end of the enum so that the special last entry is special in a different way anyway?

> Nitpick: Maybe call this `is_canonical_path` to make it more obvious it’s a
> boolean (rather than an object path)?

Good idea.
Comment 11 Philip Withnall 2017-06-01 16:47:18 UTC
(In reply to Simon McVittie from comment #10)
> Perhaps I could move the NONE sentinels to the end of the enum so that the
> special last entry is special in a different way anyway?

That would be great.
Comment 12 Simon McVittie 2017-06-02 12:49:01 UTC
I've pushed these with the changes you recommended. Fixed in git for 1.11.14.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.