Bug 46787 - add a way to eavesdrop on the system bus without undesired side-effects
Summary: add a way to eavesdrop on the system bus without undesired side-effects
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: All All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
: 45914 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-02-29 11:17 UTC by Simon McVittie
Modified: 2015-02-12 19:41 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[01/10] socket_set_epoll_add: initialize all bytes of struct epoll_event (730 bytes, patch)
2015-01-23 19:42 UTC, Simon McVittie
Details | Splinter Review
[02/10] bus: exit on fatal errors even if not syslogging (695 bytes, patch)
2015-01-23 19:43 UTC, Simon McVittie
Details | Splinter Review
[03/10] bus driver: factor out bus_driver_check_caller_is_privileged (5.78 KB, patch)
2015-01-23 19:45 UTC, Simon McVittie
Details | Splinter Review
[04/10] Add support for morphing a D-Bus connection into a "monitor" (9.16 KB, patch)
2015-01-23 19:45 UTC, Simon McVittie
Details | Splinter Review
[05/10] Factor out some utility functions from test/dbus-daemon* (19.59 KB, patch)
2015-01-23 19:45 UTC, Simon McVittie
Details | Splinter Review
[06/10] Capture all messages received or sent, and send them to monitors (20.71 KB, patch)
2015-01-23 19:46 UTC, Simon McVittie
Details | Splinter Review
[07/10] Add a regression test for becoming a monitor (54.42 KB, patch)
2015-01-23 19:46 UTC, Simon McVittie
Details | Splinter Review
[08/10] dbus-monitor: add support for using BecomeMonitor to be a read-only monitor (5.20 KB, patch)
2015-01-23 19:46 UTC, Simon McVittie
Details | Splinter Review
[09/10] dbus-monitor: add options to log binary data with or without pcap framing (9.88 KB, patch)
2015-01-23 19:47 UTC, Simon McVittie
Details | Splinter Review
[10/10] lcov: use builddir, not srcdir (983 bytes, patch)
2015-01-23 19:48 UTC, Simon McVittie
Details | Splinter Review
[PATCH 1/6] Add support for morphing a D-Bus connection into a "monitor" (9.16 KB, patch)
2015-01-26 21:20 UTC, Simon McVittie
Details | Splinter Review
[PATCH 2/6] Capture all messages received or sent, and send them to monitors (20.71 KB, patch)
2015-01-26 21:20 UTC, Simon McVittie
Details | Splinter Review
[PATCH 3/6] Add a regression test for becoming a monitor (49.57 KB, patch)
2015-01-26 21:21 UTC, Simon McVittie
Details | Splinter Review
[PATCH 4/6] dbus-monitor: add support for using BecomeMonitor to be a read-only monitor (5.20 KB, patch)
2015-01-26 21:21 UTC, Simon McVittie
Details | Splinter Review
[PATCH 5/6] dbus-monitor: add options to log binary data with or without pcap framing (9.87 KB, patch)
2015-01-26 21:22 UTC, Simon McVittie
Details | Splinter Review
[7/7] Allow root to monitor the system bus by default (970 bytes, patch)
2015-01-26 21:22 UTC, Simon McVittie
Details | Splinter Review
1/7] Add support for morphing a D-Bus connection into a "monitor" (10.59 KB, patch)
2015-02-02 20:39 UTC, Simon McVittie
Details | Splinter Review
2/7] Capture all messages received or sent, and send them to monitors (22.13 KB, patch)
2015-02-02 20:40 UTC, Simon McVittie
Details | Splinter Review
3/7] Add a regression test for becoming a monitor (49.24 KB, patch)
2015-02-02 20:41 UTC, Simon McVittie
Details | Splinter Review
4/7] dbus-monitor: use common code from dbus-test-tool (2.02 KB, patch)
2015-02-02 20:42 UTC, Simon McVittie
Details | Splinter Review
5/7] dbus-monitor: add support for using BecomeMonitor to be a read-only monitor (3.76 KB, patch)
2015-02-02 20:42 UTC, Simon McVittie
Details | Splinter Review
6/7] dbus-monitor: add options to log binary data with or without pcap framing (10.20 KB, patch)
2015-02-02 20:43 UTC, Simon McVittie
Details | Splinter Review
[fix for 2/7] bus_transaction_capture_error_reply: we must set the sender (1.25 KB, patch)
2015-02-03 15:14 UTC, Simon McVittie
Details | Splinter Review
[fix for 4/7] dbus-monitor: less horrible whitespace in Makefile.am (661 bytes, patch)
2015-02-03 15:14 UTC, Simon McVittie
Details | Splinter Review
[fix for 1/7] Drop all prior match rules when becoming a monitor (1.16 KB, patch)
2015-02-03 15:14 UTC, Simon McVittie
Details | Splinter Review
[fix for 3/7] monitor regression test: verify that we lose previous match rules (1.16 KB, patch)
2015-02-03 15:15 UTC, Simon McVittie
Details | Splinter Review
[8] Add a common test_init() for GLib tests which prevents hanging forever (9.25 KB, patch)
2015-02-03 15:15 UTC, Simon McVittie
Details | Splinter Review
[fix for 2/7] Capture a fake reply if a broadcast cannot be delivered (4.09 KB, patch)
2015-02-03 15:16 UTC, Simon McVittie
Details | Splinter Review
[fix for 4/7] monitor test: check that we log a fake reply to forbidden broadcasts (2.50 KB, patch)
2015-02-03 15:16 UTC, Simon McVittie
Details | Splinter Review
[fix for 2/7] use em dash where typographically appropriate (1.10 KB, patch)
2015-02-03 15:17 UTC, Simon McVittie
Details | Splinter Review
[fix for 3/7] monitor regression test: verify that we lose previous match rules (873 bytes, patch)
2015-02-03 16:05 UTC, Simon McVittie
Details | Splinter Review
[fix for 2/7] bus_transaction_send_from_driver: preserve existing return (1.46 KB, patch)
2015-02-04 15:38 UTC, Simon McVittie
Details | Splinter Review
[fix for 3/7] monitor regression test: verify that we lose previous match rules (1.07 KB, patch)
2015-02-04 15:39 UTC, Simon McVittie
Details | Splinter Review
[fix for 5/7] Fix cmake build for dbus-monitor (694 bytes, patch)
2015-02-04 15:40 UTC, Simon McVittie
Details | Splinter Review
[fix for 8] Fix cmake build for dbus-testutils-glib with timeout (925 bytes, patch)
2015-02-04 15:40 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2012-02-29 11:17:03 UTC
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.
Comment 1 Simon McVittie 2012-02-29 11:28:29 UTC
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
Comment 2 Simon McVittie 2012-02-29 11:29:43 UTC
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".
Comment 3 Simon McVittie 2012-02-29 11:51:28 UTC
(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.
Comment 4 Colin Walters 2012-03-06 09:27:35 UTC
I'd still prefer the control channel approach.
Comment 5 Simon McVittie 2012-03-26 06:21:15 UTC
(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...
Comment 6 Simon McVittie 2013-09-26 15:01:53 UTC
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.
Comment 7 Colin Walters 2013-10-10 16:26:08 UTC
(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.
Comment 8 Simon McVittie 2015-01-22 17:20:17 UTC
*** Bug 45914 has been marked as a duplicate of this bug. ***
Comment 9 Simon McVittie 2015-01-22 18:04:35 UTC
(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?
Comment 10 Simon McVittie 2015-01-22 20:40:47 UTC
(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.
Comment 11 Simon McVittie 2015-01-23 19:42:30 UTC
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.
Comment 12 Simon McVittie 2015-01-23 19:43:49 UTC
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.
Comment 13 Simon McVittie 2015-01-23 19:45:09 UTC
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.
Comment 14 Simon McVittie 2015-01-23 19:45:29 UTC
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.
Comment 15 Simon McVittie 2015-01-23 19:45:44 UTC
Created attachment 112744 [details] [review]
[05/10] Factor out some utility functions from  test/dbus-daemon*
Comment 16 Simon McVittie 2015-01-23 19:46:03 UTC
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.
Comment 17 Simon McVittie 2015-01-23 19:46:28 UTC
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.
Comment 18 Simon McVittie 2015-01-23 19:46:47 UTC
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.
Comment 19 Simon McVittie 2015-01-23 19:47:49 UTC
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 :-)
Comment 20 Simon McVittie 2015-01-23 19:48:11 UTC
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.
Comment 21 Simon McVittie 2015-01-23 19:49:12 UTC
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
Comment 22 Simon McVittie 2015-01-26 21:20:12 UTC
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.
Comment 23 Simon McVittie 2015-01-26 21:20:41 UTC
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.
Comment 24 Simon McVittie 2015-01-26 21:21:27 UTC
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.
Comment 25 Simon McVittie 2015-01-26 21:21:43 UTC
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.
Comment 26 Simon McVittie 2015-01-26 21:22:02 UTC
Created attachment 112860 [details] [review]
[PATCH 5/6] dbus-monitor: add options to log binary data with or  without pcap framing
Comment 27 Simon McVittie 2015-01-26 21:22:17 UTC
Created attachment 112861 [details] [review]
[7/7] Allow root to monitor the system bus by default

---

New.
Comment 28 Philip Withnall 2015-02-02 10:48:28 UTC
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 29 Philip Withnall 2015-02-02 11:10:58 UTC
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 30 Philip Withnall 2015-02-02 11:35:48 UTC
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 31 Philip Withnall 2015-02-02 11:39:26 UTC
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 32 Philip Withnall 2015-02-02 11:47:48 UTC
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 33 Philip Withnall 2015-02-02 11:48:21 UTC
Comment on attachment 112861 [details] [review]
[7/7] Allow root to monitor the system bus by default

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

++
Comment 34 Simon McVittie 2015-02-02 11:48:53 UTC
(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.
Comment 35 Simon McVittie 2015-02-02 12:23:44 UTC
(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.
Comment 36 Simon McVittie 2015-02-02 12:27:54 UTC
(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.
Comment 37 Simon McVittie 2015-02-02 12:34:06 UTC
(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.
Comment 38 Philip Withnall 2015-02-02 13:42:09 UTC
(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.
Comment 39 Simon McVittie 2015-02-02 20:36:42 UTC
(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
Comment 40 Simon McVittie 2015-02-02 20:39:53 UTC
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.
Comment 41 Simon McVittie 2015-02-02 20:40:28 UTC
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.
Comment 42 Simon McVittie 2015-02-02 20:41:05 UTC
Created attachment 113069 [details] [review]
3/7] Add a regression test for becoming a monitor
Comment 43 Simon McVittie 2015-02-02 20:42:17 UTC
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 :-(
Comment 44 Simon McVittie 2015-02-02 20:42:35 UTC
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.
Comment 45 Simon McVittie 2015-02-02 20:43:31 UTC
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.
Comment 46 Simon McVittie 2015-02-02 20:46:45 UTC
(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 47 Philip Withnall 2015-02-03 08:28:34 UTC
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 48 Philip Withnall 2015-02-03 08:30:20 UTC
Comment on attachment 113070 [details] [review]
4/7] dbus-monitor: use common code from dbus-test-tool

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

++
Comment 49 Philip Withnall 2015-02-03 08:34:34 UTC
(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.
Comment 50 Simon McVittie 2015-02-03 15:14:05 UTC
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.
Comment 51 Simon McVittie 2015-02-03 15:14:26 UTC
Created attachment 113114 [details] [review]
[fix for 4/7] dbus-monitor: less horrible whitespace in Makefile.am
Comment 52 Simon McVittie 2015-02-03 15:14:49 UTC
Created attachment 113115 [details] [review]
[fix for 1/7] Drop all prior match rules when becoming a monitor
Comment 53 Simon McVittie 2015-02-03 15:15:16 UTC
Created attachment 113116 [details] [review]
[fix for 3/7] monitor regression test: verify that we lose previous  match rules
Comment 54 Simon McVittie 2015-02-03 15:15:39 UTC
Created attachment 113117 [details] [review]
[8] Add a common test_init() for GLib tests which prevents  hanging forever
Comment 55 Simon McVittie 2015-02-03 15:16:01 UTC
Created attachment 113118 [details] [review]
[fix for 2/7] Capture a fake reply if a broadcast cannot be delivered
Comment 56 Simon McVittie 2015-02-03 15:16:18 UTC
Created attachment 113119 [details] [review]
[fix for 4/7] monitor test: check that we log a fake reply to forbidden  broadcasts
Comment 57 Simon McVittie 2015-02-03 15:17:17 UTC
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 58 Philip Withnall 2015-02-03 15:47:55 UTC
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 59 Philip Withnall 2015-02-03 15:48:08 UTC
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 60 Philip Withnall 2015-02-03 15:52:27 UTC
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 61 Philip Withnall 2015-02-03 15:53:42 UTC
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 62 Philip Withnall 2015-02-03 16:00:57 UTC
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]:
-----------------------------------------------------------------

++
Comment 63 Simon McVittie 2015-02-03 16:04:33 UTC
(In reply to Philip Withnall from comment #61)
> This looks like a duplicate of attachment #113115 [details] [review]?

Sorry, yes, wrong file.
Comment 64 Simon McVittie 2015-02-03 16:05:14 UTC
Created attachment 113121 [details] [review]
[fix for 3/7] monitor regression test: verify that we lose previous  match rules
Comment 65 Philip Withnall 2015-02-03 16:11:55 UTC
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 66 Philip Withnall 2015-02-03 16:13:10 UTC
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 67 Philip Withnall 2015-02-03 16:15:23 UTC
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 68 Philip Withnall 2015-02-03 16:20:15 UTC
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.
Comment 69 Simon McVittie 2015-02-04 14:12:41 UTC
(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?
Comment 70 Simon McVittie 2015-02-04 14:17:13 UTC
(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 71 Simon McVittie 2015-02-04 14:19:30 UTC
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
Comment 72 Simon McVittie 2015-02-04 15:38:27 UTC
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.
Comment 73 Simon McVittie 2015-02-04 15:39:32 UTC
Created attachment 113162 [details] [review]
[fix for 3/7] monitor regression test: verify that we lose previous  match rules

---

I hope this is clearer?
Comment 74 Simon McVittie 2015-02-04 15:40:29 UTC
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.
Comment 75 Simon McVittie 2015-02-04 15:40:53 UTC
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 76 Philip Withnall 2015-02-04 17:00:31 UTC
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 77 Philip Withnall 2015-02-04 17:01:14 UTC
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 78 Philip Withnall 2015-02-04 17:01:16 UTC
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 79 Philip Withnall 2015-02-04 17:08:47 UTC
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.
Comment 80 Simon McVittie 2015-02-12 19:41:33 UTC
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.