Bug 100317

Summary: should be able to add new header fields that can be trusted
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: D-Bus Maintainers <dbus>
Status: NEW --- QA Contact: D-Bus Maintainers <dbus>
Severity: normal    
Priority: medium CC: desrt, dh.herrmann
Version: git master   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description Simon McVittie 2017-03-22 14:43:20 UTC
The D-Bus Specification says:

    A header must contain the required header fields for its message
    type, and zero or more of any optional header fields. Future 
    versions of this protocol specification may add new fields.
    **Implementations must ignore fields they do not understand.**
    Implementations must not invent their own header fields; only changes
    to this specification may introduce new header fields.

In practice the reference dbus-daemon happily passes through messages with arbitrary message header fields that it does not understand. The only restrictions are:

* the message header fields that *are* understood must have their documented types
* the message header fields that *are* understood can only appear once each
* the sender field is overwritten by the dbus-daemon

This unfortunately makes them non-useful as a way to add fields that are validated/guaranteed by the dbus-daemon, because a recipient can't tell the difference between "a new dbus-daemon set this field, it is definitely true" and "the sender set this field, an old dbus-daemon merely didn't understand it well enough to remove it".

For example, "uid of sender" and "LSM label of sender" as message headers would be good feature requests - if we could trust those headers, they would remove the need to call GetConnectionCredentials() in many cases.

Discussing this with Allison Lortie at the GTK Hackfest, we thought dbus-daemon should reinterpret "implementations must ignore fields they do not understand" to mean it will now filter out any header field that it does not understand; then we can add a negotiation step in which a peer asks the dbus-daemon to confirm that it actually does filter out the others. If the dbus-daemon doesn't, the high-level APIs used by services (libdbus, GDBus) would behave as though new/future header fields weren't there.

Unfortunately, GDBus already has g_dbus_message_get_header(), which can look up arbitrary fields by numeric ID.

