Bug 86194

(In reply to Jacek Bukarewicz from comment #0)
> In bus_activation_send_pending_auto_activation_messages, when
> bus_dispatch_matches returns FALSE, error is sent. However in certain
> circumstances error is not set.

If that's the case, then it's a bug. Please fix it in the correct place: bus_dispatch_matches (or perhaps a function that is called by bus_dispatch_matches).

DBusError conventions are basically the same as GLib's GError conventions: if a function returns a particular value to indicate failure, and it has an error parameter, then the error parameter should be filled if and only if the failure value is returned.

I notice your email address says Samsung, and I've seen a lot of Samsung people doing things related to the Smack LSM. If you are patching dbus-daemon or libdbus for Smack, kdbus or similar, please verify that your patches have correct error handling. I am aware that the existing SELinux code does not do some of its error handling in the conventional way, so any LSM integration that is based on that is likely to have copy-pasted those design flaws. In particular, the unmerged branch on Bug #47581 has bugs in its error-handling. If you want dbus-daemon to have Smack LSM support, responding to that branch's code review and getting it merged would be an excellent next step :-)

> In such case there is assertion
> failure in bus_transaction_send_error_reply which triggers SIGABRT.

It's fine for debugging, and obviously any assertion failure indicates a bug somewhere, but compiling a "production" version of dbus-daemon with assertions enabled is not recommended.

In my patch I followed approach taken in bus_dispatch which also calls bus_dispatch_matches and in case of failure sends error message, but only if the error is set. This, I believe, was done so error message due to recipient policy denial is not sent to the sender.
I started wondering if not sending error message due to recipient policy denial is intended as it wasn't always the case and I think informing sender about denial at addressed recipient is reasonable.
I see that the change was made in commit 79f02ca which looks like the one that is meant to do merely code refactoring, not bigger changes. Before this commit, when bus_client_policy_check_can_receive() failed in bus_context_check_security_policy(), error was set which in turn caused error message to be sent.
If this change has been made by accident then I believe we could fix it by putting error instead of NULL in the last parameter of complain_about_message that is called when bus_client_policy_check_can_receive fails. Otherwise fix would be harder - there would be a need to differentiate between errors due to sender and recipient policy denial so that we know whether to send error message or not.

As for the smack patches - I'm not sure if anyone is working on them at the moment because a new security approach in Tizen is being developed. It uses Cynara (polkit-like security daemon but lighter and thus better suited for embedded environments).
We are also experimenting with integrating it with D-Bus daemon. We do it by extending XML policy language with <check ... privilege="privilege_name" /> element. It works like <allow> and <deny> but the answer is taken from the Cynara daemon or client-side cache if available.

 Patches are available here if you are interested: git://review.tizen.org/platform/upstream/dbus (sandbox/jacekbe/cynara-integration branch)

I'm not sure if you would like to see such changes in upstream D-Bus. Especially parts when answer is not available immediately and special measures must be taken to not block message dispatching (and keep message order as much as possible)  might seem to intrusive, but obviously from our point of view it would be ideal to make D-Bus support external policy checkers. Anyway, it's probably not the best place to discuss it, but I felt it might be good to inform you about it already. I planned to add new topic in D-Bus mailing list when it's finished (it's working, but we'd like to check if it really suits our needs and does not cause problems before announcing it).

Created attachment 109474 [details] [review]
Set error on receive rule denial

Set error when message delivery is denied due to receive rule
    
This makes bus_context_check_security_policy follow convention of
setting errors if function indicates failure and has error parameter.
Notable implication is that AccessDenied error will be sent if sending message
to addressed recipient is denied due to receive rule. Previously, message
was silently dropped.
This also fixes assertion failure when message is denied at addressed recipient while sending pending auto activation messages.

Thanks, your new patch looks good.

(In reply to Jacek Bukarewicz from comment #2)
> We are also experimenting with integrating it with D-Bus daemon. We do it by
> extending XML policy language with <check ... privilege="privilege_name" />
> element. It works like <allow> and <deny> but the answer is taken from the
> Cynara daemon or client-side cache if available.
...
> I'm not sure if you would like to see such changes in upstream D-Bus.
> Especially parts when answer is not available immediately and special
> measures must be taken to not block message dispatching (and keep message
> order as much as possible)

I don't think I would be comfortable with merging asynchronous policy checking into dbus-daemon, no; that seems like a recipe for making dbus-daemon unmaintainable. I'm also uncomfortable with "keep message order as much as possible": either you can guarantee what dbus-daemon currently guarantees (that each sender's messages are kept in their original order, and interleaved in some arbitrary order chosen by the dbus-daemon that is the same for every recipient), or you can't.

If you need to be asynchronous (like the use cases PolicyKit was designed for, where a less-privileged client talks to a trusted service and the trusted service must determine whether to obey it), the approach taken by PolicyKit seems better, particularly with some good syntactic sugar for the service (which sd-bus seems to have) - the message is delivered, and the service explicitly calls out to PK to ask "hey is this OK?". It knows its own ordering constraints, so it can know how much queue-jumping is acceptable.

If you don't need to be asynchronous (like the use cases LSMs were designed for, flat-out forbidding communication between certain components even if both of those components might be conspiring against you), a blocking call into an LSM or whatever seems appropriate... or maybe some sort of containerization (possibly with some sort of proxying for a limited set of services) where the components *can't* communicate.

Fixed in git for 1.8.12, 1.9.4. Thanks!