Bug 18961 - nondeterministic denials for no-interface messages
nondeterministic denials for no-interface messages
Status: NEEDINFO
Product: dbus
Classification: Unclassified
Component: core
1.5
All All
: medium normal
Assigned To: Havoc Pennington
John (J5) Palmieri
: NEEDINFO
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-08 13:21 UTC by Colin Walters
Modified: 2014-10-03 12:18 UTC (History)
7 users (show)

See Also:
i915 platform:
i915 features:


Attachments
don't process deny rules if interface doesn't match (433 bytes, patch)
2008-12-08 13:21 UTC, Colin Walters
Details | Splinter Review
workaround for deny send_interface (2.42 KB, patch)
2008-12-17 17:30 UTC, Colin Walters
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Walters 2008-12-08 13:21:28 UTC
Created attachment 20913 [details] [review]
don't process deny rules if interface doesn't match

There is a flaw (or at least, problematic behavior) in the current policy logic that causes nondeterminism.  This is not a security flaw per se, more of a reliability flaw because it causes denials where they were not intended.  This flaw mirrors CVE-2008-0595 (http://lists.freedesktop.org/archives/dbus/2008-February/009401.html).  The problem stems from rules of the form:

File A:
<policy context="default>
  <allow send_destination="com.foo.MyService"/>
</policy>

File B:
<policy context="default">
  <allow send_destination="org.foo.Bar"/>
  <deny send_interface="org.foo.Bar.Secure"/>
</policy>

This looks innocuous enough at first (as all these rules do); clearly the intent of the second rule is to lock down access to just part of org.foo.Bar.  But it breaks the case of messages sent with no interface to the com.foo.MyService:  **depending on include ordering**

The no-interface case happens in dynamic language scripts like Python typically.

Tracing the logic in policy.c, what happens when a message is sent with no interface is that we see the rule has an interface, then fall into the logic:

          if ((no_interface && rule->allow) ||
               (!no_interface && 
                strcmp (dbus_message_get_interface (message),
                        rule->d.send.interface) != 0))
            continue;

So we're processing a deny rule, with no interface, and this logic does *not* match.  Thus we hit the rule, and the default becomes *deny*.  If we happened to process the rules from the other file first, the message will be spuriously denied.
Comment 1 Ray Strode [halfline] 2008-12-08 14:27:12 UTC
So, we probably should have caught this when we did CVE-2008-0595.  It's the exact same issue, but for deny rules instead of allow rules.

Your patch seems right to me.  The only reason I can think that the rule->allow test was there at all was because of a thinko.  Adding it there kept the semantics in the non-allow case the same and since the non-allow case couldn't be a security issue we didn't think about it hard enough.

The scenario is definitely bogus though, and the fix looks right.

What about the receive?  It's got the same problem right?
  
Comment 2 Ray Strode [halfline] 2008-12-08 14:52:34 UTC
"Your patch seems right to me"

Famous last words.

Colin found me and noticed the patch is bogus.

The patch even has the comment

"for deny rules, if the message has no interface we want to use the rule (and thus deny)."

calling continue; prematurely will sidestep some checks and reintroduce a security hole.
Comment 3 Ray Strode [halfline] 2008-12-08 15:03:40 UTC
One idea was to ignore (well post nastygram in syslog for) lines like:

  <deny send_interface="org.foo.Bar.Secure"/>

(i.e. require a send_destination when giving a send_interface)

Comment 4 Colin Walters 2008-12-08 18:27:54 UTC
Comment on attachment 20913 [details] [review]
don't process deny rules if interface doesn't match

As rstrode said, this patch is wrong.  We need to fix every rule that does <deny send_interface="foo"/>.
Comment 5 Colin Walters 2008-12-16 10:17:31 UTC
Let's make this bug into a tracker for bare <deny send_interface/>.  

First, GDM:

http://bugzilla.gnome.org/show_bug.cgi?id=564767
Comment 6 Colin Walters 2008-12-16 11:02:55 UTC
yum:

https://bugzilla.redhat.com/show_bug.cgi?id=476718
Comment 7 Colin Walters 2008-12-16 11:03:16 UTC
Setroubleshoot:

https://bugzilla.redhat.com/show_bug.cgi?id=476722
Comment 8 Colin Walters 2008-12-16 11:16:43 UTC
dnsmasq (no upstream bug tracker (sigh), patch follows):

--- dnsmasq-2.45/dbus/dnsmasq.conf	2008-07-20 14:26:07.000000000 -0400
+++ dnsmasq-2.45.orig/dbus/dnsmasq.conf	2008-12-16 14:11:48.000000000 -0500
@@ -5,12 +5,10 @@
 	<policy user="root">
 		<allow own="uk.org.thekelleys.dnsmasq"/>
 		<allow send_destination="uk.org.thekelleys.dnsmasq"/>
-                <allow send_interface="uk.org.thekelleys.dnsmasq"/>
 	</policy>
 	<policy context="default">
                 <deny own="uk.org.thekelleys.dnsmasq"/>
                 <deny send_destination="uk.org.thekelleys.dnsmasq"/>
-                <deny send_interface="uk.org.thekelleys.dnsmasq"/>
         </policy>
 </busconfig>
 


Comment 9 Colin Walters 2008-12-16 11:18:16 UTC
ConsoleKit is in both the broken services bug and this set.

http://bugs.freedesktop.org/show_bug.cgi?id=19020
Comment 10 Colin Walters 2008-12-16 12:13:49 UTC
NetworkManager:

http://bugzilla.gnome.org/show_bug.cgi?id=563730
Comment 11 Tomas Hoger 2008-12-17 00:56:53 UTC
(In reply to comment #8)
> dnsmasq (no upstream bug tracker (sigh), patch follows):

Following the approach used in other clean-up changes, whole context="default" part can be dropped, as it only contains denies, right?

dnsmasq also does:
  dbus_message_new_signal(DNSMASQ_PATH, DNSMASQ_SERVICE, "Up")
so based on what the default for signals is, it may or may not need extra rule to allow this (not needed with current:
  http://cgit.freedesktop.org/dbus/dbus/commit/?id=920c3c02 )
Comment 12 Colin Walters 2008-12-17 10:26:25 UTC
I suggest we not remove any existing denies in the default context for now.  This will allow the policy to remain secure even when used with dbus 1.2.4 (and upcoming temporary dbus.1.2.4.Xpermissive stream).
Comment 13 Colin Walters 2008-12-17 13:50:29 UTC
This just gets worse.  Here's a denial I see with my new logging patch:

Dec 17 16:39:40 space-ghost dbus: Rejected send message, 20 matched rules; type="method_return", sender=":1.70" (uid=0 pid=4821 comm
="/usr/libexec/nm-dispatcher.action ") interface="(unset)" member="(unset)" error name="(unset)" destination=":1.18" (uid=0 pid=2753
 comm="NetworkManager --pid-file=/var/run/NetworkManager/"))

I believe what's happening is that all the <deny send_interface=""/> rules are applying to this even though it's a method return, just because it has no interface!  

This makes all those bare deny send_interface rules even more damaging than we originally thought.  

My temptation here is to make <deny send_interface=""/> only apply to method_call and signal.  (Or possibly only !requested_reply?)  In other words, automatically turn

<deny send_interface="foo"/>

into:

<deny send_interface="foo" send_type="method_call"/>
<deny send_interface="foo" send_type="signal"/>

The best thing to do of course is to fix all of those rules so that they use send_destination, but there are a *lot*.
Comment 14 Colin Walters 2008-12-17 15:52:48 UTC
Thinking about this, if we are really denying method returns now like that, why don't I see a ton more of them?  
Comment 15 Colin Walters 2008-12-17 16:12:21 UTC
(In reply to comment #14)
> Thinking about this, if we are really denying method returns now like that, why
> don't I see a ton more of them?  

Likely answer here: That message is actually an unrequested reply.  For requested replies, this logic kicks in: if (requested_reply && !rule->allow && !rule->d.send.requested_reply) continue;

Comment 16 Colin Walters 2008-12-17 17:30:04 UTC
Created attachment 21257 [details] [review]
workaround for deny send_interface

Proposed partial workaround for spurious denials with deny send_interface
Comment 17 Colin Walters 2008-12-18 08:56:04 UTC
(In reply to comment #16)
> Created an attachment (id=21257) [details]
> workaround for deny send_interface
> 
> Proposed partial workaround for spurious denials with deny send_interface

I can confirm this patch reduces the unrequested reply denial to 1 instead of N (where N is the number of <deny send_interface=""/> one has).

It's an open question whether this patch matters - we're denying unrequested replies by default anyways.  I'm dropping this one from proposal for the first 1.2.4.Xpermissive stream, but I'd like discussion of whether it makes sense to put in later.
Comment 19 Ludwig Nussel 2009-01-27 07:53:43 UTC
Have you arrived at a conclusion about how to address this?
Comment 20 Colin Walters 2009-01-27 08:06:23 UTC
Basically, messages sent with no interface on the system bus have been broken for a while, and we just have to remove these rules from config files one at a time.

This issue isn't as severe as one might think at first - because they haven't been working for a long time, no one can be relying on them.  Generally the best thing is for code to explicitly specify the interface anyways.
Comment 21 Ludwig Nussel 2009-01-27 08:15:01 UTC
I just wonder whether we need to fix that in older distributions. Hopefully it's safe to ignore those log messages there :-)
Comment 22 Colin Walters 2009-01-27 08:44:50 UTC
It should be - but what denials are you getting?
Comment 23 Ludwig Nussel 2009-01-27 23:46:33 UTC
The same as you got in commment #13 from nm-dispatcher.action
Comment 24 Colin Walters 2009-01-28 06:47:04 UTC
Comment #13 is mainly a dbus-glib bug for trying to send a reply even when none was requested (https://bugs.freedesktop.org/show_bug.cgi?id=19441), fixed in git master.  You can see the analysis for that one here: http://bugzilla.gnome.org/show_bug.cgi?id=565008

I'm not actually sure offhand if this bug applies to it too; hm, it's kind of strange, I see some old denials in my logs with 20+ matched rules (implying that these <deny send_interface> rules are active), and also many that only have one.  I'll investigate.

Anyways the bottom line is that we need a new dbus-glib release with that bug fix which will quiet that issue.
Comment 25 Ghee Teo 2009-02-04 07:28:59 UTC
I am puzzled by the proposed changes for apps in /etc/dbus-1/system.d

Colin stated line comment #5 bare <deny send_interface/>.  
suggestion is to provide a matching rule for send_destination and send_interface
be it an allow rule or deny rule.

Then in comment#8 the patch is simply to remove the send_interface regardless of whether it is a allow or denny. This is in contradiction to comment #5 so which is it then?

I am looking to see whether the avahi dbus conf file needs to be patched or not.

Thanks

Comment 26 Colin Walters 2009-02-04 08:38:59 UTC
(In reply to comment #25)

> Then in comment#8 the patch is simply to remove the send_interface regardless
> of whether it is a allow or denny. This is in contradiction to comment #5 so
> which is it then?

Both the allow and deny send_interface rules were redundant with the send_destination ones (as is the cast for almost but not all services).

> I am looking to see whether the avahi dbus conf file needs to be patched or
> not.

I don't think we ended up with an avahi patch.
Comment 27 Ghee Teo 2009-02-04 09:37:34 UTC
(In reply to comment #26)
> (In reply to comment #25)
> 
> > Then in comment#8 the patch is simply to remove the send_interface regardless
> > of whether it is a allow or denny. This is in contradiction to comment #5 so
> > which is it then?
> 
> Both the allow and deny send_interface rules were redundant with the
> send_destination ones (as is the cast for almost but not all services).
> 
> > I am looking to see whether the avahi dbus conf file needs to be patched or
> > not.
> 
> I don't think we ended up with an avahi patch.
Just to be sure, you are saying this file,
http://www.avahi.org/browser/avahi-daemon/avahi-dbus.conf.in

does not need updated for dbus 1.2.8 and beyond?

Thanks,

-Ghee


Comment 28 Colin Walters 2009-02-04 14:59:28 UTC
Well it does have a <deny send_interface>, however it also has a specific member match "SetHostName", so while it should be fixed (by also adding a send_destination="org.freedesktop.Avahi", it will only affect any no-interface method).

Let's avahi to the list of things that need to be fixed for this bug.  I tried to file a bug at avahi.org but apparently don't have permissions.
Comment 29 Colin Walters 2009-02-04 15:45:39 UTC
Avahi bug filed:

http://avahi.org/ticket/263
Comment 30 Simon McVittie 2011-01-26 10:12:52 UTC
Is there anything more for us to do here?