On Wed, 28 Jan 2015 at 14:14:56 +0000, Simon McVittie wrote: > Patches also available from: > ssh://git.debian.org/git/users/smcv/lintian.git dbus
Oops, I pointed to a git repository but forgot to attach them. Here they are. S
>From 090485494408f4c72b8768d95160e8950c3cc7e3 Mon Sep 17 00:00:00 2001 From: Simon McVittie <s...@debian.org> Date: Mon, 5 Jan 2015 16:21:27 +0000 Subject: [PATCH 1/5] Transcode checks/dbus.pm to UTF-8 The UTF-8 turned into Latin-1 on its way through the BTS, or something. --- checks/dbus.pm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/checks/dbus.pm b/checks/dbus.pm index 364c980..23ea0cd 100644 --- a/checks/dbus.pm +++ b/checks/dbus.pm @@ -1,7 +1,7 @@ # dbus -- lintian check script, vaguely based on apache2 -*- perl -*- # -# Copyright © 2012 Arno Töll -# Copyright © 2014 Collabora Ltd. +# Copyright © 2012 Arno Töll +# Copyright © 2014 Collabora Ltd. # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by -- 2.1.4
>From b5bb0b11edb3ab2c7fc58c6e73b64cecb4d7c8aa Mon Sep 17 00:00:00 2001 From: Simon McVittie <s...@debian.org> Date: Mon, 5 Jan 2015 16:49:51 +0000 Subject: [PATCH 2/5] dbus: capture <policy> for each <allow> or <deny> rule Seeing a rule that says <allow send_interface="x.y.z"/> doesn't tell you whether it is <policy user="root"><allow send_interface="x.y.z"/> (usually an anti-pattern, but sometimes necessary for the "agent" pattern as seen in BlueZ) or <policy context="default"><allow send_interface="x.y.z"/> (which should ring alarm bells). To solve this, capture the enclosing <policy> for each <allow> or <deny> rule. This also means our output for the at_console check can indicate precisely which rules apply to console users. --- checks/dbus.pm | 31 +++++++++++++++------- .../debian/etc/dbus-1/system.d/at-console.conf | 2 ++ t/tests/dbus-policy/tags | 9 ++++--- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/checks/dbus.pm b/checks/dbus.pm index 23ea0cd..2aecae8 100644 --- a/checks/dbus.pm +++ b/checks/dbus.pm @@ -70,20 +70,33 @@ sub _check_policy { # note that we are parsing the entire file as one big string, # so that we catch <policy\nat_console="true"\n> or whatever. - if ($xml =~ m{<policy[^>]+at_console=(["'])true\1.*?</policy>}s) { - tag('dbus-policy-at-console', $file); - } - my @rules; - while ($xml =~ m{(<(?:allow|deny)[^>]+send_\w+=[^>]+>)}sg) { - push(@rules, $1); + # a small rubbish state machine: we want to match a <policy> containing + # any <allow> or <deny> rule that is about sending + my $policy = ''; + while ($xml =~ m{(<policy[^>]*>)|(</policy\s*>)|(<(?:allow|deny)[^>]*>)}sg) + { + if (defined $1) { + $policy = $1; + } elsif (defined $2) { + $policy = ''; + } else { + push(@rules, $policy.$3); + } } foreach my $rule (@rules) { - if ($rule !~ m{send_destination=}) { - # normalize whitespace a bit - $rule =~ s{\s+}{ }g; + # normalize whitespace a bit so we can report it sensibly: + # typically it will now look like + # <policy context="default"><allow send_destination="com.example.Foo"/> + $rule =~ s{\s+}{ }g; + + if ($rule =~ m{send_} && $rule !~ m{send_destination=}) { tag('dbus-policy-without-send-destination', $file, $rule); } + + if ($rule =~ m{at_console=['"]true}) { + tag('dbus-policy-at-console', $file, $rule); + } } return; diff --git a/t/tests/dbus-policy/debian/etc/dbus-1/system.d/at-console.conf b/t/tests/dbus-policy/debian/etc/dbus-1/system.d/at-console.conf index 06d96c8..8c47adb 100644 --- a/t/tests/dbus-policy/debian/etc/dbus-1/system.d/at-console.conf +++ b/t/tests/dbus-policy/debian/etc/dbus-1/system.d/at-console.conf @@ -4,10 +4,12 @@ <!-- this is OK, at least for now --> <policy group="bluetooth"> <allow send_destination="com.example.Service"/> + <allow send_destination="com.example.Other"/> </policy> <!-- this is deprecated --> <policy at_console="true"> <allow send_destination="com.example.Service"/> + <allow send_destination="com.example.Other"/> </policy> </busconfig> diff --git a/t/tests/dbus-policy/tags b/t/tests/dbus-policy/tags index 0705661..f9028e6 100644 --- a/t/tests/dbus-policy/tags +++ b/t/tests/dbus-policy/tags @@ -1,4 +1,5 @@ -X: dbus-policy: dbus-policy-at-console etc/dbus-1/system.d/at-console.conf -X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <allow send_interface="org.freedesktop.DBus.ObjectManager"/> -X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <allow send_member="AreYouReallySureThisMethodIsAlwaysOK"/> -X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <allow send_path="/com/example/Here"/> +X: dbus-policy: dbus-policy-at-console etc/dbus-1/system.d/at-console.conf <policy at_console="true"><allow send_destination="com.example.Other"/> +X: dbus-policy: dbus-policy-at-console etc/dbus-1/system.d/at-console.conf <policy at_console="true"><allow send_destination="com.example.Service"/> +X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_interface="org.freedesktop.DBus.ObjectManager"/> +X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_member="AreYouReallySureThisMethodIsAlwaysOK"/> +X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_path="/com/example/Here"/> -- 2.1.4
>From dccd58d69ca39f97e6a256869c1aca3184f2dfe6 Mon Sep 17 00:00:00 2001 From: Simon McVittie <s...@debian.org> Date: Wed, 28 Jan 2015 13:36:28 +0000 Subject: [PATCH 3/5] dbus: do not emit dbus-policy-without-send-destination for rules limited to root root is all-powerful anyway, so this is probably OK - and as a practical matter, this cuts down a lot of hits for this Lintian test which are not actually security vulnerabilities in practice. --- checks/dbus.desc | 19 ++++++++++++++++--- checks/dbus.pm | 11 ++++++++++- .../debian/etc/dbus-1/system.d/send-destination.conf | 6 ++++++ t/tests/dbus-policy/tags | 1 + 4 files changed, 33 insertions(+), 4 deletions(-) diff --git a/checks/dbus.desc b/checks/dbus.desc index 42b9665..19befb5 100644 --- a/checks/dbus.desc +++ b/checks/dbus.desc @@ -33,10 +33,10 @@ Experimental: yes Tag: dbus-policy-without-send-destination Severity: normal -Certainty: possible +Certainty: certain Info: The package contains D-Bus policy configuration that uses - one of the <tt>send_*</tt> conditions but does not specify a - <tt>send_destination</tt>. + one of the <tt>send_*</tt> conditions, but does not specify a + <tt>send_destination</tt>, and is not specific to root. . Rules of the form . @@ -52,6 +52,19 @@ Info: The package contains D-Bus policy configuration that uses . are redundant with the system bus' default-deny policy, and have unintended effects on other services. + . + This check ignores rules of the form + . + <policy user="root"> + <allow ... /> + </policy> + . + which are commonly used for the "agent" pattern seen in services like + BlueZ and NetworkManager: a root-privileged daemon calls out to + one or more per-user user interface agent processes with no specific + name, so <tt>send_destination</tt> is not easily applicable. + However, such rules should still be made as specific as possible to + avoid undesired side-effects. Ref: https://bugs.freedesktop.org/show_bug.cgi?id=18961,http://lists.freedesktop.org/archives/dbus/2008-February/009401.html Experimental: yes diff --git a/checks/dbus.pm b/checks/dbus.pm index 2aecae8..18c976b 100644 --- a/checks/dbus.pm +++ b/checks/dbus.pm @@ -91,7 +91,16 @@ sub _check_policy { $rule =~ s{\s+}{ }g; if ($rule =~ m{send_} && $rule !~ m{send_destination=}) { - tag('dbus-policy-without-send-destination', $file, $rule); + # It is about sending but does not specify a send-destination. + # This could be bad. + + if ($rule =~ m{[^>]*user=['"]root['"].*<allow}) { + # skip it: it's probably the "agent" pattern (as seen in + # e.g. BlueZ), and cannot normally be a security flaw + # because root can do anything anyway + } else { + tag('dbus-policy-without-send-destination', $file, $rule); + } } if ($rule =~ m{at_console=['"]true}) { diff --git a/t/tests/dbus-policy/debian/etc/dbus-1/system.d/send-destination.conf b/t/tests/dbus-policy/debian/etc/dbus-1/system.d/send-destination.conf index 6bb7150..ae052ff 100644 --- a/t/tests/dbus-policy/debian/etc/dbus-1/system.d/send-destination.conf +++ b/t/tests/dbus-policy/debian/etc/dbus-1/system.d/send-destination.conf @@ -1,5 +1,11 @@ <!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-BUS Bus Configuration 1.0//EN" "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd"> <busconfig> + <policy user="root"> + <allow send_interface="org.freedesktop.DBus.ObjectManager"/> + </policy> + <policy user="nobody"> + <allow send_interface="org.freedesktop.DBus.ObjectManager"/> + </policy> <policy context="default"> <allow send_interface="org.freedesktop.DBus.ObjectManager"/> <allow send_member="AreYouReallySureThisMethodIsAlwaysOK"/> diff --git a/t/tests/dbus-policy/tags b/t/tests/dbus-policy/tags index f9028e6..2d1d32e 100644 --- a/t/tests/dbus-policy/tags +++ b/t/tests/dbus-policy/tags @@ -3,3 +3,4 @@ X: dbus-policy: dbus-policy-at-console etc/dbus-1/system.d/at-console.conf <poli X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_interface="org.freedesktop.DBus.ObjectManager"/> X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_member="AreYouReallySureThisMethodIsAlwaysOK"/> X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_path="/com/example/Here"/> +X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy user="nobody"><allow send_interface="org.freedesktop.DBus.ObjectManager"/> -- 2.1.4
>From 8241b2fd3d8383b5e609374dbf56b646e9754a21 Mon Sep 17 00:00:00 2001 From: Simon McVittie <s...@debian.org> Date: Wed, 28 Jan 2015 13:44:18 +0000 Subject: [PATCH 4/5] dbus: add a new tag for excessively broad match rules This check has already found CVE-2014-8148 and the recently unembargoed CVE-2014-8156, which I believe are the only ones in current Debian. Hopefully having this as a Lintian check will protect us from this class of vulnerability. --- checks/dbus.desc | 31 +++++++++++++++++++++++++++++++ checks/dbus.pm | 18 ++++++++++++++++++ t/tests/dbus-policy/tags | 2 ++ 3 files changed, 51 insertions(+) diff --git a/checks/dbus.desc b/checks/dbus.desc index 19befb5..c08e3ee 100644 --- a/checks/dbus.desc +++ b/checks/dbus.desc @@ -68,6 +68,37 @@ Info: The package contains D-Bus policy configuration that uses Ref: https://bugs.freedesktop.org/show_bug.cgi?id=18961,http://lists.freedesktop.org/archives/dbus/2008-February/009401.html Experimental: yes +Tag: dbus-policy-excessively-broad +Severity: serious +Certainty: possible +Info: The package contains D-Bus policy configuration that + matches broad classes of messages. This will cause strange side-effects, + is almost certainly unintended, and is a probable security flaw. + . + For instance, + . + <policy user="daemon"> + <allow send_type="method_call"/> + <allow send_destination="com.example.Bees"/> + </policy> + . + in any system bus policy file would allow the <tt>daemon</tt> user to send + any method call to any service, including method calls which are meant to + be restricted to root-only for security, such as + <tt>org.freedesktop.systemd1.Manager.StartTransientUnit</tt>. (In addition, + it allows that user to send any message to the <tt>com.example.Bees</tt> + service.) + . + The intended policy for that particular example was probably more like + . + <policy user="daemon"> + <allow send_type="method_call" send_destination="com.example.Bees"/> + </policy> + . + which correctly allows method calls to that particular service only. +Ref: http://www.openwall.com/lists/oss-security/2015/01/27/25 +Experimental: yes + Tag: dbus-session-service-wrong-name Severity: wishlist Certainty: certain diff --git a/checks/dbus.pm b/checks/dbus.pm index 18c976b..e56d753 100644 --- a/checks/dbus.pm +++ b/checks/dbus.pm @@ -61,6 +61,8 @@ sub run { return; } +my $PROPERTIES = 'org.freedesktop.DBus.Properties'; + sub _check_policy { my ($file) = @_; @@ -100,6 +102,22 @@ sub _check_policy { # because root can do anything anyway } else { tag('dbus-policy-without-send-destination', $file, $rule); + + if ( $rule =~ m{send_interface=} + && $rule !~ m{send_interface=['"]\Q${PROPERTIES}\E['"]}) { + # That's undesirable, because it opens up communication + # with arbitrary services and can undo DoS mitigation + # efforts; but at least it's specific to an interface + # other than o.fd.DBus.Properties, so all that should + # happen is that the service sends back an error message. + # + # Properties doesn't count as an effective limitation, + # because it's a sort of meta-interface. + } elsif ($rule =~ m{<allow}) { + # Looks like CVE-2014-8148 or similar. This is really bad; + # emit an additional tag. + tag('dbus-policy-excessively-broad', $file, $rule); + } } } diff --git a/t/tests/dbus-policy/tags b/t/tests/dbus-policy/tags index 2d1d32e..8b40f8a 100644 --- a/t/tests/dbus-policy/tags +++ b/t/tests/dbus-policy/tags @@ -1,5 +1,7 @@ X: dbus-policy: dbus-policy-at-console etc/dbus-1/system.d/at-console.conf <policy at_console="true"><allow send_destination="com.example.Other"/> X: dbus-policy: dbus-policy-at-console etc/dbus-1/system.d/at-console.conf <policy at_console="true"><allow send_destination="com.example.Service"/> +X: dbus-policy: dbus-policy-excessively-broad etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_member="AreYouReallySureThisMethodIsAlwaysOK"/> +X: dbus-policy: dbus-policy-excessively-broad etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_path="/com/example/Here"/> X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_interface="org.freedesktop.DBus.ObjectManager"/> X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_member="AreYouReallySureThisMethodIsAlwaysOK"/> X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_path="/com/example/Here"/> -- 2.1.4
>From 8104d50ee6dfcae73062ba89140882462aeff901 Mon Sep 17 00:00:00 2001 From: Simon McVittie <s...@debian.org> Date: Wed, 28 Jan 2015 13:53:43 +0000 Subject: [PATCH 5/5] Mark D-Bus checks as non-experimental They've found two unrelated CVEs, I think the experiment was a success :-) --- checks/dbus.desc | 5 ----- t/tests/dbus-policy/tags | 16 ++++++++-------- t/tests/dbus-services/tags | 4 ++-- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/checks/dbus.desc b/checks/dbus.desc index c08e3ee..2066f3d 100644 --- a/checks/dbus.desc +++ b/checks/dbus.desc @@ -29,7 +29,6 @@ Info: The package contains D-Bus policy configuration that uses the including distinguishing between users who are "at the console" and those who are not. Ref: https://bugs.freedesktop.org/show_bug.cgi?id=39611 -Experimental: yes Tag: dbus-policy-without-send-destination Severity: normal @@ -66,7 +65,6 @@ Info: The package contains D-Bus policy configuration that uses However, such rules should still be made as specific as possible to avoid undesired side-effects. Ref: https://bugs.freedesktop.org/show_bug.cgi?id=18961,http://lists.freedesktop.org/archives/dbus/2008-February/009401.html -Experimental: yes Tag: dbus-policy-excessively-broad Severity: serious @@ -97,7 +95,6 @@ Info: The package contains D-Bus policy configuration that . which correctly allows method calls to that particular service only. Ref: http://www.openwall.com/lists/oss-security/2015/01/27/25 -Experimental: yes Tag: dbus-session-service-wrong-name Severity: wishlist @@ -113,7 +110,6 @@ Info: The package contains a D-Bus session service whose filename name is <tt>com.example.MyService1</tt>, and it should be service-activatable, you should achieve that by packaging <tt>/usr/share/dbus-1/services/com.example.MyService1.service</tt>. -Experimental: yes Tag: dbus-system-service-wrong-name Severity: serious @@ -128,4 +124,3 @@ Info: The package contains a D-Bus system service whose filename <tt>com.example.MyService1</tt>, and it should be service-activatable, you must provide <tt>/usr/share/dbus-1/system-services/com.example.MyService1.service</tt>. -Experimental: yes diff --git a/t/tests/dbus-policy/tags b/t/tests/dbus-policy/tags index 8b40f8a..1f65e49 100644 --- a/t/tests/dbus-policy/tags +++ b/t/tests/dbus-policy/tags @@ -1,8 +1,8 @@ -X: dbus-policy: dbus-policy-at-console etc/dbus-1/system.d/at-console.conf <policy at_console="true"><allow send_destination="com.example.Other"/> -X: dbus-policy: dbus-policy-at-console etc/dbus-1/system.d/at-console.conf <policy at_console="true"><allow send_destination="com.example.Service"/> -X: dbus-policy: dbus-policy-excessively-broad etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_member="AreYouReallySureThisMethodIsAlwaysOK"/> -X: dbus-policy: dbus-policy-excessively-broad etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_path="/com/example/Here"/> -X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_interface="org.freedesktop.DBus.ObjectManager"/> -X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_member="AreYouReallySureThisMethodIsAlwaysOK"/> -X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_path="/com/example/Here"/> -X: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy user="nobody"><allow send_interface="org.freedesktop.DBus.ObjectManager"/> +E: dbus-policy: dbus-policy-excessively-broad etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_member="AreYouReallySureThisMethodIsAlwaysOK"/> +E: dbus-policy: dbus-policy-excessively-broad etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_path="/com/example/Here"/> +W: dbus-policy: dbus-policy-at-console etc/dbus-1/system.d/at-console.conf <policy at_console="true"><allow send_destination="com.example.Other"/> +W: dbus-policy: dbus-policy-at-console etc/dbus-1/system.d/at-console.conf <policy at_console="true"><allow send_destination="com.example.Service"/> +W: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_interface="org.freedesktop.DBus.ObjectManager"/> +W: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_member="AreYouReallySureThisMethodIsAlwaysOK"/> +W: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy context="default"><allow send_path="/com/example/Here"/> +W: dbus-policy: dbus-policy-without-send-destination etc/dbus-1/system.d/send-destination.conf <policy user="nobody"><allow send_interface="org.freedesktop.DBus.ObjectManager"/> diff --git a/t/tests/dbus-services/tags b/t/tests/dbus-services/tags index 217b153..9802a46 100644 --- a/t/tests/dbus-services/tags +++ b/t/tests/dbus-services/tags @@ -1,2 +1,2 @@ -X: dbus-services: dbus-session-service-wrong-name org.gtk.vfs.Daemon.service usr/share/dbus-1/services/gvfs-daemon.service -X: dbus-services: dbus-system-service-wrong-name com.example.SystemDaemon1.service usr/share/dbus-1/system-services/this-name-cannot-work.service +E: dbus-services: dbus-system-service-wrong-name com.example.SystemDaemon1.service usr/share/dbus-1/system-services/this-name-cannot-work.service +I: dbus-services: dbus-session-service-wrong-name org.gtk.vfs.Daemon.service usr/share/dbus-1/services/gvfs-daemon.service -- 2.1.4