The interaction between match rules and security policies is rather crazy. While trying to debug why an eavesdropping dbus-monitor didn't receive method replies or some method calls, I changed the logging in dbus-daemon to log all failed deliveries, even if eavesdropped or otherwise matched via match rules. I had naively assumed that the rules for delivery of a single message went like this: 1. Take the destination of the message, if it isn't a broadcast; call this the *addressed recipient* 2. List the clients whose match rules indicate that they are interested in the message; add the addressed recipient, if any, to this list. Call them the *proposed recipients*. If the message is not a broadcast, every proposed recipient except the addressed recipient is said to be *eavesdropping*. 3. If the message is a broadcast, decide whether the sender is allowed to send this broadcast (policy rules that specify a destination are skipped). If it is not allowed to do this, syslog and drop the message on the floor. 4. If the message is not a broadcast, it has an addressed recipient. Decide whether the sender is allowed to send the message to the addressed recipient. If not, syslog and drop the message on the floor. 5. For each proposed recipient, decide whether it is allowed to receive the message, taking into account whether it is eavesdropping. If not, skip this recipient. However, in the real implementation, send rules are applied individually to each proposed recipient, so steps 4 and 5 are replaced with: 4'. For each proposed recipient, decide whether the sender would have been allowed to send to that recipient. If not, skip this recipient. 5'. For each proposed recipient to which the sender is allowed to send, decide whether it is allowed to receive the message. If not, skip this recipient.
One effect of the rules as implemented is that it's problematic to write a rule that says, for instance, "root may eavesdrop on everything". You have to allow sending all messages to root, which makes root-owned daemons vulnerable to method calls which would otherwise be denied by their security policy files. For instance, if you set things up so root's dbus-monitor can eavesdrop all messages, then all users can call SetHostName on Avahi, call methods on BlueZ and its agents, call all ConsoleKit methods, call all dnsmasq methods, and I stopped after "d" in my /etc/dbus-1/system.d because I think I've proved my point already. If what you're eavesdropping to debug is a permissions problem, then enabling eavesdropping like that might even make it go away! Possible workarounds: 1) require that sysadmins wanting to debug their system bus add a dedicated user to run dbus-monitor, and allow sending messages to that user 2) recommend that sysadmins wanting to debug their system bus allow the user who runs the system dbus-daemon (e.g. 'messagebus' on Debian) to eavesdrop, receive everything and have everything sent to them, then run dbus-monitor as that user via su or whatever 3) allow the system user who runs dbus-daemon to eavesdrop by default, because they could eavesdrop just as well by ptrace'ing the dbus-daemon
Another effect of the rules as implemented is that the send and receive rules are more or less redundant: they're always checked in pairs, with the same "parameters".
(In reply to comment #1) > Possible workarounds: > > 1) require that sysadmins wanting to debug their system bus add a dedicated > user to run dbus-monitor, and allow sending messages to that user This (and the other workarounds I suggested) doesn't actually work, because "uid of the proposed recipient" isn't one of the things that can appear in an allow rule. You can say <policy user="root"> <allow receive_type="method_call" eavesdrop="true"/> </policy> but you can't say <policy context="default"> <allow send_type="method_call" if_recipient_uid="root"/> </policy> One possible way around this is to make use of the odd semantics of destination attributes. A destination attribute matches if and only if the recipient is *in the queue for* the name - whether it's the owner or further down the queue is irrelevant. So we could do something like this: <policy user="root"> <!-- or SELinux credentials or whatever --> <allow own="org.freedesktop.DBus.Eavesdropping"/> <allow receive_destination="method_call" eavesdrop="true"/> <allow receive_type="method_return" eavesdrop="true"/> <allow receive_type="error" eavesdrop="true"/> <allow receive_type="signal" eavesdrop="true"/> </policy> <policy context="default"> <allow send_destination="org.freedesktop.DBus.Eavesdropping"/> </policy> and make dbus-monitor, Bustle and similar eavesdropping tools request org.freedesktop.DBus.Eavesdropping on startup.
I'd still prefer the control channel approach.
(In reply to comment #4) > I'd still prefer the control channel approach. I know, implementations welcome. I'm not going to implement it myself if it will go unreviewed for as long as Bug #39197, Bug #39196, Bug #38252 have...
Here is a mad idea I had while commenting on <http://www.piware.de/2013/09/how-to-watch-system-d-bus-method-calls/>: we don't actually need a separate socket in the filesystem. Now that we have Unix fd passing, dbus-daemon could have an Eavesdrop() method which returns a fd that is one end of a socketpair() carrying a copy of the messages.
(In reply to comment #6) > Here is a mad idea I had while commenting on > <http://www.piware.de/2013/09/how-to-watch-system-d-bus-method-calls/>: we > don't actually need a separate socket in the filesystem. Now that we have > Unix fd passing, dbus-daemon could have an Eavesdrop() method which returns > a fd that is one end of a socketpair() carrying a copy of the messages. That sounds fine to me too.
*** Bug 45914 has been marked as a duplicate of this bug. ***
(In reply to Simon McVittie from comment #6) > Now that we have > Unix fd passing, dbus-daemon could have an Eavesdrop() method which returns > a fd that is one end of a socketpair() carrying a copy of the messages. I've started some work on this. Throughout this message please read "syslog" as "an appropriate place for logs, depending on configuration" - in practice it's the Journal on systemd systems, syslog for the system bus, or stderr for the session bus. Initial design -------------- There's a new o.fd.DBus.Monitoring.BecomeMonitor() method. This irrevocably turns your connection into a "monitor". Monitors lose all their bus names (including the unique name!) so that nobody can send them unicast messages any more. They also lose all pending reply tracking and all match rules. (Rationale: having "monitors" be special connections is borrowed from kdbus. It ensures that developers won't try to do their monitoring on the same connection they use for other things, which would lead to confusion in practice. I'll recommend use of a private connection for monitoring when I document this.) It is forbidden for a monitor to send messages: in libdbus and GDBus, this means all monitors must install a filter that returns HANDLED (I'm not going to bother providing convenience API for this, people writing D-Bus monitoring tools should know this already). If it does send a message, the dbus-daemon logs to syslog and disconnects it. (Rationale: this ensures that if a monitor is buggy and tries to reply to other people's messages, its developer will notice after even the slightest bit of testing, and fix it.) To mitigate broken bus configurations like CVE-2014-8148, only uid 0 and the uid of the dbus-daemon are allowed to call BecomeMonitor(), even if the bus configuration says otherwise. Developers can use "sudo [-u messagebus] dbus-monitor" or similar. When dbus-daemon receives messages, as part of the message's transaction, a copy is sent to every monitor. <allow> and <deny> rules are ignored - if you want to lock down monitoring via <deny>, do so by locking down access to the Monitoring interface instead. This "capture" is considerably earlier than when eavesdropping takes place. In particular, for the message that activates an activated service, monitors see it at the same time that dbus-daemon attempts to activate the service, whereas eavesdroppers only see it after the activated service starts. I also considered sending the activation message to monitors twice - when it is received, and when eavesdroppers would see it - but I think that would be confusing. When dbus-daemon sends a self-generated message, as part of the message's transaction, a copy is sent to every monitor. Again, <allow> and <deny> rules are ignored. When dbus-daemon fails to complete a transaction because it ran out of memory, or fails to send a message to monitors because it ran out of memory, it logs to syslog. (Rationale: developers should be able to rely on the monitor log being complete, unless complaints in the system log say otherwise.) This is as far as my implementation has got so far. I'm still adding cases to the regression test to make sure we capture everything (notably, I need to add activation) but it seems to work. Enhancements ------------ The most obvious is that I need to enhance dbus-monitor to use this new API, falling back to AddMatch on older buses. BecomeMonitor() should be BecomeMonitor(filters: array of string) -> void, with BecomeMonitor([""]) being the common case. The filters would be in AddMatch syntax; BecomeMonitor([]) would be a shortcut for BecomeMonitor([""]). It would be nice to have GetMonitor(filters: array of string) -> pipe: fd on Unix platforms, but I would like it to use a pipe rather than a Unix socket to avoid fd-passing Unix sockets over Unix sockets and to enforce the read-only nature of the pipe, and I think that would need DBusTransport work (to be able to have transports that are considered pre-authenticated and do not have to do the handshake, which requires bidirectionality). It would be nice to have a way to configure a shell command-line in /etc/dbus-1/system.conf, with the semantics that dbus-daemon forks it off before doing anything else, and gives it the readable end of a pipe of D-Bus messages as stdin. This lets you buffer the complete messages from the beginning of boot onwards. For Bustle users, we could have a tool that reads D-Bus message framing on stdin, splices in pcap framing, and writes to stdout. I don't think dbus-daemon needs to be able to do this. Questions --------- I need to decide whether monitors see NameAcquired and NameLost. They probably shouldn't, because those signals are (a) somewhat special, and (b) entirely redundant with NameOwnerChanged. At the moment, if dbus-daemon does not allow itself to send a broadcast, monitors do not get an AccessDenied error. Possibly they should?
(In reply to Simon McVittie from comment #9) > BecomeMonitor() should be BecomeMonitor(filters: array of string) -> void, > with BecomeMonitor([""]) being the common case. The filters would be in > AddMatch syntax; BecomeMonitor([]) would be a shortcut for > BecomeMonitor([""]). Or in fact BecomeMonitor(as, u) with an unused flags parameter. I've implemented that, but not fully tested it yet. > It would be nice to have a way to configure a shell command-line in > /etc/dbus-1/system.conf, with the semantics that dbus-daemon forks it off > before doing anything else, and gives it the readable end of a pipe of D-Bus > messages as stdin. This lets you buffer the complete messages from the > beginning of boot onwards. This doesn't *strictly* need to be a pipe: dbus-daemon could fork off a helper process (a dbus-monitor?) that starts before the main DBusServer comes up, takes a socket as its stdin, reads D-Bus messages, and emits them on its stdout (which could, if desired, be a pipe to whatever you want). > For Bustle users, we could have a tool that reads D-Bus message framing on > stdin, splices in pcap framing, and writes to stdout. That could also be dbus-monitor. Straw man: dbus-monitor --binary ... write D-Bus message stream to stdout dbus-monitor --pcap ... write pcap packets to stdout > I need to decide whether monitors see NameAcquired and NameLost. They > probably shouldn't, because those signals are (a) somewhat special, and (b) > entirely redundant with NameOwnerChanged. At the moment they do see those signals, FWIW.
Created attachment 112739 [details] [review] [01/10] socket_set_epoll_add: initialize all bytes of struct epoll_event This should be a no-op, but it shuts Valgrind up. --- The reason for the warning is that we fill in event.events and event.data.fd, but the union event.data actually contains more bytes than that. We'll get the same partially initialized union back from the kernel in socket_set_epoll_poll(), where we take events[i].data.fd and ignore the rest. So the current code is safe, but valgrind is right to worry. (I'll move that text into the commit message when I commit) Not really related to this at all, but I tried valgrinding and it complained.
Created attachment 112740 [details] [review] [02/10] bus: exit on fatal errors even if not syslogging --- Also just something I noticed while I was there - I think it was previously pointed out on the mailing list or some such, but does not have a real bug report. bus_context_log() with DBUS_SYSTEM_LOG_FATAL is currently not actually fatal unless <syslog/> is enabled.
Created attachment 112742 [details] [review] [03/10] bus driver: factor out bus_driver_check_caller_is_privileged Unlike the initial mitigation for CVE-2014-8148, this does allow uid 0 to call UpdateActivationEnvironment. There's no point in root doing that, but there's also no reason why it's particularly bad - if an attacker is uid 0 we've already lost - and it simplifies use of this function for future things that do want to be callable by root. --- I still need to test this on a real system bus with multiple uids.
Created attachment 112743 [details] [review] [04/10] Add support for morphing a D-Bus connection into a "monitor" This is a special connection that is not allowed to send anything, and loses all its well-known names. In future commits, it will get a new set of match rules and the ability to eavesdrop on messages before the rest of the bus daemon has had a chance to process them.
Created attachment 112744 [details] [review] [05/10] Factor out some utility functions from test/dbus-daemon*
Created attachment 112745 [details] [review] [06/10] Capture all messages received or sent, and send them to monitors Unlike eavesdropping, the point of capture is when the message is received, except for messages originating inside the dbus-daemon.
Created attachment 112746 [details] [review] [07/10] Add a regression test for becoming a monitor This includes most of the situations I could think of: * method call on dbus-daemon and response * NameOwnerChanged * NameAcquired, NameLost (although I'm not 100% sure these should get captured, since they're redundant with NameOwnerChanged) * unicast message is allowed through * unicast message is rejected by no-sending or no-receiving policy * broadcast is allowed through * broadcast is rejected by no-sending policy (the error reply is also captured) * broadcast is rejected by no-receiving policy (there is no error reply) * message causing service activation, and the message telling systemd to do the actual activation * systemd reporting that activation failed Because some of the code here is systemd-specific, I wrote a test-case for systemd activation, which mostly addresses #57952 as a bonus. It does not cover: * sending a message to dbus-daemon, then provoking a reply, then dbus-daemon does not allow itself to send the reply due to its own security policy This is such an obscure corner case that I don't think it can be tested without using a second uid.
Created attachment 112747 [details] [review] [08/10] dbus-monitor: add support for using BecomeMonitor to be a read-only monitor Move the dbus_connection_add_filter() call further up as a precaution, because it isn't safe for a monitor to not have a filter that swallows all messages.
Created attachment 112748 [details] [review] [09/10] dbus-monitor: add options to log binary data with or without pcap framing --- The --bustle option (which is pcap with a slightly wrong header, and should be precisely the same thing that Bustle logs) is deliberately undocumented :-)
Created attachment 112749 [details] [review] [10/10] lcov: use builddir, not srcdir It seems lcov (or gcc?) has changed its paths since last time this worked.
Still to do: * patch it into a real system bus and test with multiple uids involved * add a rule to the default system.conf allowing root (only) to monitor it
Created attachment 112856 [details] [review] [PATCH 1/6] Add support for morphing a D-Bus connection into a "monitor" This is a special connection that is not allowed to send anything, and loses all its well-known names. In future commits, it will get a new set of match rules and the ability to eavesdrop on messages before the rest of the bus daemon has had a chance to process them. --- Various commits have moved to Bug #88808, Bug #88810, Bug #59752.
Created attachment 112857 [details] [review] [PATCH 2/6] Capture all messages received or sent, and send them to monitors Unlike eavesdropping, the point of capture is when the message is received, except for messages originating inside the dbus-daemon.
Created attachment 112858 [details] [review] [PATCH 3/6] Add a regression test for becoming a monitor This includes most of the situations I could think of: * method call on dbus-daemon and response * NameOwnerChanged * NameAcquired, NameLost (although I'm not 100% sure these should get captured, since they're redundant with NameOwnerChanged) * unicast message is allowed through * unicast message is rejected by no-sending or no-receiving policy * broadcast is allowed through * broadcast is rejected by no-sending policy (the error reply is also captured) * broadcast is rejected by no-receiving policy (there is no error reply) * message causing service activation, and the message telling systemd to do the actual activation * systemd reporting that activation failed Because some of the code here is systemd-specific, I wrote a test-case for systemd activation, which mostly addresses #57952 as a bonus. It does not cover: * sending a message to dbus-daemon, then provoking a reply, then dbus-daemon does not allow itself to send the reply due to its own security policy This is such an obscure corner case that I'm not even convinced it's testable without dropping down into lower-level socket manipulation: dbus-daemon's replies are always assumed to be requested replies, and replies contain so little other metadata that I think we can only forbid them by forbidding all method replies. If we do that, the reply to Hello() won't arrive and the client-side connection will not become active. --- Now requires Bug #59752 for the sd-activation test infrastructure.
Created attachment 112859 [details] [review] [PATCH 4/6] dbus-monitor: add support for using BecomeMonitor to be a read-only monitor Move the dbus_connection_add_filter() call further up as a precaution, because it isn't safe for a monitor to not have a filter that swallows all messages.
Created attachment 112860 [details] [review] [PATCH 5/6] dbus-monitor: add options to log binary data with or without pcap framing
Created attachment 112861 [details] [review] [7/7] Allow root to monitor the system bus by default --- New.
Comment on attachment 112856 [details] [review] [PATCH 1/6] Add support for morphing a D-Bus connection into a "monitor" Review of attachment 112856 [details] [review]: ----------------------------------------------------------------- Note (applies to all my reviews): I do not know D-Bus’ security model well enough to give a security review of these patches. My reviews are for the code only. ::: bus/connection.c @@ +63,4 @@ > DBusTimeout *expire_timeout; /**< Timeout for expiring incomplete connections. */ > int stamp; /**< Incrementing number */ > BusExpireList *pending_replies; /**< List of pending replies */ > + DBusList *monitors; /**< List of all monitoring connections, a subset of completed */ Would be useful to give the actual generic type here: unowned DBusConnection*. @@ +2555,5 @@ > + if (!bus_service_remove_owner (service, connection, transaction, error)) > + { > + _dbus_list_free_link (link); > + _dbus_list_clear (&tmp); > + return FALSE; If we hit this failure case, the connection will end up in an intermediate state where it’s not a monitor, but has lost half its services. I don’t suppose there’s any way to avoid this? ::: bus/dispatch.c @@ +227,5 @@ > + */ > + bus_context_log (context, DBUS_SYSTEM_LOG_WARNING, > + "Monitoring connection %s is not allowed to send " > + "messages; closing it. Please fix the monitor " > + "to not do that.", sender); Could we include some details of the message in this log message? Would make fixing the monitor easier.
Comment on attachment 112857 [details] [review] [PATCH 2/6] Capture all messages received or sent, and send them to monitors Review of attachment 112857 [details] [review]: ----------------------------------------------------------------- ::: bus/connection.c @@ +2196,5 @@ > + return TRUE; > + > + reply = dbus_message_new_error (in_reply_to, > + error->name, > + error->message); Need to check for OOM here. @@ +2624,5 @@ > > +static dbus_bool_t > +bcd_add_monitor_rules (BusConnectionData *d, > + DBusConnection *connection, > + DBusList **rules) Niggle: Parameters are not formatted as other methods in this file (spacing before asterisks). @@ +2646,5 @@ > + { > + if (!bus_matchmaker_add_rule (mm, iter->data)) > + { > + bus_matchmaker_disconnected (mm, connection); > + return FALSE; Do we want to clear d->connections->monitor_matchmaker on this path? ::: bus/dispatch.c @@ +321,5 @@ > + > + if (!bus_transaction_capture (transaction, connection, message)) > + { > + BUS_SET_OOM (&error); > + goto out; Worth documenting somewhere that monitoring adds the side-effect of failing the connection on OOM in the monitoring code. Pretty obvious, but we should be explicit that the monitoring code is not side-effect-free. I think you have stated this before, though. ::: doc/dbus-specification.xml @@ +4644,5 @@ > </para> > + > + <para> > + Eavesdropping interacts poorly with buses with non-trivial > + access control restrictions. The ‘with non-trivial access control restrictions, such as the system bus.’? @@ +6273,5 @@ > + Converts the connection into a <emphasis>monitor > + connection</emphasis> which can be used as a debugging/monitoring > + tool. Only a user who is privileged on this > + bus (by some implementation-specific definition) may create > + monitor connections. Is it worthwhile clarifying this with a (non-normative) example of the definition (e.g. being root)? @@ +6287,5 @@ > + <para> > + Monitor connections may receive all messages, even messages that > + should only have gone to some other connection ("eavesdropping"). > + The first argument is a list of match rules, which replace any > + match rules that were previously active for this connection. Should document that an empty rules parameter is equivalent to a single, empty match rule and matches everything.
Comment on attachment 112858 [details] [review] [PATCH 3/6] Add a regression test for becoming a monitor Review of attachment 112858 [details] [review]: ----------------------------------------------------------------- ::: test/monitor.c @@ +1,3 @@ > +/* Integration tests for monitor-mode D-Bus connections > + * > + * Copyright © 2010-2011 Nokia Corporation The © symbol is corrupted for me, but this could be a Bugzilla/Splinter problem. @@ +450,5 @@ > + DBusMessage **message_p = data; > + > + *message_p = dbus_pending_call_steal_reply (pc); > + g_assert (*message_p != NULL); > +} You have previously factored this out as test_pending_call_store_reply() in bug #88810. @@ +550,5 @@ > + g_assert_cmpint (ret, ==, DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER); > + > + while (!got_unique || !got_a || !got_b || !got_c) > + { > + test_main_context_iterate (f->ctx, TRUE); It might make sense to add a timeout to this loop so the test can’t continue forever if there’s a bug. @@ +605,5 @@ > + become_monitor (f); > + > + while (!lost_unique || !lost_a || !lost_b || !lost_c) > + { > + test_main_context_iterate (f->ctx, TRUE); Same here: timeout? @@ +660,5 @@ > + g_assert (!dbus_connection_get_is_connected (f->monitor)); > + > + while (TRUE) > + { > + test_main_context_iterate (f->ctx, TRUE); And here, throughout the rest of the file, etc. @@ +769,5 @@ > + "BroadcastSignal1", "", NULL); > + dbus_message_unref (m); > + > + /* FIXME: dbus-daemon does not fake a denial message for the monitor's > + * benefit; should it? */ Not sure this is addressed in a later patch. Should it be? I can imagine the monitor being used to diagnose exactly these kinds of denial issues, so I guess a message should be faked.
Comment on attachment 112859 [details] [review] [PATCH 4/6] dbus-monitor: add support for using BecomeMonitor to be a read-only monitor Review of attachment 112859 [details] [review]: ----------------------------------------------------------------- ::: tools/Makefile.am @@ +31,4 @@ > dbus-print-message.h \ > dbus-send.c > > +dbus_monitor_SOURCES= \ Niggle: Missing a space before the ‘=’. @@ +35,5 @@ > + dbus-monitor.c \ > + dbus-print-message.c \ > + dbus-print-message.h \ > + tool-common.c \ > + tool-common.h \ Could we factor out the migration to tool-common into a separate patch? Would make things a little clearer. ::: tools/dbus-monitor.c @@ +237,5 @@ > > +static dbus_bool_t > +become_monitor (DBusConnection *connection, > + int numFilters, > + char **filters) Shouldn’t that be (const char * const *)?
Comment on attachment 112860 [details] [review] [PATCH 5/6] dbus-monitor: add options to log binary data with or without pcap framing Review of attachment 112860 [details] [review]: ----------------------------------------------------------------- ::: doc/dbus-monitor.1.xml.in @@ +56,5 @@ > + entire binary message stream (without the initial authentication handshake). > + The PCAP mode, selected by <literal>--pcap</literal>, adds a > + PCAP file header to the beginning of the output, and prepends a PCAP > + message header to each message; this produces a binary file that can > + be read by, for instance, Wireshark.</para> What about the --bustle option? ::: tools/dbus-monitor.c @@ +47,5 @@ > + > +/* http://www.tcpdump.org/linktypes.html */ > +#define LINKTYPE_DBUS 231 > +/* what Bustle uses */ > +#define LINKTYPE_NULL 0 Is that something which needs fixing in Bustle? @@ +305,4 @@ > static void > usage (char *name, int ecode) > { > + fprintf (stderr, "Usage: %s [--system | --session | --address ADDRESS] [--monitor | --profile | --pcap | --binary ] [watch expressions]\n", name); What about --bustle here? @@ +604,5 @@ > + 0, /* capture in GMT */ > + 0, /* no opinion on timestamp precision */ > + (1 << 27), /* D-Bus spec says so */ > + LINKTYPE_DBUS > + }; Can we have some documentation for this header format, such as a link to the pcap docs?
Comment on attachment 112861 [details] [review] [7/7] Allow root to monitor the system bus by default Review of attachment 112861 [details] [review]: ----------------------------------------------------------------- ++
(In reply to Simon McVittie from comment #23) > Unlike eavesdropping, the point of capture is when the message is > received, except for messages originating inside the dbus-daemon. Note to self: kdbus has a very similar monitoring concept, and I need to go through what they capture and what I capture, and open bugs (in whichever project I think is less correct) where they differ.
(In reply to Philip Withnall from comment #28) > Would be useful to give the actual generic type here: unowned > DBusConnection*. Good point. > If we hit this failure case, the connection will end up in an intermediate > state where it’s not a monitor, but has lost half its services. I don’t > suppose there’s any way to avoid this? I don't think so, but it can only happen on OOM so I'm not really very concerned. > Could we include some details of the message in this log message? Would make > fixing the monitor easier. Maybe. I'll look into the complain_about_message() function we use for <deny/> rules and see whether I can factor something out. (I assume this is not a merge-blocker though.) In practice, the fix for the monitor is always the same: "install a filter that says HANDLED for every message". (In reply to Philip Withnall from comment #29) > > + reply = dbus_message_new_error (in_reply_to, > > + error->name, > > + error->message); > > Need to check for OOM here. Good catch. > Niggle: Parameters are not formatted as other methods in this file (spacing > before asterisks). I hate that spacing, but yes, I should be consistent. > > + if (!bus_matchmaker_add_rule (mm, iter->data)) > > + { > > + bus_matchmaker_disconnected (mm, connection); > > + return FALSE; > > Do we want to clear d->connections->monitor_matchmaker on this path? No we don't. There is only one monitor matchmaker per process (d->connections is a pointer to something that is, in practice, a singleton) - I'm just allocating it lazily because it's an easy way to reduce overhead in the common case of 0 monitors. If you have ever had >= 1 monitor (or strictly speaking, if you have ever got at least this far into making a monitor before failing), then you have the monitor matchmaker forever. > Worth documenting somewhere that monitoring adds the side-effect of failing > the connection on OOM in the monitoring code. Pretty obvious, but we should > be explicit that the monitoring code is not side-effect-free. I think you > have stated this before, though. I think I stated it somewhere but possibly not in the code. I'll check. > > + Eavesdropping interacts poorly with buses with non-trivial > > + access control restrictions. The > > ‘with non-trivial access control restrictions, such as the system bus.’? Good point. > > + tool. Only a user who is privileged on this > > + bus (by some implementation-specific definition) may create > > + monitor connections. > > Is it worthwhile clarifying this with a (non-normative) example of the > definition (e.g. being root)? Probably, yes. "For instance, in the reference implementation of a message bus, the user ID running the dbus-daemon and the Unix root user are considered to be privileged" in a footnote, perhaps. > Should document that an empty rules parameter is equivalent to a single, > empty match rule and matches everything. Good point. (In reply to Philip Withnall from comment #30) > The © symbol is corrupted for me, but this could be a Bugzilla/Splinter > problem. It is. I'll verify that all files are valid UTF-8 before pushing. > You have previously factored this out as test_pending_call_store_reply() in > bug #88810. Yeah, you might have noticed that I did some of this out-of-order :-) I'll fix that. > It might make sense to add a timeout to this loop so the test can’t continue > forever if there’s a bug. I might try to steal the timeout machinery from telepathy-glib, which is pretty reliable (it uses the GLib main loop, with alarm() as an extra safety net on Unix). In telepathy-glib I never bothered with a timeout on individual loops because that would be rather verbose, I just imposed a (generous) timeout on the test as a whole. This is complicated by the fact that dbus may or may not in fact be using the GLib main loop here (it's complicated)... I might just put in the alarm() behind an #ifdef, with the caveat "if you're on Windows, the test might signal failure by hanging forever". Reasonable? > @@ +769,5 @@ > > + "BroadcastSignal1", "", NULL); > > + dbus_message_unref (m); > > + > > + /* FIXME: dbus-daemon does not fake a denial message for the monitor's > > + * benefit; should it? */ > > Not sure this is addressed in a later patch. Should it be? > > I can imagine the monitor being used to diagnose exactly these kinds of > denial issues, so I guess a message should be faked. I'm still in two minds about this... on one hand, what you said; on the other hand, denials are logged to syslog anyway. One complicating factor is that a broadcast can be forbidden in two ways (both of which we exercise here): sending forbidden, or receiving (by a particular recipient) forbidden. In the latter case it might still be received by other potential recipients ... so do we still want to capture the fake reply? So actually, three options: no fake reply; fake reply only if sending is forbidden; fake reply if sending is forbidden or if receiving is forbidden.
(In reply to Philip Withnall from comment #31) > > +dbus_monitor_SOURCES= \ > > Niggle: Missing a space before the ‘=’. Sure, will fix that. > Could we factor out the migration to tool-common into a separate patch? > Would make things a little clearer. It's a bit annoying to disentangle, but OK. > > + char **filters) > > Shouldn’t that be (const char * const *)? I'll try it, but I don't think you can assign a char ** to a (const char * const *) without casting... so I can either make the parameter const char **, or cast it.
(In reply to Philip Withnall from comment #32) > What about the --bustle option? Deliberately undocumented. It produces the same pcap file that Bustle would, but that pcap file is actually wrong (LINKTYPE_NULL refers to a specific framing for captured network packets, used on BSD). I only added --bustle because I tried reading the Bustle source code to find out whether it ignores the link type in practice, but couldn't find it among all the monads :-) I spoke to wjt at FOSDEM and he thinks it does ignore the link type, so perhaps we don't need this hack - I'll try it and see whether Bustle can understand the pcap dump with the correct header. Wireshark certainly can. > Is that something which needs fixing in Bustle? In principle yes. wjt used LINKTYPE_NULL because LINKTYPE_DBUS hadn't been added to libpcap yet at the time. > @@ +604,5 @@ > > + 0, /* capture in GMT */ > > + 0, /* no opinion on timestamp precision */ > > + (1 << 27), /* D-Bus spec says so */ > > + LINKTYPE_DBUS > > + }; > > Can we have some documentation for this header format, such as a link to the > pcap docs? Sure. I'm deliberately not using libpcap, because it isn't worth it - as you can see, the format is really simple.
(In reply to Simon McVittie from comment #35) > > Could we include some details of the message in this log message? Would make > > fixing the monitor easier. > > Maybe. I'll look into the complain_about_message() function we use for > <deny/> rules and see whether I can factor something out. (I assume this is > not a merge-blocker though.) No, it's not a merge-blocker. It's a nice-to-have. > > > + tool. Only a user who is privileged on this > > > + bus (by some implementation-specific definition) may create > > > + monitor connections. > > > > Is it worthwhile clarifying this with a (non-normative) example of the > > definition (e.g. being root)? > > Probably, yes. "For instance, in the reference implementation of a message > bus, the user ID running the dbus-daemon and the Unix root user are > considered to be privileged" in a footnote, perhaps. ++ > > It might make sense to add a timeout to this loop so the test can’t continue > > forever if there’s a bug. > > I might try to steal the timeout machinery from telepathy-glib, which is > pretty reliable (it uses the GLib main loop, with alarm() as an extra safety > net on Unix). In telepathy-glib I never bothered with a timeout on > individual loops because that would be rather verbose, I just imposed a > (generous) timeout on the test as a whole. > > This is complicated by the fact that dbus may or may not in fact be using > the GLib main loop here (it's complicated)... I might just put in the > alarm() behind an #ifdef, with the caveat "if you're on Windows, the test > might signal failure by hanging forever". Reasonable? If we get a chance, it would be cool to upstream the timeout machinery from telepathy-glib to GLib itself, since it's pretty general purpose, and people keep reinventing it. But yes, any option is fine by me, as long as we don't have unit tests spinning forever on build bots. I'm assuming we're never realistically going to have a Windows build bot for D-Bus. > > @@ +769,5 @@ > > > + "BroadcastSignal1", "", NULL); > > > + dbus_message_unref (m); > > > + > > > + /* FIXME: dbus-daemon does not fake a denial message for the monitor's > > > + * benefit; should it? */ > > > > Not sure this is addressed in a later patch. Should it be? > > > > I can imagine the monitor being used to diagnose exactly these kinds of > > denial issues, so I guess a message should be faked. > > I'm still in two minds about this... on one hand, what you said; on the > other hand, denials are logged to syslog anyway. > > One complicating factor is that a broadcast can be forbidden in two ways > (both of which we exercise here): sending forbidden, or receiving (by a > particular recipient) forbidden. In the latter case it might still be > received by other potential recipients ... so do we still want to capture > the fake reply? > > So actually, three options: no fake reply; fake reply only if sending is > forbidden; fake reply if sending is forbidden or if receiving is forbidden. I would go for a fake reply if sending or receiving are forbidden, going by the principle that more logging is always better than less logging. As long as the reply format can be distinguished by dbus-monitor if necessary, it should be fine, and would help with diagnosing the issues where "every other listener gets the signal; why don't I?". (In reply to Simon McVittie from comment #36) > > Could we factor out the migration to tool-common into a separate patch? > > Would make things a little clearer. > > It's a bit annoying to disentangle, but OK. If it's a real pain, don't bother. > > > + char **filters) > > > > Shouldn’t that be (const char * const *)? > > I'll try it, but I don't think you can assign a char ** to a (const char * > const *) without casting... so I can either make the parameter const char > **, or cast it. I think (const char * const *) is technically correct here, so I would go with the cast. (In reply to Simon McVittie from comment #37) > (In reply to Philip Withnall from comment #32) > > What about the --bustle option? > > Deliberately undocumented. It produces the same pcap file that Bustle would, > but that pcap file is actually wrong (LINKTYPE_NULL refers to a specific > framing for captured network packets, used on BSD). OK. Could that be added in a comment in the code if we keep the --bustle option? > > Is that something which needs fixing in Bustle? > > In principle yes. wjt used LINKTYPE_NULL because LINKTYPE_DBUS hadn't been > added to libpcap yet at the time. OK. Did you speak to wjt about moving to LINKTYPE_DBUS? > > @@ +604,5 @@ > > > + 0, /* capture in GMT */ > > > + 0, /* no opinion on timestamp precision */ > > > + (1 << 27), /* D-Bus spec says so */ > > > + LINKTYPE_DBUS > > > + }; > > > > Can we have some documentation for this header format, such as a link to the > > pcap docs? > > Sure. I'm deliberately not using libpcap, because it isn't worth it - as you > can see, the format is really simple. Totally agreed, but a link to the protocol specification would be great.
(In reply to Simon McVittie from comment #35) > (In reply to Philip Withnall from comment #28) > > If we hit this failure case, the connection will end up in an intermediate > > state where it’s not a monitor, but has lost half its services. I don’t > > suppose there’s any way to avoid this? > > I don't think so, but it can only happen on OOM so I'm not really very > concerned. Actually, the names magically come back when the transaction is cancelled. (Srsly. The lengths dbus-daemon will go to in order to handle a situation that never actually happens...) > > Could we include some details of the message in this log message? Would make > > fixing the monitor easier. > > Maybe. I'll look into the complain_about_message() function we use for > <deny/> rules and see whether I can factor something out. Done. I copied similar logic rather than factoring out, because the available information is different. Maybe one day I will make nonnull() into API rather than putting the same pseudo-macro in more places, but that would require me to think of a good name for it, and increasing the length of its name would make it more annoying to use... I wish glibc's handling of printf ("%s", NULL) was portable. > > Worth documenting somewhere that monitoring adds the side-effect of failing > > the connection on OOM in the monitoring code. Pretty obvious, but we should > > be explicit that the monitoring code is not side-effect-free. I think you > > have stated this before, though. > > I think I stated it somewhere but possibly not in the code. I'll check. Oh, in fact I misunderstood what you were saying, and no I had not said that. To clarify, OOM in the monitoring code is the same as OOM anywhere else: it rolls back the transaction and does its best to report an error (which will itself probably fail with an out-of-memory error because what else would you expect in such a precarious situation? but at least we tried). I've added some text to the spec which I think resolves this. > > It might make sense to add a timeout to this loop so the test can’t continue > > forever if there’s a bug. Not yet addressed, should probably be a separate patch anyway > > > + /* FIXME: dbus-daemon does not fake a denial message for the monitor's > > > + * benefit; should it? */ > > > > Not sure this is addressed in a later patch. Should it be? > > > > I can imagine the monitor being used to diagnose exactly these kinds of > > denial issues, so I guess a message should be faked. Not yet addressed, should probably be a separate patch anyway
Created attachment 113067 [details] [review] 1/7] Add support for morphing a D-Bus connection into a "monitor" This is a special connection that is not allowed to send anything, and loses all its well-known names. In future commits, it will get a new set of match rules and the ability to eavesdrop on messages before the rest of the bus daemon has had a chance to process them.
Created attachment 113068 [details] [review] 2/7] Capture all messages received or sent, and send them to monitors Unlike eavesdropping, the point of capture is when the message is received, except for messages originating inside the dbus-daemon.
Created attachment 113069 [details] [review] 3/7] Add a regression test for becoming a monitor
Created attachment 113070 [details] [review] 4/7] dbus-monitor: use common code from dbus-test-tool --- I'd forgotten why this was awkward to disentangle. It wasn't because of conflicts... it was because the part of the patch being reverted to disentangle it fixed some trailing whitespace, so temporarily reverting that part fell foul of dbus' commit hooks :-(
Created attachment 113071 [details] [review] 5/7] dbus-monitor: add support for using BecomeMonitor to be a read-only monitor Move the dbus_connection_add_filter() call further up as a precaution, because it isn't safe for a monitor to not have a filter that swallows all messages.
Created attachment 113072 [details] [review] 6/7] dbus-monitor: add options to log binary data with or without pcap framing I removed --bustle because it turns out Bustle does understand the correct header fine.
(In reply to Simon McVittie from comment #36) > (In reply to Philip Withnall from comment #31) > > > +dbus_monitor_SOURCES= \ > > > > Niggle: Missing a space before the ‘=’. > > Sure, will fix that. I forgot this bit. Also still to do, in addition to the things previously mentioned (test timeouts; logging fake error "replies" for rejected broadcasts; comparing with kdbus). Also, looking at this again, I'm not sure that I'm actually doing the documented "forget all normal match rules" bit, so a monitor might be getting messages for its own match rules (although in practice it would be wrong if it had any) via eavesdropping *as well as* via monitoring.
Comment on attachment 113068 [details] [review] 2/7] Capture all messages received or sent, and send them to monitors Review of attachment 113068 [details] [review]: ----------------------------------------------------------------- ::: doc/dbus-specification.xml @@ +6313,5 @@ > + </para> > + > + <para> > + Message bus implementations should attempt to minimize the > + side-effects of monitoring - in particular, unlike ordinary The additions here look good to me. Niggle: Could we use an em-dash (‘—’) rather than a hyphen (‘ - ’)?
Comment on attachment 113070 [details] [review] 4/7] dbus-monitor: use common code from dbus-test-tool Review of attachment 113070 [details] [review]: ----------------------------------------------------------------- ++
(In reply to Simon McVittie from comment #46) > (In reply to Simon McVittie from comment #36) > > (In reply to Philip Withnall from comment #31) > > > > +dbus_monitor_SOURCES= \ > > > > > > Niggle: Missing a space before the ‘=’. > > > > Sure, will fix that. > > I forgot this bit. Also still to do, in addition to the things previously > mentioned (test timeouts; logging fake error "replies" for rejected > broadcasts; comparing with kdbus). > > Also, looking at this again, I'm not sure that I'm actually doing the > documented "forget all normal match rules" bit, so a monitor might be > getting messages for its own match rules (although in practice it would be > wrong if it had any) via eavesdropping *as well as* via monitoring. The updated patches all look good to me apart from the issues still listed in comment #46 and a niggle in comment #47.
Created attachment 113113 [details] [review] [fix for 2/7] bus_transaction_capture_error_reply: we must set the sender dbus-daemon refuses to send a message with no sender, even to eavesdroppers. The one case where this function is used is untested and possibly untestable, but I'm about to use it for fake replies to broadcasts as well. --- Attaching corrections as separate patches because that's probably rather easier to review at this point, but I'll squash them in when ready.
Created attachment 113114 [details] [review] [fix for 4/7] dbus-monitor: less horrible whitespace in Makefile.am
Created attachment 113115 [details] [review] [fix for 1/7] Drop all prior match rules when becoming a monitor
Created attachment 113116 [details] [review] [fix for 3/7] monitor regression test: verify that we lose previous match rules
Created attachment 113117 [details] [review] [8] Add a common test_init() for GLib tests which prevents hanging forever
Created attachment 113118 [details] [review] [fix for 2/7] Capture a fake reply if a broadcast cannot be delivered
Created attachment 113119 [details] [review] [fix for 4/7] monitor test: check that we log a fake reply to forbidden broadcasts
Created attachment 113120 [details] [review] [fix for 2/7] use em dash where typographically appropriate I've used space, em dash, space (slightly non-standard, but apparently correct according to some style guides) to avoid the resulting markup looking weird in monospaced fonts. --- I hope you realise most people don't care about Unicode punctuation as much as you do :-P
Comment on attachment 113113 [details] [review] [fix for 2/7] bus_transaction_capture_error_reply: we must set the sender Review of attachment 113113 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 113114 [details] [review] [fix for 4/7] dbus-monitor: less horrible whitespace in Makefile.am Review of attachment 113114 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 113115 [details] [review] [fix for 1/7] Drop all prior match rules when becoming a monitor Review of attachment 113115 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 113116 [details] [review] [fix for 3/7] monitor regression test: verify that we lose previous match rules Review of attachment 113116 [details] [review]: ----------------------------------------------------------------- This looks like a duplicate of attachment #113115 [details] [review]?
Comment on attachment 113117 [details] [review] [8] Add a common test_init() for GLib tests which prevents hanging forever Review of attachment 113117 [details] [review]: ----------------------------------------------------------------- ++
(In reply to Philip Withnall from comment #61) > This looks like a duplicate of attachment #113115 [details] [review]? Sorry, yes, wrong file.
Created attachment 113121 [details] [review] [fix for 3/7] monitor regression test: verify that we lose previous match rules
Comment on attachment 113118 [details] [review] [fix for 2/7] Capture a fake reply if a broadcast cannot be delivered Review of attachment 113118 [details] [review]: ----------------------------------------------------------------- Do we also need changes in bus_transaction_send_from_driver()?
Comment on attachment 113119 [details] [review] [fix for 4/7] monitor test: check that we log a fake reply to forbidden broadcasts Review of attachment 113119 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 113120 [details] [review] [fix for 2/7] use em dash where typographically appropriate Review of attachment 113120 [details] [review]: ----------------------------------------------------------------- Splinter and Bugzilla are corrupting this for me, but I assume it’s correct in git.
Comment on attachment 113121 [details] [review] [fix for 3/7] monitor regression test: verify that we lose previous match rules Review of attachment 113121 [details] [review]: ----------------------------------------------------------------- I’m not entirely sure how this one verifies previous match rules are lost, since this test has never received the Tedious signal, so I’m not sure how we can differentiate from not receiving it twice.
(In reply to Philip Withnall from comment #68) > I’m not entirely sure how this one verifies previous match rules are lost, > since this test has never received the Tedious signal, so I’m not sure how > we can differentiate from not receiving it twice. The comment is a bit misleading. The reasoning for wanting match rules to go away is that we don't want to see messages twice, but this is exercised in the test by not receiving an undesired message at all. The other process in this test emits Interesting, Tedious, Fun, and we assert that we receive exactly Interesting and Fun (in that order). The monitor's callback also asserts that it never receives Tedious, for completeness. In the original test case, we added monitor match rules which should have matched only Interesting and Fun, so if we received Tedious, that would be an indication that the monitor match rule processing is faulty (in fact I think I verified that the test failed, then added match rule processing and verified that it passed). After this commit, there are two reasons we might see Tedious: either because our old non-monitor match rule is still there, or because our new monitor match rules are processed incorrectly. I did verify that the test failed (for the former reason) before adding the code to drop old non-monitor match rules. Perhaps it would be more clearly correct if the test added match rules for both Interesting and Tedious, so that if we received Interesting twice (before Fun), that would *also* be a sign that something was wrong?
(In reply to Philip Withnall from comment #65) > Do we also need changes in bus_transaction_send_from_driver()? bus_transaction_send_from_driver() already captures the error reply - it was the original caller of bus_transaction_capture_error_reply(), in fact. Looking at it again, I notice that the behaviour of the function has (incorrectly) changed, and it would now fail the transaction if the security policy forbids sending the reply, not just if it hits OOM. I'll fix that. Is that the change that you had in mind, or was there something else?
Comment on attachment 113070 [details] [review] 4/7] dbus-monitor: use common code from dbus-test-tool applied this one, with the whitespace adjustment, and an unreviewed change to sync up the CMake equivalent
Created attachment 113161 [details] [review] [fix for 2/7] bus_transaction_send_from_driver: preserve existing return My previous attempt at this impossible situation (I can't think how you could set it up without breaking the client by failing to send its Hello reply, hence it is untested) would have falsely signalled OOM if the client was forbidden from receiving dbus-daemon's reply to a method call. In principle we could return FALSE if we hit OOM while capturing the message, but there's no real need to do so, and the other callers of bus_transaction_capture_error_reply() don't. I'm sure you have noticed this is all rather theoretical.
Created attachment 113162 [details] [review] [fix for 3/7] monitor regression test: verify that we lose previous match rules --- I hope this is clearer?
Created attachment 113163 [details] [review] [fix for 5/7] Fix cmake build for dbus-monitor Now that it uses dbus-internals macros, it needs -DDBUS_COMPILATION.
Created attachment 113164 [details] [review] [fix for 8] Fix cmake build for dbus-testutils-glib with timeout It now uses functionality from dbus-testutils.
Comment on attachment 113162 [details] [review] [fix for 3/7] monitor regression test: verify that we lose previous match rules Review of attachment 113162 [details] [review]: ----------------------------------------------------------------- Much clearer. ++
Comment on attachment 113163 [details] [review] [fix for 5/7] Fix cmake build for dbus-monitor Review of attachment 113163 [details] [review]: ----------------------------------------------------------------- I’m no CMake person, but this doesn’t look actively bad.
Comment on attachment 113164 [details] [review] [fix for 8] Fix cmake build for dbus-testutils-glib with timeout Review of attachment 113164 [details] [review]: ----------------------------------------------------------------- I’m no CMake person, but this doesn’t look actively bad.
Comment on attachment 113161 [details] [review] [fix for 2/7] bus_transaction_send_from_driver: preserve existing return Review of attachment 113161 [details] [review]: ----------------------------------------------------------------- Ah, good catch. ++ I was actually referring to adding a bus_transaction_capture_error_reply() call to bus_transaction_send_from_driver()…which had already been added in the original patch. Embarrassingly, It appears I can’t juggle non-squashed patches in my head. Sorry.
This all landed in dbus 1.9.10.
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.