Bug 92853

Summary: dbus-daemon policy language should be able to describe broadcasts
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: D-Bus Maintainers <dbus>
Severity: normal    
Priority: medium CC: bugzilla, dh.herrmann, smcv, teg
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
Whiteboard: review+
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 101848    
Attachments: Add send_broadcast as an attribute of <allow> and <deny> elements
Add a regression test for applying bus policy to broadcasts/unicasts
[1/3] config-parser: Clarify how <allow>, <deny> attributes work
[2/3] Add send_broadcast as an attribute of <allow> and <deny> elements
[3/3] Add a regression test for applying bus policy to broadcasts/unicasts
config-parser: Clarify how <allow>, <deny> attributes work
Add a test-case for combining receive_type and send_destination
dbus-daemon(1): Document the wildcard attribute value "*" more clearly
dbus-daemon(1): Actually document "own" rules
dbus-daemon(1): Clarify how user, group rules work
dbus-daemon(1): Be more truthful about the default policy
dbus-daemon(1): Document how send_* and receive_* work in general
Add send_broadcast as an attribute of <allow> and <deny> elements
Add a regression test for applying bus policy to broadcasts/unicasts
dbus-daemon(1): Document how send_* and receive_* work in general
config-parser: Fail on impossible send_broadcast/send_destination pair
test/data: Test impossible send_broadcast/send_destination pair

Description Simon McVittie 2015-11-07 10:48:17 UTC
The default system bus policy has:

  <policy context="default">
    <!-- All users can connect to system bus -->
    <allow user="*"/>

    <!-- Holes must be punched in service configuration files for
         name ownership and sending method calls -->
    <deny own="*"/>
    <deny send_type="method_call"/>

    <!-- Signals and reply messages (method returns, errors) are allowed
         by default -->
    <allow send_type="signal"/>
    <allow send_requested_reply="true" send_type="method_return"/>
    <allow send_requested_reply="true" send_type="error"/>

    <!-- All messages may be received by default -->
    <allow receive_type="method_call"/>
    <allow receive_type="method_return"/>
    <allow receive_type="error"/>
    <allow receive_type="signal"/>

This allows any process to send unicast signals to any other. This seems unlikely to be what was intended: it would seem better to allow any process to send broadcasts, but treat unicast signals like method calls.

Unfortunately, the D-Bus policy language cannot currently express "is a broadcast": you can match by a specific destination, or by "any or no destination" (the default).

Because the dbus-daemon does not support re-exec'ing itself and does not tolerate unknown attributes or elements, distributions that perform in-place upgrades cannot actually change the default until they no longer support upgrades from a version that does not have this feature. I would like to add syntax for it now, so that we can consider actually using it by default in a couple of years.
Comment 1 Simon McVittie 2015-11-07 10:51:24 UTC
Created attachment 119459 [details] [review]
Add send_broadcast as an attribute of <allow> and <deny>  elements
Comment 2 Simon McVittie 2015-11-07 10:53:09 UTC
Created attachment 119460 [details] [review]
Add a regression test for applying bus policy to   broadcasts/unicasts

This test-case is actually in the test for monitoring the bus,
because it's easier to see what's going on there - the error reply
to a rejected broadcast is not visible unless you are monitoring.
Comment 3 Philip Withnall 2015-11-20 15:34:42 UTC
Comment on attachment 119459 [details] [review]
Add send_broadcast as an attribute of <allow> and <deny>  elements

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

This should be documented in doc/dbus-daemon.1.xml.in.

::: bus/config-parser.c
@@ +1324,5 @@
> +      (send_broadcast && (receive_interface ||
> +                          receive_member ||
> +                          receive_error ||
> +                          receive_sender ||
> +                          receive_requested_reply ||

This allows (send_broadcast && receive_path) and (send_broadcast && receive_type) — is that intentional?

@@ +1327,5 @@
> +                          receive_sender ||
> +                          receive_requested_reply ||
> +                          own || own_prefix ||
> +                          user ||
> +                          group)) ||

