Bug 68597 - spec requires "no-interface" method calls to work, but they can't be secure on the system bus
Summary: spec requires "no-interface" method calls to work, but they can't be secure o...
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact:
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-08-27 10:19 UTC by Simon McVittie
Modified: 2014-01-06 15:43 UTC (History)
6 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Do not require messages without INTERFACE to be dispatched (2.51 KB, patch)
2013-09-16 13:21 UTC, Simon McVittie
Details | Splinter Review
spec: explicitly mention filtering messages with no INTERFACE (1.59 KB, patch)
2013-09-16 13:22 UTC, Simon McVittie
Details | Splinter Review

Description Simon McVittie 2013-08-27 10:19:59 UTC
Sending method calls "to no interface" is allowed, but a bad idea. Its semantics are "I don't know or care which interface I'm calling a method on, please pick one arbitrarily" which doesn't make a whole lot of sense, since the semantics of a method are only defined by its interface.

The D-Bus protocol spec says:

    A method call message is required to have a MEMBER header field
    indicating the name of the method. Optionally, the message has an
    INTERFACE field giving the interface the method is a part of. In
    the absence of an INTERFACE field, if two interfaces on the same
    object have a method with the same name, it is undefined which of
    the two methods will be invoked. Implementations may also choose to
    return an error in this ambiguous case.

I think we should consider *encouraging* implementations to return an error in this ambiguous case.

The spec also says

    However, if a method name is unique implementations must not
    require an interface field

but that doesn't seem very well thought-out, and I'm tempted to
delete it from the spec. For instance, messages with no interface on the
system bus will usually be shot down by security restrictions, since the
system bus has a default-deny policy and the "firewall" rules that allow
messages through will typically only match particular interfaces.
Allowing messages with an unspecified interface to match a rule with a
particular interface would be a security flaw, because the dbus-daemon
can't know how a service will interpret such a message.

See also <https://bugzilla.gnome.org/show_bug.cgi?id=706675> and a recent thread on gtk-devel-list.

Thoughts from the maintainer cabal? Cc'ing desrt and davidz, as GDBus people.
Comment 1 Thiago Macieira 2013-08-27 15:17:04 UTC
This is probably a carry-over from DCOP, where interfaces were possible but never used.