It seems like this is potentially too many "sharp edges" - at best it's an API in which it's easy to be insecure by mistake - so we might have to close this WONTFIX :-(
Comment 1 Philip Withnall 2017-03-24 15:29:48 UTC
(In reply to Simon McVittie from comment #0)
> Unfortunately, GDBus already has g_dbus_message_get_header(), which can look
> up arbitrary fields by numeric ID.

Does that matter? Surely what GDBus would do is to rewrite the GDBusMessage on receiving it, to omit the new/future header fields (unless it had negotiated with the dbus-daemon to ensure they’re safe).
Comment 2 Simon McVittie 2017-03-24 16:30:47 UTC
(In reply to Philip Withnall from comment #1)
> Does that matter? Surely what GDBus would do

Current GDBus doesn't do whatever future thing you would like it to do.
Comment 3 Philip Withnall 2017-03-24 18:01:14 UTC
(In reply to Simon McVittie from comment #2)
> (In reply to Philip Withnall from comment #1)
> > Does that matter? Surely what GDBus would do
> 
> Current GDBus doesn't do whatever future thing you would like it to do.

Could we perhaps rely on this only in the situation where we’re using containers, and hence there’s a gating point for relying on a new-enough GDBus? i.e. “We can trust the header fields on this system because we are in/interacting-with a container, and the container technology mandates a new-enough GDBus and dbus-daemon to make this secure”.
Comment 4 Simon McVittie 2017-03-24 20:23:33 UTC
(In reply to Philip Withnall from comment #3)
> Could we perhaps rely on this only in the situation where we’re using
> containers, and hence there’s a gating point for relying on a new-enough
> GDBus?

We can do that for container instance metadata, sure, and that's why this doesn't block Bug #100344. But we can't use that argument for the other things I'd like to put in header fields: the uid and the LSM label.
Comment 5 Philip Withnall 2017-03-29 12:42:43 UTC
(In reply to Simon McVittie from comment #4)
> (In reply to Philip Withnall from comment #3)
> > Could we perhaps rely on this only in the situation where we’re using
> > containers, and hence there’s a gating point for relying on a new-enough
> > GDBus?
> 
> We can do that for container instance metadata, sure, and that's why this
> doesn't block Bug #100344. But we can't use that argument for the other
> things I'd like to put in header fields: the uid and the LSM label.

Surely we could for the case where one of the peers involved in a message is in a container (in the sense of using the API from bug #100344), since then both peers know dbus-daemon is new enough to filter out the UID/LSM label otherwise?

This basically shifts the trust decision from ensuring that the UID/LSM label are unforgeable, to ensuring that it’s not possible to make a D-Bus peer look like it’s using the container API (bug #100344) when it’s not.
Comment 6 David Herrmann 2017-05-11 09:15:25 UTC
How about bumping the version number? We use protocol version '2' and then force dbus-daemon to filter header fields.

gdbus can now easily see whether header fields are trusted or not, by just looking at the version number. Additionally, whoever *sends* a message, simply uses version '2' if the calling application did *NOT* add custom fields. If it added custom fields, it uses version '1'.

This still requires a negotiation at AUTH time (or Hello) to figure out which versions a client supports. But it would provide perfect backwards compatibility. The only issue is that nobody can rely on augmented header fields, if their clients send custom header fields.
Comment 7 Simon McVittie 2017-10-20 18:22:11 UTC
(In reply to David Herrmann from comment #6)
> This still requires a negotiation at AUTH time (or Hello) to figure out
> which versions a client supports.

If you have that, then it's easier to use that negotiation to figure out whether this dbus-daemon filters header fields. dbus-daemon now implements Properties, so we could use that (and in fact this was one of the use-cases I had in mind).

> How about bumping the version number? We use protocol version '2' and then
> force dbus-daemon to filter header fields.

I don't think that really helps (given the above), and using protocol version 2 requires action by all library implementors. Bumping the protocol version is the nuclear option: we'd use it if we changed the binary encoding of message payloads, but for anything less than that, it's overkill.

> gdbus can now easily see whether header fields are trusted or not, by just
> looking at the version number.

"Current GDBus doesn't do whatever future thing you would like it to do" applies just as much here as it did for Comment #1.

I'm (still) concerned that it would be far too easy for a service implementer to rely on "header field #42 is the uid" or something, use g_dbus_message_get_header(), make security decisions based on the assumption that the contents of header field #42 are indeed the uid, miss the fact that they're on a dbus-daemon (e.g. v1.10.x) that doesn't enforce the truthfulness of header field #42 because it doesn't know about that header field and hasn't implemented this feature request, and have a security bug.

But perhaps we can solve that with documentation, and the services whose authors read documentation[1] will be secure.

[1] i.e. approximately none

> Additionally, whoever *sends* a message,
> simply uses version '2' if the calling application did *NOT* add custom
> fields. If it added custom fields, it uses version '1'.

Applications already can't rely on being able to add custom fields, because fields are defined by the D-Bus Specification and unavailable for assignment or extension by applications (they are a dense integer-indexed space, not a sparse string-indexed space).

I suppose in principle a future D-Bus Specification could do something like "if the high bit is set, dbus-daemons that don't understand it should pass it through anyway; if not, dbus-daemons that don't understand it should remove it". Then we'd put security-sensitive header fields (uid, LSM label) in the low half of the space, and informational fields (monotonic time of sending) in the high half. But I suspect we're not going to need that in practice?
Comment 8 Simon McVittie 2017-10-24 15:21:26 UTC
(In reply to Simon McVittie from comment #0)
> Unfortunately, GDBus already has g_dbus_message_get_header(), which can look
> up arbitrary fields by numeric ID.
> 
> It seems like this is potentially too many "sharp edges" - at best it's an
> API in which it's easy to be insecure by mistake

The best ways I can think of to mitigate this:

* Have a way for services to check that the dbus-daemon implements this
  filtering ("HeaderFilter" in its o.fd.DBus.Features property?
  NEGOTIATE_HEADER_FILTERING at auth time?)

* As a design principle for the future, don't send new header fields
  until the recipient has done some sort of opt-in (so that recipients
  that don't opt-in don't work in practice, and therefore have an incentive
  to do an opt-in that will fail cleanly on all dbus-daemons that didn't
  implement the filtering - as well as not bloating messages to recipients
  that don't care)

* In libdbus and the D-Bus Specification, when adding new header fields,
  warn that in older implementations they can be sender-supplied, so
  they cannot be trusted until/unless the feature check has been done

* Submit a patch implementing that negotiation in GDBus, with an accessor
  on the GDBusConnection for whether the dbus-daemon does the filtering
  (If it's a method call, this would have to be async, because we can't
  send method calls before Hello() and can start receiving method calls
  immediately after; if it's a negotiation during the auth handshake, it
  can be sync)

* Submit a patch to GDBus changing the g_dbus_message_get_header()
  documentation to warn that older dbus-daemons didn't filter (with a
  reference to the accessor)
  - potentially also make g_dbus_message_get_header() skip header fields
    beyond the current set if the dbus-daemon doesn't filter, although
    making newer GDBus do this might give users of older GDBus a false
    sense of security

* Maybe avoid adding header fields for sender uid or LSM label (if we
  want those, which I think we eventually should) until 1.15.x, so that
  header filtering is significantly older than the fields that really,
  really need it

Do people think those would be enough?

sd-bus is like libdbus in this respect, rather than like GDBus - it doesn't have an accessor for header fields by numeric ID. So it doesn't need special attention like GDBus does.

(In reply to Simon McVittie from comment #7)
> dbus-daemon now implements
> Properties, so we could use that (and in fact this was one of the use-cases
> I had in mind).

It might be nicer to do this at auth time so that as soon as we start receiving method calls, we can know (synchronously) whether we can trust their header fields.

(But that does require extending the auth handshake, which is understood less well and by fewer people than ordinary messaging.)

> I suppose in principle a future D-Bus Specification could do something like
> "if the high bit is set, dbus-daemons that don't understand it should pass
> it through anyway; if not, dbus-daemons that don't understand it should
> remove it"

The only use-case I can think of is that there was a request for a message header in which the sender puts the monotonic timestamp from their perspective, so that recipients can see how long the message has taken to get through the dbus-daemon; but I can't actually remember why people wanted that anyway.
Comment 9 Allison Lortie (desrt) 2017-10-24 16:04:30 UTC
I would feel completely comfortable adding a patch to current GDBus that filters out "unknown" header fields.  Strictly speaking, GDBusMessageHeaderField is a closed enum, and if people start passing random ints to an API that takes this enum, then they weren't using it right to begin with...
Comment 10 Philip Withnall 2017-10-25 11:55:35 UTC
(In reply to Simon McVittie from comment #8)
> (In reply to Simon McVittie from comment #0)
> > Unfortunately, GDBus already has g_dbus_message_get_header(), which can look
> > up arbitrary fields by numeric ID.
> > 
> > It seems like this is potentially too many "sharp edges" - at best it's an
> > API in which it's easy to be insecure by mistake
> 
> The best ways I can think of to mitigate this:
> 
> * Have a way for services to check that the dbus-daemon implements this
>   filtering ("HeaderFilter" in its o.fd.DBus.Features property?
>   NEGOTIATE_HEADER_FILTERING at auth time?)
> 
> * As a design principle for the future, don't send new header fields
>   until the recipient has done some sort of opt-in (so that recipients
>   that don't opt-in don't work in practice, and therefore have an incentive
>   to do an opt-in that will fail cleanly on all dbus-daemons that didn't
>   implement the filtering - as well as not bloating messages to recipients
>   that don't care)

Aside: Is there somewhere that all these design principles are written down?


> * Submit a patch to GDBus changing the g_dbus_message_get_header()
>   documentation to warn that older dbus-daemons didn't filter (with a
>   reference to the accessor)
>   - potentially also make g_dbus_message_get_header() skip header fields
>     beyond the current set if the dbus-daemon doesn't filter, although
>     making newer GDBus do this might give users of older GDBus a false
>     sense of security

The alternative here is to deprecate the existing g_dbus_message_get_header() API and replace it with something which looks very similar, but with a different name. Then hopefully people using new GLib will use the new API, and will bump their version requirements on GLib, so will never end up being run against old GLib (which didn’t do the header filtering checks).

I guess this could be considered a configure-time opt-in rather than a runtime one.

> * Maybe avoid adding header fields for sender uid or LSM label (if we
>   want those, which I think we eventually should) until 1.15.x, so that
>   header filtering is significantly older than the fields that really,
>   really need it

Seems reasonable.

> Do people think those would be enough?

Yes. See also: what Allison suggests in comment #9.

> It might be nicer to do this at auth time so that as soon as we start
> receiving method calls, we can know (synchronously) whether we can trust
> their header fields.
> 
> (But that does require extending the auth handshake, which is understood
> less well and by fewer people than ordinary messaging.)

Good opportunity to make the auth handshake better understood by more people!

> > I suppose in principle a future D-Bus Specification could do something like
> > "if the high bit is set, dbus-daemons that don't understand it should pass
> > it through anyway; if not, dbus-daemons that don't understand it should
> > remove it"
> 
> The only use-case I can think of is that there was a request for a message
> header in which the sender puts the monotonic timestamp from their
> perspective, so that recipients can see how long the message has taken to
> get through the dbus-daemon; but I can't actually remember why people wanted
> that anyway.

I can’t see any advantages of doing that over putting the timestamp in the message body.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.