(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). (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. (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”. (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. (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. 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. (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? (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. 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... (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. Created attachment 135758 [details] [review] [1/9] DBusHeader: Add a diagram of the header --- This helped me to understand what's going on while starting to think about the implementation for this feature. It'll probably also be useful for fixing Bug #38288. Created attachment 135759 [details] [review] [2] dbus_message_demarshal: Free the loader if we run out of memory --- Bug found while testing header handling, in preparation for actually implementing the desired feature here (which I haven't done yet). Created attachment 135760 [details] [review] [3] test_try_connect_to_bus: Don't leak the connection on OOM Created attachment 135761 [details] [review] [4] test_try_connect_to_bus: Cope with OOM while setting up connection Created attachment 135762 [details] [review] [5] test_connection_setup: Don't crash on unlikely OOM Created attachment 135763 [details] [review] [6] test-utils: Separate failable and non-failable functions test_object_try_whatever() now has libdbus-like OOM handling, while test_object_whatever() has GLib-like OOM handling. This is because an overwhelming majority of the callers of these functions either didn't check for OOM anyway, or checked for it but then aborted. In the uncommon case where we do care, we can use the _try_ version. Created attachment 135764 [details] [review] [7] tests: Add the ability to multiply up test timeouts Created attachment 135765 [details] [review] [8] _dbus_test_oom_handling: print TAP diagnostics These aren't *that* verbose, so it seems OK to print them all the time, not just in the needlessly spammy verbose mode. Created attachment 135766 [details] [review] [9] WiP: Add a test for header fields --- This is probably going to need a bit of modification to handle how I implement the removal of unknown header fields, which I haven't actually done (or decided how to do) yet. I'm not sure whether I'm going to strip unknown header fields in _dbus_header_load() (which would apply to anything that receives or demarshals a D-Bus message, but make it impossible to pass unknown header fields through even if someone wants to); or whether I'm going to do so in the dbus-daemon (the same place we set the sender), since it's the dbus-daemon that is the real trust boundary. If I do it in the dbus-daemon, we'll want a new method _dbus_message_clean_header() or something, which this unit test could call in addition to passing the message through the dbus-daemon in order to get the OOM coverage. Does anyone have an opinion either way? Comment on attachment 135758 [details] [review] [1/9] DBusHeader: Add a diagram of the header Review of attachment 135758 [details] [review]: ----------------------------------------------------------------- r- ::: dbus/dbus-marshal-header.h @@ +80,5 @@ > + * To understand _dbus_header_load(): > + * > + * [A] is FIRST_FIELD_OFFSET. > + * header_len is from 0 to [C]. > + * padding_start is [B], adjusted because the header is not necessarily ‘adjusted’ how? Adjusted to include the length of the stuff preceding the header, or adjusted to exclude it? Comment on attachment 135759 [details] [review] [2] dbus_message_demarshal: Free the loader if we run out of memory Review of attachment 135759 [details] [review]: ----------------------------------------------------------------- I think the commit message is wrong here: shouldn’t it be something like “set @error if we run out of memory”? The loader doesn’t need freeing in the (loader == NULL) case, but previously _DBUS_SET_OOM(error) was not being called. Comment on attachment 135760 [details] [review] [3] test_try_connect_to_bus: Don't leak the connection on OOM Review of attachment 135760 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 135761 [details] [review] [4] test_try_connect_to_bus: Cope with OOM while setting up connection Review of attachment 135761 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 135762 [details] [review] [5] test_connection_setup: Don't crash on unlikely OOM Review of attachment 135762 [details] [review]: ----------------------------------------------------------------- r+ (In reply to Philip Withnall from comment #20) > ::: dbus/dbus-marshal-header.h > @@ +80,5 @@ > > + * To understand _dbus_header_load(): > > + * > > + * [A] is FIRST_FIELD_OFFSET. > > + * header_len is from 0 to [C]. > > + * padding_start is [B], adjusted because the header is not necessarily > > ‘adjusted’ how? Adjusted to include the length of the stuff preceding the > header, or adjusted to exclude it? To include it. Would this be a clearer way to express it? start is the distance from beginning of buffer to beginning of message FIRST_FIELD_OFFSET is the distance from beginning of message to [A] header_len is the distance from beginning of message to [C] padding_start is the distance from beginning of buffer to [B] (so it includes start and header_len but not padding_len) padding_len is the padding from [B] to [C] (In reply to Philip Withnall from comment #21) > I think the commit message is wrong here: shouldn’t it be something like > “set @error if we run out of memory”? The loader doesn’t need freeing in the > (loader == NULL) case, but previously _DBUS_SET_OOM(error) was not being > called. Yes, you're right. A test failed because the cleanup step wasn't run, but I didn't describe the right part of the cleanup in the commit message. (In reply to Simon McVittie from comment #25) > start is the distance from beginning of buffer to beginning of message ... which it turns out is always 0, because we always pop messages from the beginning of the buffer, so a useful piece of refactoring would be to remove that parameter altogether so we no longer have to think about it. This is a milder form of the speculative generality that I was talking about on Bug #38288. Created attachment 135863 [details] [review] [0] _dbus_header_load: Operate on the entire string, not a substring This function worked with a (string,position,length) triple, but it turns out to only have one caller, which tells it to look at the entire string anyway. It'll be easier to document if all the offsets start from 0. Created attachment 135864 [details] [review] [1 v2] DBusHeader: Add a diagram of the header --- Stop talking about @start, it has gone away. Created attachment 135865 [details] [review] [2 v2] dbus_message_demarshal: Set error if we can't allocate --- Same content, different commit message. Comment on attachment 135863 [details] [review] [0] _dbus_header_load: Operate on the entire string, not a substring Review of attachment 135863 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 135864 [details] [review] [1 v2] DBusHeader: Add a diagram of the header Review of attachment 135864 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 135865 [details] [review] [2 v2] dbus_message_demarshal: Set error if we can't allocate Review of attachment 135865 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 135763 [details] [review] [6] test-utils: Separate failable and non-failable functions Review of attachment 135763 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 135764 [details] [review] [7] tests: Add the ability to multiply up test timeouts Review of attachment 135764 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 135765 [details] [review] [8] _dbus_test_oom_handling: print TAP diagnostics Review of attachment 135765 [details] [review]: ----------------------------------------------------------------- r+ (In reply to Simon McVittie from comment #19) > Created attachment 135766 [details] [review] [review] > [9] WiP: Add a test for header fields > > --- > > This is probably going to need a bit of modification to handle how I > implement the removal of unknown header fields, which I haven't actually > done (or decided how to do) yet. > > I'm not sure whether I'm going to strip unknown header fields in > _dbus_header_load() (which would apply to anything that receives or > demarshals a D-Bus message, but make it impossible to pass unknown header > fields through even if someone wants to); or whether I'm going to do so in > the dbus-daemon (the same place we set the sender), since it's the > dbus-daemon that is the real trust boundary. If I do it in the dbus-daemon, > we'll want a new method _dbus_message_clean_header() or something, which > this unit test could call in addition to passing the message through the > dbus-daemon in order to get the OOM coverage. > > Does anyone have an opinion either way? Doing it in the dbus-daemon seems to make sense from what you say, but I don’t really understand the situation here. Go with that and I’ll review it? (In reply to Philip Withnall from comment #10) > (In reply to Simon McVittie from comment #8) > > 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. I started writing spec wording for this, but it's actually a bit unpleasant. Putting this in the auth handshake seems like the wrong layer: NEGOTIATE_UNIX_FD is a fact about the transport, and if we ever land Bug #27857, NEGOTIATE_MAYBE (or NEGOTIATE_EXTRA_TYPES or whatever) is a fact about the message deserializer (which is arguably part of the transport, at least in libdbus). But what we do about header fields is really more like a fact about the message bus, or the DBusServer, and how it interacts with less-than-totally-trusted clients - it's only meaningful if we are receiving messages from another client (or replaying messages from a file, or receiving messages through a proxy, or whatever) and relaying them through a central point that is more trusted than the source of the messages. It is possible to define what header field filtering means for non-message-bus servers, but if we do it adds quite a lot more words that will never be applicable to 95% of D-Bus users (and the remaining 5%, implementing things like Telepathy Tubes, already have to reinvent the half of the message bus that they want). If the actual filtering is happening in the message loader and is unconditional, then we could implement the negotiation similarly unconditionally. But as I mentioned in Comment #19, this seems odd: it affects all uses of the message (de)serialization layer, even those that aren't any sort of trust boundary. If the actual filtering is going to happen in the dbus-daemon rather than in the message loader, then the part about trust boundaries makes sense, but the implementation gets a bit weird: we'd have to add a (possibly private) method on DBusServer, dbus_server_promise_to_filter_messages() or something, and make the message bus call it; then have that feed through the DBusConnection into the DBusTransport for every client connection that the server accepts. An alternative is to have clients of the dbus-daemon call a method on the dbus-daemon, either RequestHeaderFiltering() or RequestFeatures([...]) or even Properties.Get(DBUS_INTERFACE_DBUS, 'Features'). We can receive incoming messages before we have done this, and for those messages we have to make the conservative assumption that headers won't be filtered. However, in practice we won't normally get messages until we have called either AddMatch() or RequestName(), and if we do, it seems fair to say that those messages just won't get the extra headers. Let's think about the use cases for new header fields. I said in Comment #8 that we should probably avoid sending newly-invented header fields, at least in unicast messages, until a client asks for them: that way, clients that don't care don't get overhead, and clients that do care have an incentive to call the appropriate method to express an interest, which serves as an implicit assertion that this dbus-daemon does the necessary filtering. So perhaps we don't even need explicit feature discovery for this, because for performance reasons we'll be making them opt-in anyway? For the hypothetical SENDER_UNIX_UID and SENDER_LINUX_SECURITY_LABEL header fields, I expect 99.9% of users of those fields will be services that receive method calls, and check for permission to take the action denoted by the method call by either calling out to polkit, or implementing polkit-like policies internally. This means we probably don't have to care about broadcasts here. I think it would be reasonable to expect those services to call some sort of TellMeCredentials() method before requesting their well-known bus names, so that all non-abusive method calls will come in after the TellMeCredentials() call returns: then they can know that they can expect (and trust) the credentials headers iff TellMeCredentials() has succeeded. For Bug #101899, we do care about broadcasts. However, the only thing a connection could do by forging the new header that could be construed to be a privilege increase would be to escalate from "I will not say whether this is in a container or not" (no header) to "definitely not in a container" (header = "/"), and even that won't be possible unless it has unrestricted access to the bus (without being filtered by e.g. flatpak-dbus-proxy), in which case it's more or less legitimate for it to claim not to be in a container. > > (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've run out of time today, but I did fix up the documentation of that handshake a bit for NEGOTIATE_UNIX_FD technical debt, and I'll try to attach the results of that tomorrow. Putting NEGOTIATE_UNIX_FD at the end already made the handshake rather weird IMO - it added an extra step at the point where the server would previously have guaranteed that the next thing it would say would be either an error message or the D-Bus message stream - but it's too late to do anything about that now! (In reply to Simon McVittie from comment #38) > I've run out of time today, but I did fix up the documentation of that > handshake a bit for NEGOTIATE_UNIX_FD technical debt Bug #104224. Created attachment 136098 [details] [review] [1/7] spec: Allow non-message-bus servers to use SENDER and DESTINATION The Telepathy "Tubes" APIs are an example of a server that is not a message bus, but makes use of the sender and destination fields to provide broadly unique-connection-name-like semantics. Created attachment 136099 [details] [review] [2/7] spec: Recommend that relaying servers filter header fields This is an interpretation of the existing text. There are two plausible ways a relaying server could interpret "must ignore [new] fields": it could pass them through as-is, or it could delete them before relaying. Until now, the reference implementation has done the former. However, this behaviour is difficult to defend. If a server relays messages without filtering out header fields that it doesn't understand, then a client can't know whether the header field was supplied by the server, or whether it was supplied by a (possibly malicious) fellow client. We can't introduce useful round-trip-reducing header fields like SENDER_UNIX_USER_ID or SENDER_LINUX_SECURITY_LABEL until the message bus filters them out, *and* provides a way for clients to know for sure that it has done so. This is a step towards that feature. Created attachment 136100 [details] [review] [3/7] Add a test for header fields Created attachment 136101 [details] [review] [4/7] _dbus_message_remove_unknown_fields: Add Created attachment 136102 [details] [review] 5/7] header-fields test: Exercise _dbus_message_remove_unknown_fields Created attachment 136103 [details] [review] [6/7] dbus-daemon: Filter out unknown header fields Created attachment 136104 [details] [review] [7/7] tests: Assert that dbus-daemon filters unknown header fields The spec patches depend on [some of] the SASL documentation improvements from Bug #104224, although they could probably be rebased to disentangle them. Also available on github: https://github.com/smcv/dbus/commits/new-header-fields-100317 (commits since "spec: Describe the EXTERNAL and ANONYMOUS auth mechanisms"). Created attachment 136106 [details] [review] [6 v2] dbus-daemon: Filter out unknown header fields --- spec: Document HeaderFiltering as a suitable way for a client to check that the server will not relay untrustworthy header fields. My original plan had been to use a new NEGOTIATE_HEADER_FILTERING for this, but I now think that's the wrong layer. Created attachment 136107 [details] [review] [7] tests: Assert that dbus-daemon filters unknown header fields --- Unchanged, just attaching to fix the sequence Created attachment 136108 [details] [review] 8/8] spec: Document the design principle that new headers must be asked for --- This seems a bit weird to be putting in the spec - it isn't really constraining servers or clients - but at least this way the design principle is documented. I intend to follow this for Bug #101899. The wording is a little vague: it doesn't say what we will do about a broadcast (or the deprecated eavesdropping mechanism) that is going to be delivered to multiple clients, some of which want the new header field SENDER_HAT_COLOUR and some of which do not. We can either set all the header fields that are wanted by at least one of the clients, or copy the message and deliver modified copies to the clients that want new header fields. My inclination would be to say it's OK for broadcasts to carry more headers than were requested, which would avoid needing to do copying. If we do copy the message, then we'll need a _dbus_message_copy(), and perhaps a way to share refcounted message bodies so we only actually have to *copy* the header (if we're concerned that broadcast message bodies might be large). Comment on attachment 136098 [details] [review] [1/7] spec: Allow non-message-bus servers to use SENDER and DESTINATION Review of attachment 136098 [details] [review]: ----------------------------------------------------------------- Seems reasonable. Comment on attachment 136099 [details] [review] [2/7] spec: Recommend that relaying servers filter header fields Review of attachment 136099 [details] [review]: ----------------------------------------------------------------- OK. Comment on attachment 136100 [details] [review] [3/7] Add a test for header fields Review of attachment 136100 [details] [review]: ----------------------------------------------------------------- Bit of a saga to review, but looks OK to me aside from one comment. ::: test/header-fields.c @@ +1,1 @@ > +/* Targeted unit tests for OOM paths in DBusMessage Not sure this first comment line is relevant. Copy-pasta? Comment on attachment 136101 [details] [review] [4/7] _dbus_message_remove_unknown_fields: Add Review of attachment 136101 [details] [review]: ----------------------------------------------------------------- Looks reasonable. Comment on attachment 136102 [details] [review] 5/7] header-fields test: Exercise _dbus_message_remove_unknown_fields Review of attachment 136102 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-marshal-header.c @@ -1559,5 @@ > } > - else > - { > - _dbus_type_reader_next (&sub); > - } Why remove this? Comment on attachment 136106 [details] [review] [6 v2] dbus-daemon: Filter out unknown header fields Review of attachment 136106 [details] [review]: ----------------------------------------------------------------- Looks good apart from this one comment. ::: bus/dispatch.c @@ +292,5 @@ > + * don't understand (or validate), so that we can add header fields > + * in future and clients can assume that we have checked them. */ > + if (!_dbus_message_remove_unknown_fields (message)) > + { > + BUS_SET_OOM (&error); Is OOM really the right error code here? Might make sense to also print a warning, since otherwise any failure here is going to go unnoticed (or get confused with actual OOM failures). Comment on attachment 136107 [details] [review] [7] tests: Assert that dbus-daemon filters unknown header fields Review of attachment 136107 [details] [review]: ----------------------------------------------------------------- Yup. Comment on attachment 136108 [details] [review] 8/8] spec: Document the design principle that new headers must be asked for Review of attachment 136108 [details] [review]: ----------------------------------------------------------------- The content seems OK, although the whole paragraph is only 3 sentences, so feels a bit long and rambly. It might be improved by splitting up into a few more sentences. Comment on attachment 136102 [details] [review] 5/7] header-fields test: Exercise _dbus_message_remove_unknown_fields Review of attachment 136102 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-marshal-header.c @@ -1559,5 @@ > } > - else > - { > - _dbus_type_reader_next (&sub); > - } The part removed here should have been squashed into 4/7, not 5/7. In the case where we don't remove the header field, advancing the reader to the variant, recursing into it and updating the cache is valid but useless. In the case where we do remove the header field, the assertion "_dbus_assert (_dbus_type_reader_get_current_type (&sub) == DBUS_TYPE_VARIANT);" is going to fail, because sub is now invalid. Comment on attachment 136106 [details] [review] [6 v2] dbus-daemon: Filter out unknown header fields Review of attachment 136106 [details] [review]: ----------------------------------------------------------------- ::: bus/dispatch.c @@ +292,5 @@ > + * don't understand (or validate), so that we can add header fields > + * in future and clients can assume that we have checked them. */ > + if (!_dbus_message_remove_unknown_fields (message)) > + { > + BUS_SET_OOM (&error); Yes, OOM is the right error code. The only reason _dbus_message_remove_unknown_fields() can legitimately fail is if it runs out of memory while manipulating the DBusString. (In reply to Simon McVittie from comment #47) > The spec patches depend on [some of] the SASL documentation improvements > from Bug #104224, although they could probably be rebased to disentangle > them. Now disentangled, in fact. Created attachment 136287 [details] [review] [3a/8] squash! Add a test for header fields --- To be squashed into Attachment #136100 [details]. Use g_test_slow() instead of reinventing it. Do some limited OOM testing even in quick mode (it takes about a second per test on my laptop, so hopefully no more than a minute even when run on a potato). In slow mode, each of the four tests takes about 150 seconds on my laptop - when I say slow, I mean it. Send a method call that expects a reply, and stop waiting for it to be delivered to the other side if we get an error reply. Otherwise, we'll wait forever in the case where our attempt to send the message is the thing that runs out of memory. Correct copypasta in comment. Created attachment 136288 [details] [review] [4/8 v2] _dbus_message_remove_unknown_fields: Add --- Remove some useless/incorrect/crashy code. I've done this as a patch v2 because I think that's going to be easier to review than the delta. Created attachment 136289 [details] [review] [5/8 v2] header-fields test: Exercise _dbus_message_remove_unknown_fields --- Now without the bits that were meant to be squashed into patch 4. Created attachment 136290 [details] [review] [8/8 v2] spec: Document the design principle that new headers must be asked for --- Rewrite Created attachment 136296 [details] [review] [4/8 v3] _dbus_message_remove_unknown_fields: Add --- Don't advance the array-reader if we delete a header field: in this case, the beginning of the next header field has occupied the space previously used for the deleted header field, so advancing the pointer would incorrectly cause us to not delete it. Created attachment 136297 [details] [review] [9/9] header-fields test: Assert that we can remove multiple unknown fields --- This reproduces the bug in Attachment #136288 [details], which is fixed by Attachment #136296 [details]. (In reply to Simon McVittie from comment #66) > Don't advance the array-reader if we delete a header field: in this > case, the beginning of the next header field has occupied the space > previously used for the deleted header field, so advancing the > pointer would incorrectly cause us to not delete it. By which I meant: ... to not check the validity of the next header field. In particular we wouldn't delete the next field if it too is unknown, i.e. if there were two consecutive unknown header fields. Comment on attachment 136287 [details] [review] [3a/8] squash! Add a test for header fields Review of attachment 136287 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 136289 [details] [review] [5/8 v2] header-fields test: Exercise _dbus_message_remove_unknown_fields Review of attachment 136289 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 136296 [details] [review] [4/8 v3] _dbus_message_remove_unknown_fields: Add Review of attachment 136296 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 136297 [details] [review] [9/9] header-fields test: Assert that we can remove multiple unknown fields Review of attachment 136297 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 136290 [details] [review] [8/8 v2] spec: Document the design principle that new headers must be asked for Review of attachment 136290 [details] [review]: ----------------------------------------------------------------- Nice, r+ Fixed in git for 1.13.0 |
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.
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 :-(