I'm fine with reducing the no-interface case to optional behaviour instead of required. Please don't make implementations have to return an error in case of ambiguity -- it might require a linear search to find instead of stopping on the first match.
Comment 2 Simon McVittie 2013-08-27 15:31:30 UTC
(In reply to comment #1)
> Please don't make implementations have to return an error in
> case of ambiguity -- it might require a linear search to find instead of
> stopping on the first match.

That's a fair point. OK, so on receiving a method call with no interface, implementations MAY either treat it as a call to the same method on an implementation-chosen interface, or return an error.
Comment 3 Thiago Macieira 2013-08-27 16:07:34 UTC
By the way, some implementations have taken upon themselves (or as a side-effect) to apply the same rules for properties: calling Get or Set with an empty interface name causes the receiver to search for the property name in all available interfaces.

I remember that QtDBus did not support this when first released, but the code is there today (on the receiver side) because someone reported a bug.
Comment 4 David Zeuthen (not reading bugmail) 2013-08-27 17:04:17 UTC
(In reply to comment #0)
> I think we should consider *encouraging* implementations to return an error
> in this ambiguous case.

I think that's a good idea.

> The spec also says
> 
>     However, if a method name is unique implementations must not
>     require an interface field
> 
> but that doesn't seem very well thought-out, and I'm tempted to
> delete it from the spec.

I concur.
Comment 5 Chengwei Yang 2013-09-11 09:14:47 UTC
Seems this is another difference between system and session bus, plus to the one "servicehelper <service>" requires the .service file match its owned bus name while the dbus spec doesn't say that. See #bug 66608.
Comment 6 Simon McVittie 2013-09-16 13:11:42 UTC
(In reply to comment #3)
> By the way, some implementations have taken upon themselves (or as a
> side-effect) to apply the same rules for properties

The spec does note this:

        An empty string may be provided for the interface name; in this case, 
        if there are multiple properties on an object with the same name, 
        the results are undefined (picking one by according to an arbitrary 
        deterministic rule, or returning an error, are the reasonable 
        possibilities).
Comment 7 Simon McVittie 2013-09-16 13:21:40 UTC
Created attachment 85910 [details] [review]
Do not require messages without INTERFACE to be  dispatched

reviously, if we have interfaces:

    interface com.example.foo:
        method Ambiguous()
    interface com.example.bar:
        method Ambiguous()
        method Unambiguous()

implementations were required to deliver a message with no INTERFACE
and METHOD=Unambiguous to "bar". A message with no INTERFACE and
METHOD=Ambiguous could either be delivered to "foo", delivered to "bar"
or treated as an error.

Relax this to allow an error for the unambiguous case, too, and
strongly recommend specifying the interface (which is best-practice).
Comment 8 Simon McVittie 2013-09-16 13:22:01 UTC
Created attachment 85911 [details] [review]
spec: explicitly mention filtering messages with no  INTERFACE

This is an important security measure. Without it, the system bus
would not deliver its intended security properties. The actual
implementation has always behaved like this, I think.
Comment 9 Allison Lortie (desrt) 2013-10-25 13:14:55 UTC
+          In the absence of an <literal>INTERFACE</literal> field, if two
+          or more interfaces on the same object have a method with the same
+          name, it is undefined which of those methods will be invoked.
+          Implementations may choose to either return an error, or deliver the
+          message as though it had an arbitrary one of those interfaces.

This sounds like if the method exists only on one interface then we still must deliver it and may only return an error as one of our possible options in the case that there is ambiguity.

I'd much prefer if we changed the language to something like "Client implementations are strongly encouraged to reject all incoming messages without interface set and to never send such messages.  The message bug will pass these messages on only for reasons of backward compatibility."
Comment 10 Allison Lortie (desrt) 2013-10-25 13:15:21 UTC
*bus ;)
Comment 11 Simon McVittie 2013-10-25 14:37:45 UTC
(In reply to comment #9)
> This sounds like if the method exists only on one interface then we still
> must deliver it and may only return an error as one of our possible options
> in the case that there is ambiguity.

That was my intention, but if you want to change the spec further (so it's undefined in all cases), I wouldn't be against that.

> I'd much prefer if we changed the language to something like "Client
> implementations are strongly encouraged to reject all incoming messages
> without interface set and to never send such messages.  The message bug will
> pass these messages on only for reasons of backward compatibility."

It seems a bit strange to be going straight from "MUST call the method in the unambiguous case, and MAY call a method in the ambiguous case" to "SHOULD NOT call the method, even in the unambiguous case". I'm generally against specification changes that make every previously-correct implementation violate a SHOULD.

(The service-side of GDBus is not currently a correct implementation according to the spec. I'd be inclined to say that GDBus is fine, and it's the spec that's wrong, hence this bug.)

How about this, which makes it explicitly undefined, like properties?

+          <literal>INTERFACE</literal> field giving the interface the method is a part of.
+          Including the <literal>INTERFACE</literal> in all method call
+          messages is strongly recommended.
+        </para>
+        <para>
+          In the absence of an <literal>INTERFACE</literal> field, the
+          result is undefined. Implementations may choose to either return
+          an error, or deliver the message to a method of that name
+          on any interface (if there is one).

I'd also be fine with appending "Returning an error is recommended in this situation" if there's consensus, but I'm not going to try to force that if people consider it to be equally valid.

In practical terms, I don't think existing libraries that handle the unambiguous case, or even the ambiguous case, as a non-error (QtDBus, dbus-python, etc.) should change it now; but for new implementations, I agree with desrt.
Comment 12 Chengwei Yang 2013-11-18 06:03:17 UTC
Comment on attachment 85910 [details] [review]
Do not require messages without INTERFACE to be  dispatched

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

looks good!
Comment 13 Chengwei Yang 2013-11-18 06:04:23 UTC
Comment on attachment 85911 [details] [review]
spec: explicitly mention filtering messages with no  INTERFACE

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

Looks good. Yes, this is the current way how dbus-daemon works.
Comment 14 Simon McVittie 2013-11-27 15:26:27 UTC
I'm going to merge these, since they make the spec reflect reality, and are halfway between what the spec currently says and what desrt wants it to say.

desrt, if you want further divergence from the current spec, please reply to Comment #11 or propose different wording.

If any maintainers object to these patches going in, please complain before 1.7.10. :-)
Comment 15 Simon McVittie 2014-01-06 15:43:59 UTC
Fixed in git for 1.7.10.

desrt (or other interested maintainers), if you think that the result should be undefined even when unambiguous, please propose a patch, or state that you agree with my text below "How about this" in Comment #11.

If you think that, additionally, returning an error for any no-INTERFACE call should be recommended, please propose further patches that you'd be happy with.


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.