Description
Simon McVittie
2015-11-07 10:48:17 UTC
Created attachment 119459 [details] [review] Add send_broadcast as an attribute of <allow> and <deny> elements 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 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 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. (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. 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). 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] 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 Also available at https://github.com/smcv/dbus/commits/92853-send-broadcast 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 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 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 on attachment 131604 [details] [review] [3/3] Add a regression test for applying bus policy to broadcasts/unicasts Review of attachment 131604 [details] [review]: ----------------------------------------------------------------- ++ (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. (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. 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). (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. (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. (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. (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. (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). 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. 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> Created attachment 132759 [details] [review] dbus-daemon(1): Document the wildcard attribute value "*" more clearly Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132760 [details] [review] dbus-daemon(1): Actually document "own" rules Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132761 [details] [review] dbus-daemon(1): Clarify how user, group rules work Signed-off-by: Simon McVittie <smcv@collabora.com> 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> Created attachment 132763 [details] [review] dbus-daemon(1): Document how send_* and receive_* work in general Signed-off-by: Simon McVittie <smcv@collabora.com> 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="*". 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. 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 on attachment 132757 [details] [review] config-parser: Clarify how <allow>, <deny> attributes work Review of attachment 132757 [details] [review]: ----------------------------------------------------------------- LGTM. Ship it. 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 on attachment 132759 [details] [review] dbus-daemon(1): Document the wildcard attribute value "*" more clearly Review of attachment 132759 [details] [review]: ----------------------------------------------------------------- LGTM. Comment on attachment 132760 [details] [review] dbus-daemon(1): Actually document "own" rules Review of attachment 132760 [details] [review]: ----------------------------------------------------------------- Ship it. Comment on attachment 132761 [details] [review] dbus-daemon(1): Clarify how user, group rules work Review of attachment 132761 [details] [review]: ----------------------------------------------------------------- LGTM Comment on attachment 132762 [details] [review] dbus-daemon(1): Be more truthful about the default policy Review of attachment 132762 [details] [review]: ----------------------------------------------------------------- LGTM. 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. To be clear: that was an approval for the patch. But we may want to revisit the syntax. 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 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? (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. (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. (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". Fair enough. Then let's proceed with send_broadcast. Merged send_broadcast for 1.11.18. A couple more patches on the way. 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. Created attachment 133097 [details] [review] test/data: Test impossible send_broadcast/send_destination pair Comment on attachment 133096 [details] [review] config-parser: Fail on impossible send_broadcast/send_destination pair Review of attachment 133096 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 133097 [details] [review] test/data: Test impossible send_broadcast/send_destination pair Review of attachment 133097 [details] [review]: ----------------------------------------------------------------- ++ 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.