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.
Created attachment 131610 [details] [review] [1/4] test/monitor: Assert that BecomeMonitor() on wrong object path fails
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.
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.
Created attachment 131613 [details] [review] [4/4] spec: Document the canonical object path for the bus driver
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 on attachment 131610 [details] [review] [1/4] test/monitor: Assert that BecomeMonitor() on wrong object path fails Review of attachment 131610 [details] [review]: ----------------------------------------------------------------- ++
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 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 on attachment 131613 [details] [review] [4/4] spec: Document the canonical object path for the bus driver Review of attachment 131613 [details] [review]: ----------------------------------------------------------------- ++
(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.
(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.
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.