Created attachment 132777 [details] [review]
policy: Add max_fds, min_fds qualifiers for send, receive rules

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

Created attachment 132778 [details] [review]
test: Exercise min_fds, max_fds

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

Comment on attachment 132777 [details] [review]
policy: Add max_fds, min_fds qualifiers for send, receive rules

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

By itself the patch looks good. Ship it. 

Just wondering if we need to specify a max_fds="*" to allow any number. Of course, that is the same as not specifying max_fds in the first place, so this may be of limited use...

I see there's no checking that max_fds >= min_fds, but I wouldn't bother. You're likely to notice that problem as you're developing your software.

::: dbus/dbus-message-util.c
@@ +51,5 @@
> + * @param message the message
> + * @returns the number of file descriptors
> + */
> +unsigned int
> +_dbus_message_get_n_unix_fds (DBusMessage *message)

Might be interesting to add a public equivalent of this function.

Comment on attachment 132778 [details] [review]
test: Exercise min_fds, max_fds

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

Looks ok

(In reply to Thiago Macieira from comment #3)
> Just wondering if we need to specify a max_fds="*" to allow any number. Of
> course, that is the same as not specifying max_fds in the first place, so
> this may be of limited use...

I don't think that has any useful purpose. (You'll notice that omitting max_fds, as currently implemented, is equivalent to setting max_fds to the largest number the D-Bus Specification could theoretically let us put in a message - I didn't bother adding a special case for "unlimited" because the maximum message size imposes a limit anyway.)

> I see there's no checking that max_fds >= min_fds, but I wouldn't bother.
> You're likely to notice that problem as you're developing your software.

Indeed. <allow send_destination="*" max_fds="1" min_fds="42"/> is a perfectly valid allow rule that happens to be impossible to satisfy, hence won't ever match any messages :-)

> > +unsigned int
> > +_dbus_message_get_n_unix_fds (DBusMessage *message)
> 
> Might be interesting to add a public equivalent of this function.

Yeah, perhaps.

Fixed in git for 1.11.18

(In reply to Simon McVittie from comment #6)
> Fixed in git for 1.11.18

I don't like this addition. I don't see its purpose, and to me it introduces a knob that we will have to maintain and support, without providing a real value.

This adds configuration knobs that have no use beyond working around bugs in dbus-daemon. To me, this is like adding a knob that allows matching on the number of bytes a message carries.

I believe that we are doing something fundamentally wrong if we have to rely on policies to match for the data size that we transmit. The body of a message is under application control and we should forward it untouched. I know that we verify their syntactic correctness, and that matches interpret it. But I believe that the general interpretation of all this should still be that we don't look at the body. Introducing policy-knobs that do feels wrong to me.

If we commit to dbus-daemon transmitting FDs, than we should make it a first-class message payload the same way the body-data is.

I see no purpose in this configuration knob.

The two stated reasons for this knob were:

> * File descriptor passing on the system bus is useful but scary, because
>   file descriptors are relatively "expensive", so the arbitrary limits
>   imposed by the kernel are small. Sysadmins with mutually distrusting
>   users could avoid a fairly broad class of potential denial of service
>   attacks by limiting fd-passing to those services that actually need it.

Can we please fix FD-passing, rather than saying "only this set of services is allowed to use them, and lets hope those services don't have bugs that allow attackers to then exploit a broken dbus-daemon"?

Why is FD-passing scary?

> * Broadcasting fds is currently allowed (!), but this is completely
>   bizarre and should almost certainly not work.

Broadcasting FDs works fine. Introducing knobs to detect bizarre use-cases feels to me like taping over bad code to make us feel better, rather than strengthening the underlying concepts and making fd-passing work fine.

To me, "min_fds" and "max_fds" is a MAC feature. We already have MACs. I would prefer using SELinux and friends to do MAC filtering, rather than extending the policy language. A MAC mechanism might want to do something like this:

    <allow send_destination="org.example.foobar"
           send_method="Foobar"
           signature="uua{sv}"
           min_bytes="16"
           max_bytes="4096" />

..but I fail to see why we need why this is needed in our policy-language.

Thanks
David

(In reply to David Herrmann from comment #7)
> I don't like this addition. I don't see its purpose, and to me it introduces
> a knob that we will have to maintain and support, without providing a real
> value.

The bottom line here is:

The kernel "blames" dbus-daemon for some amount of the resource consumption of the overall transaction involved in relaying a message, because dbus-daemon is a perfectly ordinary user-space process that does not have special dispensation to  ask the kernel to account resources to its peers. Bug #33606 and Bug #81043 are examples of the sort of things that can happen.

The other processes on the bus (clients of the dbus-daemon) can have similar issues: for example, if a service doesn't dispatch its socket in a timely fashion (main loop is blocked), it will get "blamed" for the resource consumption of its unread messages, to at least some extent.

For file descriptors, the situation is the same as for memory, but much more so, because the kernel's arbitrary limits are orders of magnitude smaller.

Perhaps this indicates design flaws in the D-Bus protocol (which I should note was not designed by any of the current maintainers). Unfortunately, the right time to fix design flaws in incompatible ways was literally more than a decade ago for D-Bus in general, or 8 years ago for file descriptor passing. So we are doing the best we can to solve or mitigate failure modes without breaking real applications.

Given your involvement in the kernel, I'm sure you can understand the competing demands of "don't break your users" (user-space for the kernel, D-Bus clients for dbus-daemon) and "don't have abusable behaviours". Sometimes they conflict. Perhaps someone with more time and intelligence than me can resolve that conflict, but until they do, I have to do the best that I can, because the only alternative available to me is to do nothing and hope someone else steps up. If that isn't good enough for you then all I can do is apologise.

> This adds configuration knobs that have no use beyond working around bugs in
> dbus-daemon. To me, this is like adding a knob that allows matching on the
> number of bytes a message carries.

To be honest, policy rules that look at the size of a message seem perfectly reasonable to me - they would be a considerably less blunt instrument than the current connection-wide limits.

(In contrast, policy rules that look at the signature are less appropriate than counting fds or bytes, because as far as I am aware, equally-sized messages' resource consumption does not alter significantly with different signatures.)

> Can we please fix FD-passing, rather than saying "only this set of services
> is allowed to use them, and lets hope those services don't have bugs that
> allow attackers to then exploit a broken dbus-daemon"?

I would be delighted to see someone do so. We are a team of maintainers who all have other things we need to be doing, maintaining dbus because we need it for our other work. Telling us that our best attempts are unacceptable, without helping to do better, does not actually improve any code, and on the contrary is only going to discourage taking action.

I don't currently have the mental bandwidth to fully understand the problem space around fd-passing and its resource accounting implications, let alone solve it. Perhaps this makes me a terrible maintainer who should resign or be thrown out of the project, but given that I've been the one finalizing every dbus release since 2012, I don't really see how my absence would be likely to cause a net improvement in dbus' quality.

> To me, "min_fds" and "max_fds" is a MAC feature. We already have MACs. I
> would prefer using SELinux and friends to do MAC filtering, rather than
> extending the policy language.

You are correct to say that the entire <policy> mechanism duplicates the MAC mediation functionality that we have for SELinux and AppArmor, and this is why in general we discourage elaborate uses of <policy>. However, we cannot just remove the feature, because the time to be making incompatible changes to dbus-daemon was more than a decade ago.

<policy> is also one of the few tools we have to control what messages we will relay on any basis more fine-grained than "limit per connection". It is an imperfect fit, but we use what we have.

(In reply to Simon McVittie from comment #8)
> The kernel "blames" dbus-daemon for some amount of the resource consumption
> of the overall transaction involved in relaying a message, because
> dbus-daemon is a perfectly ordinary user-space process that does not have
> special dispensation to  ask the kernel to account resources to its peers.
> Bug #33606 and Bug #81043 are examples of the sort of things that can happen.

[...]

> For file descriptors, the situation is the same as for memory, but much more
> so, because the kernel's arbitrary limits are orders of magnitude smaller.
> 
> Perhaps this indicates design flaws in the D-Bus protocol (which I should
> note was not designed by any of the current maintainers). Unfortunately, the
> right time to fix design flaws in incompatible ways was literally more than
> a decade ago for D-Bus in general, or 8 years ago for file descriptor
> passing. So we are doing the best we can to solve or mitigate failure modes
> without breaking real applications.

Accounting is tricky. And I agree, the problem is the same for memory as for FDs, just extra tricky for FDs. I think the D-Bus protocol is mostly fine, and that there are ways of handling the accounting both of FDs and memory without making incompatible changes. We have been working on proposals for this, which we hope to take to the dbus mailing-list for debate soon (though we want to have a proof-of-concept fully done to make sure we have not missed anything before we do so, so sorry for this taking longer than we had hoped).

> > Can we please fix FD-passing, rather than saying "only this set of services
> > is allowed to use them, and lets hope those services don't have bugs that
> > allow attackers to then exploit a broken dbus-daemon"?
> 
> I would be delighted to see someone do so. We are a team of maintainers who
> all have other things we need to be doing, maintaining dbus because we need
> it for our other work. Telling us that our best attempts are unacceptable,
> without helping to do better, does not actually improve any code, and on the
> contrary is only going to discourage taking action.

We have been trying to help by filing bug reports with test-cases and proof-of-concept fixes. It would obviously be better if we had provided actual fixes too, but at least to me it did not even seem clear yet what solutions would be acceptable.

> I don't currently have the mental bandwidth to fully understand the problem
> space around fd-passing and its resource accounting implications, let alone
> solve it. Perhaps this makes me a terrible maintainer who should resign or
> be thrown out of the project, but given that I've been the one finalizing
> every dbus release since 2012, I don't really see how my absence would be
> likely to cause a net improvement in dbus' quality.

I don't think anyone intended to question your abilities as a maintainer, so please don't take it that way... :(