The comment above this massive block needs updating.

@@ +1459,5 @@
> +          if (strcmp (send_broadcast, "true") == 0)
> +            rule->d.send.broadcast = BUS_POLICY_TRISTATE_TRUE;
> +          else
> +            rule->d.send.broadcast = BUS_POLICY_TRISTATE_FALSE;
> +        }

Might be clearer to explicitly set `rule->d.send.broadcast = BUS_POLICY_TRISTATE_ANY` in the `else` case.
Comment 4 Philip Withnall 2015-11-20 15:41:47 UTC
Comment on attachment 119460 [details] [review]
Add a regression test for applying bus policy to   broadcasts/unicasts

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

Looks OK to me, but I have not worked through every argument of every call in those test cases.
Comment 5 Simon McVittie 2015-11-20 17:43:45 UTC
(In reply to Philip Withnall from comment #3)
> This should be documented in doc/dbus-daemon.1.xml.in.

Sure. I thought I had, but perhaps it got lost somewhere.

> This allows (send_broadcast && receive_path) and (send_broadcast &&
> receive_type) — is that intentional?

No. I think this convinces me that this conditional is completely unusable, and should be split up:

dbus_bool_t any_send_attribute = send_destination || send_type || ...;
dbus_bool_t any_receive_attribute = receive_sender || receive_type || ...;

if ((any_send_attribute +
    any_receive_attribute +
    (own != NULL) +
    (own_prefix != NULL) + ...) > 1)
  error ("invalid combination of attributes");

> Might be clearer to explicitly set `rule->d.send.broadcast =
> BUS_POLICY_TRISTATE_ANY` in the `else` case.

Makes sense.
Comment 6 Simon McVittie 2017-05-31 15:03:26 UTC
Created attachment 131602 [details] [review]
[1/3] config-parser: Clarify how <allow>, <deny> attributes  work

The giant conditionals used to check policy attributes are increasingly
unwieldy, so let's try something else. Bundle together the send_
attributes, the receive_ attributes, the eavesdrop attribute
(which can go on either send or receive rules) and the other attributes
into equivalence classes, and write the conditionals in terms of those
equivalence classes.

In particular, this correctly forbids
    <allow receive_type="..." send_destination="..."/>
which was previously allowed but nonsensical (the send part took
precedence and the receive part was ignored).
Comment 7 Simon McVittie 2017-05-31 15:04:05 UTC
Created attachment 131603 [details] [review]
[2/3] Add send_broadcast as an attribute of <allow> and <deny>  elements

<allow send_broadcast="true" ...> only matches broadcasts,
which are signals with a NULL destination. There was previously
no way for the policy language to express "NULL destination",
only "any destination".

<allow send_broadcast="false" ...> only matches non-broadcasts,
which are non-signals or signals with a non-NULL destination.
There was previously no way for the policy language to express
"any non-NULL destination", only "any destination".

---

Rebased onto Attachment #131602 [details]
Comment 8 Simon McVittie 2017-05-31 15:05:07 UTC
Created attachment 131604 [details] [review]
[3/3] Add a regression test for applying bus policy to  broadcasts/unicasts

This test-case is actually in the test for monitoring the bus,
because it's easier to see what's going on there - the error reply
to a rejected broadcast is not visible unless you are monitoring.

---

Rebased onto current git master, resolving conflicts with Bug #92074
Comment 9 Simon McVittie 2017-05-31 15:06:22 UTC
Also available at https://github.com/smcv/dbus/commits/92853-send-broadcast
Comment 10 Philip Withnall 2017-06-12 11:54:32 UTC
Comment on attachment 131602 [details] [review]
[1/3] config-parser: Clarify how <allow>, <deny> attributes  work

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

This looks like it makes sense.

::: bus/config-parser.c
@@ +1408,5 @@
> +  if (any_message_attribute +
> +      ((own != NULL) +
> +       (own_prefix != NULL) +
> +       (user != NULL) +
> +       (group != NULL)) > 1)

Nitpick: can you really add these boolean values, given that TRUE is defined as any non-zero value, and hence any one of these comparisons might be >1?
Comment 11 Philip Withnall 2017-06-12 11:59:09 UTC
Comment on attachment 131602 [details] [review]
[1/3] config-parser: Clarify how <allow>, <deny> attributes  work

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

> In particular, this correctly forbids
>     <allow receive_type="..." send_destination="..."/>
> which was previously allowed but nonsensical (the send part took
> precedence and the receive part was ignored).

Actually, can we have a test for this in bus_config_parser_test() please?
Comment 12 Philip Withnall 2017-06-12 12:01:09 UTC
Comment on attachment 131603 [details] [review]
[2/3] Add send_broadcast as an attribute of <allow> and <deny>  elements

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

::: doc/dbus-daemon.1.xml.in
@@ +884,5 @@
>  
> +<para>
> +  Rules with send_broadcast="true" match signal messages with no destination
> +  (broadcasts), while rules with send_broadcast="false" match messages with
> +  any unicast destination (unicast signals, together with all method calls,

Could you clarify the send_broadcast="false" case to make it really really obvious that it does not match signals which have no destination set? Some might interpret ‘any unicast destination’ to mean ‘any destination including unset’.
Comment 13 Philip Withnall 2017-06-12 12:05:19 UTC
Comment on attachment 131604 [details] [review]
[3/3] Add a regression test for applying bus policy to  broadcasts/unicasts

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

++
Comment 14 Simon McVittie 2017-06-12 15:03:53 UTC
(In reply to Philip Withnall from comment #10)
> @@ +1408,5 @@
> > +  if (any_message_attribute +
> > +      ((own != NULL) +
> > +       (own_prefix != NULL) +
> > +       (user != NULL) +
> > +       (group != NULL)) > 1)
> 
> Nitpick: can you really add these boolean values, given that TRUE is defined
> as any non-zero value, and hence any one of these comparisons might be >1?

Yes, ISO C says this is OK.

In ISO C, boolean expressions have type int and value 0 or 1: "Each of the operators [==, !=] yields 1 if the specified relation is true and 0 if it is false" (ISO/IEC 9899:201x draft n1570, §6.5.9 (3)) and the same for relational operators.

TRUE is not Standard C. It's defined by libdbus, GLib and probably lots of other libraries, with value 1.

When you say "any non-zero value", you might be thinking of the fact that "if (b)", "while (b)", etc. consider any non-zero value, and not just 1, to be "truthy". (e.g. ISO/IEC 9899:201x draft n1570, §6.8.4.1 (2) for the "if" statement): "if (b)" is equivalent to "if ((b) != 0)" for any expression b.

I am not aware of any non-standard C compiler that does not follow the standard in this respect.
Comment 15 Philip Withnall 2017-06-12 15:15:32 UTC
(In reply to Simon McVittie from comment #14)
> (In reply to Philip Withnall from comment #10)
> > @@ +1408,5 @@
> > > +  if (any_message_attribute +
> > > +      ((own != NULL) +
> > > +       (own_prefix != NULL) +
> > > +       (user != NULL) +
> > > +       (group != NULL)) > 1)
> > 
> > Nitpick: can you really add these boolean values, given that TRUE is defined
> > as any non-zero value, and hence any one of these comparisons might be >1?
>
> When you say "any non-zero value", you might be thinking of the fact that
> "if (b)", "while (b)", etc. consider any non-zero value, and not just 1, to
> be "truthy". (e.g. ISO/IEC 9899:201x draft n1570, §6.8.4.1 (2) for the "if"
> statement): "if (b)" is equivalent to "if ((b) != 0)" for any expression b.

I was. I hate that I can’t remember all the inconsistencies in C.
Comment 16 Tom Gundersen 2017-06-19 16:39:06 UTC
Would another approach to this be to treat unicast signals more like method returns, than method calls? Arguably this is already how broadcasted signals work: you add a match and get signals in return. In other words, you only ever get things you asked for.

So compared to this approach, rather than disallowing directed signals altogether (and having to selectively allow the ones you want in policy), how about adding a new rule that would allow you to only accept requested signals, where a requested signal is one you explicitly subscribed to.

This was, if my memory serves me correctly, the approach kdbus tried to take (where legacy clients were always subscribed to all directed signals for the sake of compatibility).
Comment 17 Simon McVittie 2017-07-18 10:35:12 UTC
(In reply to Tom Gundersen from comment #16)
> Would another approach to this be to treat unicast signals more like method
> returns, than method calls? Arguably this is already how broadcasted signals
> work: you add a match and get signals in return. In other words, you only
> ever get things you asked for.

I'm not sure whether we ever guaranteed that you *only* get the messages you asked for. If we had, kdbus' use of bloom filters would have been incorrect (because AIUI the guarantee for bloom filters is to match/deliver all messages that should match, together with a small but nonzero number of messages that should not have matched), and the change proposed in Bug #66114 would certainly be invalid.

If we had a trivial message bus reimplementation that sent every broadcast to every peer (including the sender) and ignored match rules, obviously it would be woefully inefficient, but would it be non-spec-compliant? I would be very tempted to say it would be valid.

Relatedly, at the moment the match-rule-matching engine is orthogonal to anything with non-trivial security implications (unicast messages bypass the matching engine, and broadcasts are assumed to be public information) and I'm not entirely positive about the idea of saying that bugs in match-rule-matching are now to be considered security vulnerabilities.
Comment 18 Tom Gundersen 2017-07-18 23:34:36 UTC
(In reply to Simon McVittie from comment #17)
> (In reply to Tom Gundersen from comment #16)
> > Would another approach to this be to treat unicast signals more like method
> > returns, than method calls? Arguably this is already how broadcasted signals
> > work: you add a match and get signals in return. In other words, you only
> > ever get things you asked for.
> 
> I'm not sure whether we ever guaranteed that you *only* get the messages you
> asked for.

Fair point. Though this is what the reference implementation does, and from experience clients can get pretty confused if they receive messages they did not expect.

> If we had, kdbus' use of bloom filters would have been incorrect
> (because AIUI the guarantee for bloom filters is to match/deliver all
> messages that should match, together with a small but nonzero number of
> messages that should not have matched),

True, but when using a legacy D-Bus client over kdbus these guarantees were given again by filtering out messages in a userspace proxy (if my memory serves me right at least).

> and the change proposed in Bug
> #66114 would certainly be invalid.

Agreed.

> If we had a trivial message bus reimplementation that sent every broadcast
> to every peer (including the sender) and ignored match rules, obviously it
> would be woefully inefficient, but would it be non-spec-compliant? I would
> be very tempted to say it would be valid.

Having recently tried to write such a trivial reimplementation, I can report that this does not work in practice (clients (or their libraries) do not handle unexpected messages well).

> Relatedly, at the moment the match-rule-matching engine is orthogonal to
> anything with non-trivial security implications (unicast messages bypass the
> matching engine, and broadcasts are assumed to be public information) and
> I'm not entirely positive about the idea of saying that bugs in
> match-rule-matching are now to be considered security vulnerabilities.

That is a good point. Though my counter to this would be that it seems less error-prone to restrict the hard parts of getting the security sensitive stuff right to the dbus-daemon, rather than pushing it on various packages/distros to get individual policy files right (which they'd need to in order to open up for the right directed signals), which has proven to be notoriously difficult in the past.
Comment 19 Simon McVittie 2017-07-19 13:52:37 UTC
(In reply to Tom Gundersen from comment #18)
> > If we had a trivial message bus reimplementation that sent every broadcast
> > to every peer (including the sender) and ignored match rules, obviously it
> > would be woefully inefficient, but would it be non-spec-compliant? I would
> > be very tempted to say it would be valid.
> 
> Having recently tried to write such a trivial reimplementation, I can report
> that this does not work in practice (clients (or their libraries) do not
> handle unexpected messages well).

Huh. Do you mean broadcasts or unicasts here?

Receiving other people's unicasts is clearly harmful, and I suspect most D-Bus libraries will try to reply to other people's method calls, mismatch replies and so on.

I would have expected that most libraries would be OK with receiving broadcast signals that they didn't ask for, because if they don't, they would have the same problem whenever two modules sharing a DBusConnection (or equivalent) independently used AddMatch() to subscribe to signals of interest.
Comment 20 David Herrmann 2017-07-19 14:10:16 UTC
(In reply to Simon McVittie from comment #19)
> I would have expected that most libraries would be OK with receiving
> broadcast signals that they didn't ask for, because if they don't, they
> would have the same problem whenever two modules sharing a DBusConnection
> (or equivalent) independently used AddMatch() to subscribe to signals of
> interest.

We saw several issues with libraries using libdbus1 directly and installing "pure" filters, relying on getting only what they asked for.

Off the top of my head, libvirt comes to mind, which failed in the kdbus days because we forwarded a NameOwnerChanged message which it did not subscribe to (which was even about its own name).

Most of the problems we saw were applications subscribing to a limited set of messages, and only checking for the fields where their filters differed. So many of those applications would just fail if they get random broadcasts, because their filters will be applied to messages that they clearly did not ask for.
Comment 21 Simon McVittie 2017-07-19 15:00:28 UTC
(In reply to Philip Withnall from comment #11)
> Actually, can we have a test for this in bus_config_parser_test() please?

Added as a separate commit which I'll attach in a moment

(In reply to Philip Withnall from comment #12)
> Could you clarify the send_broadcast="false" case to make it really really
> obvious that it does not match signals which have no destination set? Some
> might interpret ‘any unicast destination’ to mean ‘any destination including
> unset’.

I've tried to clarify this. In the process I realised how vague the documentation for <policy> was, and tried to clean it up without spending a lot of time rewriting dbus-daemon(1).
Comment 22 Simon McVittie 2017-07-19 15:30:49 UTC
Created attachment 132757 [details] [review]
config-parser: Clarify how <allow>, <deny> attributes work

The giant conditionals used to check policy attributes are increasingly
unwieldy, so let's try something else. Bundle together the send_
attributes, the receive_ attributes, the eavesdrop attribute
(which can go on either send or receive rules) and the other attributes
into equivalence classes, and write the conditionals in terms of those
equivalence classes.

In particular, this correctly forbids
    <allow receive_type="..." send_destination="..."/>
which was previously allowed but nonsensical (the send part took
precedence and the receive part was ignored).

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Not changed, the requested change will be a separate commit.
Comment 23 Simon McVittie 2017-07-19 15:30:57 UTC
Created attachment 132758 [details] [review]
Add a test-case for combining receive_type and send_destination

Until the previous commit, this would have worked. Now it correctly fails
with "send and receive attributes cannot be combined".

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 24 Simon McVittie 2017-07-19 15:31:11 UTC
Created attachment 132759 [details] [review]
dbus-daemon(1): Document the wildcard attribute value "*" more clearly

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 25 Simon McVittie 2017-07-19 15:31:16 UTC
Created attachment 132760 [details] [review]
dbus-daemon(1): Actually document "own" rules

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 26 Simon McVittie 2017-07-19 15:31:32 UTC
Created attachment 132761 [details] [review]
dbus-daemon(1): Clarify how user, group rules work

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 27 Simon McVittie 2017-07-19 15:31:57 UTC
Created attachment 132762 [details] [review]
dbus-daemon(1): Be more truthful about the default policy

We don't allow sending unrequested replies, but the documentation
implied that we did.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 28 Simon McVittie 2017-07-19 15:39:12 UTC
Created attachment 132763 [details] [review]
dbus-daemon(1): Document how send_* and receive_* work in general

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 29 Simon McVittie 2017-07-19 15:39:26 UTC
Created attachment 132764 [details] [review]
Add send_broadcast as an attribute of <allow> and <deny> elements

<allow send_broadcast="true" ...> only matches broadcasts,
which are signals with a NULL destination. There was previously
no way for the policy language to express "NULL destination",
only "any destination".

<allow send_broadcast="false" ...> only matches non-broadcasts,
which are non-signals or signals with a non-NULL destination.
There was previously no way for the policy language to express
"any non-NULL destination", only "any destination".

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=92853
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

---

Be clearer that send_broadcast="false" matches exactly those messages
that send_broadcast="true" does not, and neither is the same as
send_destination="*".
Comment 30 Simon McVittie 2017-07-19 15:39:51 UTC
Created attachment 132765 [details] [review]
Add a regression test for applying bus policy to broadcasts/unicasts

This test-case is actually in the test for monitoring the bus,
because it's easier to see what's going on there - the error reply
to a rejected broadcast is not visible unless you are monitoring.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=92853
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

---

Already reviewed, reattaching only for correct sequencing.
Comment 31 Simon McVittie 2017-07-19 16:11:43 UTC
Created attachment 132766 [details] [review]
dbus-daemon(1): Document how send_* and receive_* work in  general

---

Correct the description of eavesdrop: it only creates a receive rule
if there is no send_* attribute
Comment 32 Thiago Macieira 2017-07-24 23:48:46 UTC
Comment on attachment 132757 [details] [review]
config-parser: Clarify how <allow>, <deny> attributes work

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

LGTM. Ship it.
Comment 33 Thiago Macieira 2017-07-24 23:49:10 UTC
Comment on attachment 132758 [details] [review]
Add a test-case for combining receive_type and send_destination

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

LGTM.
Comment 34 Thiago Macieira 2017-07-24 23:50:21 UTC
Comment on attachment 132759 [details] [review]
dbus-daemon(1): Document the wildcard attribute value "*" more clearly

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

LGTM.
Comment 35 Thiago Macieira 2017-07-24 23:50:59 UTC
Comment on attachment 132760 [details] [review]
dbus-daemon(1): Actually document "own" rules

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

Ship it.
Comment 36 Thiago Macieira 2017-07-24 23:51:41 UTC
Comment on attachment 132761 [details] [review]
dbus-daemon(1): Clarify how user, group rules work

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

LGTM
Comment 37 Thiago Macieira 2017-07-24 23:52:26 UTC
Comment on attachment 132762 [details] [review]
dbus-daemon(1): Be more truthful about the default policy

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

LGTM.
Comment 38 Thiago Macieira 2017-07-25 00:01:17 UTC
Comment on attachment 132764 [details] [review]
Add send_broadcast as an attribute of <allow> and <deny> elements

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

Looks good and syntactically correct, though I'd recommend using "ALLOW" and "DENY" in the tristate name, instead of TRUE and FALSE. It's easier to read in the policy.c code that way.

The other thing to think about is whether we'll ever need to do matching on the destination field in the message header, as opposed to the send_destination that matches the names owned by the destination service. If we see use for that feature, I'd recommend we merge the two functionalities:
 * send_service="" -> no service specified (broadcast)
 * send_service="*" -> any service (broadcast or not)
 * send_service="%" (or "?*" or "*.*") -> any service specified (not broadcast)
 * send_service="foo.bar" -> only that name.

IMAP uses % to mean "any character except .", so we may want to reserve that for a future expansion that would allow "foo.bar.*" and "foo.bar.%" matching. "?*" means "at least one character," which implies zero is not allowed; "*.*" means everything with a dot, which all service names must have anyway, so it would match any service name.
Comment 39 Thiago Macieira 2017-07-25 00:02:01 UTC
To be clear: that was an approval for the patch. But we may want to revisit the syntax.
Comment 40 Thiago Macieira 2017-07-25 00:05:52 UTC
Comment on attachment 132765 [details] [review]
Add a regression test for applying bus policy to broadcasts/unicasts

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

Not familiar with the Glib API, but this looks sufficiently safe.
Comment 41 Thiago Macieira 2017-07-25 00:07:52 UTC
Comment on attachment 132766 [details] [review]
dbus-daemon(1): Document how send_* and receive_* work in  general

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

Looks good.

::: doc/dbus-daemon.1.xml.in
@@ +900,5 @@
> +  recipient if the message is a broadcast or a connection is eavesdropping).
> +  The last rule that matches the message determines whether it may be received.
> +  The well-known session bus normally allows receiving any message, including
> +  eavesdropping. The well-known system bus normally allows receiving any
> +  message that was not eavesdropped (any unicast message addressed to the

Does this imply we can send method calls to any connection on the bus?
Comment 42 Thiago Macieira 2017-07-25 00:11:45 UTC
(In reply to Thiago Macieira from comment #41)
> ::: doc/dbus-daemon.1.xml.in
> @@ +900,5 @@
> > +  recipient if the message is a broadcast or a connection is eavesdropping).
> > +  The last rule that matches the message determines whether it may be received.
> > +  The well-known session bus normally allows receiving any message, including
> > +  eavesdropping. The well-known system bus normally allows receiving any
> > +  message that was not eavesdropped (any unicast message addressed to the
> 
> Does this imply we can send method calls to any connection on the bus?

Quick test shows no. Trying to introspect a random client connected that isn't one of the services resulted in:

Rejected send message, 1 matched rules; type="method_call", sender=":1.1603" (uid=1000 pid=44987 comm="/home/tjmaciei/obj/qt/qt5/qtbase/bin/qdbus --syste") interface="org.freedesktop.DBus.Introspectable" member="Introspect" error name="(unset)" requested_reply="0" destination=":1.102" (uid=1000 pid=28095 comm="/usr/bin/akonadi_imap_resource --identifier akonad")

But like mentioned in the other bug report, I *can* send it a signal:
 dbus-send --system --dest=:1.102 --type=signal / org.foo.Signal

Nothing was logged in syslog.
Comment 43 Simon McVittie 2017-07-25 00:17:19 UTC
(In reply to Thiago Macieira from comment #41)
> Comment on attachment 132766 [details] [review]
> dbus-daemon(1): Document how send_* and receive_* work in  general
> 
> ::: doc/dbus-daemon.1.xml.in
> @@ +900,5 @@
> > +  recipient if the message is a broadcast or a connection is eavesdropping).
> > +  The last rule that matches the message determines whether it may be received.
> > +  The well-known session bus normally allows receiving any message, including
> > +  eavesdropping. The well-known system bus normally allows receiving any
> > +  message that was not eavesdropped (any unicast message addressed to the
> 
> Does this imply we can send method calls to any connection on the bus?

No, send and receive policies are checked separately. A message is only delivered if the sender is allowed to send it && the recipient is allowed to receive it. What this is saying is essentially "in practice, you only need to consider the send policy".

(In reply to Thiago Macieira from comment #42)
> But like mentioned in the other bug report, I *can* send it a signal:
>  dbus-send --system --dest=:1.102 --type=signal / org.foo.Signal

Yes. That's what <allow send_type="signal"/> in system.conf does (as mentioned in Comment #0). If the policy language had always had send_broadcast, then this rule would probably have been <allow send_type="signal" send_broadcast="true"/> - but it doesn't exist (and the point of this feature request is to add it) so that can't happen yet.
Comment 44 Simon McVittie 2017-07-25 00:32:32 UTC
(In reply to Thiago Macieira from comment #38)
> Looks good and syntactically correct, though I'd recommend using "ALLOW" and
> "DENY" in the tristate name, instead of TRUE and FALSE. It's easier to read
> in the policy.c code that way.

That would be wrong. Whether the rule is an allow or deny rule is orthogonal to whether it matches broadcasts or unicasts. <allow send_broadcast="false"/> allows sending unicasts and says nothing about broadcasts; <allow send_broadcast="true"/> allows sending broadcasts and says nothing about unicasts. Corresponding <deny> rules can also exist.

If you want to make this more specific, at the cost of making it non-reusable for other tristates (which might be better, I'm not sure), the enum would have to be { UNICAST, BROADCAST, ANYTHING }.

When looking at the send_ and receive_ attributes, think "does this match?" rather than "does this allow?". If the attributes don't match the message, the rule is ignored. If they do match, the allow or deny element name (the "action") determines what happens as a result.

> The other thing to think about is whether we'll ever need to do matching on
> the destination field in the message header, as opposed to the
> send_destination that matches the names owned by the destination service.

My instinct is to say we aren't going to need it, because it wouldn't make much sense - practical client libraries (libdbus, GDBus, QtDBus, sd-bus etc.) treat messages sent to any of the names they own as being exactly equivalent.

> If we see use for that feature, I'd recommend we merge the two functionalities:
>  * send_service="" -> no service specified (broadcast)
>  * send_service="*" -> any service (broadcast or not)
>  * send_service="%" (or "?*" or "*.*") -> any service specified (not
> broadcast)
>  * send_service="foo.bar" -> only that name.

I prefer send_broadcast. I think it's clearer for what we are likely to use it to say.

I don't think we'll ever want full ?/* glob matching, and I'm not sure % is particularly useful either. I suspect the only syntax expansion that would be useful in practice for destinations is "namespace" matching (pattern "foo.bar" matches destinations "foo.bar", "foo.bar.baz" and "foo.bar.a.b.c" but not "foo.barred"), like arg0namespace in signal matches - and if we add that in future then I think I'd prefer to spell it as send_destination_namespace="foo.bar".
Comment 45 Thiago Macieira 2017-07-25 01:33:43 UTC
Fair enough. Then let's proceed with send_broadcast.
Comment 46 Simon McVittie 2017-07-28 10:28:50 UTC
Merged send_broadcast for 1.11.18. A couple more patches on the way.
Comment 47 Simon McVittie 2017-07-28 10:32:23 UTC
Created attachment 133096 [details] [review]
config-parser: Fail on impossible send_broadcast/send_destination pair

If we add a rule like

    <allow send_destination="com.example" send_broadcast="true"/>

then it cannot possibly match anything, because to be a broadcast, the
message would have to have no destination. The only value of
send_destination that can be combined with send_broadcast="true" is
the wildcard "*", but by this point in the function we already
replaced "*" with NULL.

Adapted from an earlier implementation of send_broadcast by
Alban Crequy.

---

I found an old git branch from when Alban was still working at Collabora, in which we'd implemented this as a way to make it possible to configure the dbus-daemon to impose the same restrictions as kdbus. I mostly prefer my new implementation, but this bit of the old implementation was quite nice, so I propose we reinstate it.
Comment 48 Simon McVittie 2017-07-28 10:33:02 UTC
Created attachment 133097 [details] [review]
test/data: Test impossible send_broadcast/send_destination pair
Comment 49 Philip Withnall 2017-07-31 13:11:29 UTC
Comment on attachment 133096 [details] [review]
config-parser: Fail on impossible send_broadcast/send_destination pair

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

++
Comment 50 Philip Withnall 2017-07-31 13:11:51 UTC
Comment on attachment 133097 [details] [review]
test/data: Test impossible send_broadcast/send_destination pair

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

++
Comment 51 Simon McVittie 2017-09-25 13:51:25 UTC
Thanks, fixed in git for 1.11.18.

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.