|
Description
Simon McVittie
2015-01-26 20:50:19 UTC
Created attachment 112849 [details] [review] [PATCH 1/4] bus_driver_get_owner_of_name: factor out from bus_driver_get_conn_helper --- Part of a "nice" code fix for master. Opinions welcome on whether I should backport this to 1.8; I'm certainly not going back as far as 1.6. Created attachment 112850 [details] [review] [PATCH 2/4] Prevent a local attacker from making systemd activation appear to fail --- This is the actual code fix. Requires patch 1. Created attachment 112851 [details] [review] [master only] Add a regression test for making systemd activation appear to fail --- Requires Bug #57952 fixed. Certainly not going to be backported. Created attachment 112852 [details] [review] [PATCH 4/4] Prevent forged ActivationFailure messages from non-root processes This is redundant with the code fix but is likely to be easier to backport. Proof of concept (it's easier to demonstrate with a service whose activation succeeds or fails slowly): % cat /usr/share/dbus-1/system-services/com.example.Slow.service [D-BUS Service] Name=com.example.Slow Exec=/bin/sleep 60 User=root SystemdService=dbus-com.example.Slow.service % cat /etc/systemd/system/dbus-com.example.Slow.service [Unit] Description=Slow service [Service] Type=dbus BusName=com.example.Slow ExecStart=/bin/sleep 60 % dbus-send --system --dest=com.example.Slow / org.freedesktop.DBus.Peer.Ping & % dbus-send --system --type=signal --dest=org.freedesktop.DBus /org/freedesktop/systemd1 org.freedesktop.systemd1.Activator.ActivationFailure string:dbus-com.example.Slow.service string:com.example.Badness string:"This is bad" "Good" result: Forged ActivationFailure is rejected with a syslog message. Activation of com.example.Slow times out after 60 seconds. "Bad" result: Forged ActivationFailure is received and processed. The syslog message from dbus-daemon says "This is bad". I'll ask the distros@vs list for a CVE ID when someone has ack'd these patches. Not screamingly urgent because it's only local DoS, but should be fixed. (In reply to Simon McVittie from comment #6) > I'll ask the distros@vs list for a CVE ID when someone has ack'd these > patches. now done anyway Sorry for the slow response. I am at the GNOME developer experience/docs hackfest this week, so I will not have much to look at these until next week. I had a (very) cursory look and they seem OK. This is CVE-2015-0245 Created attachment 112911 [details] [review] [1.8] CVE-2015-0245: prevent forged ActivationFailure from non-root processes Without either this rule or better checking in dbus-daemon, non-systemd processes can make dbus-daemon think systemd failed to activate a system service, resulting in an error reply back to the requester. This is redundant with the fix in the C code (which I consider to be the real solution), but is likely to be easier to backport. --- Better commit message, now applicable to 1.8 without patch conflicts, no functional changes. This is the only one I am considering backporting to 1.6. Created attachment 112912 [details] [review] [1.8] bus_driver_get_owner_of_name: factor out from bus_driver_get_conn_helper We need this, or something equivalent, to address CVE-2015-0245 via code changes. --- Clearer commit message, diff is relative to 1.8, no functional changes. Created attachment 112913 [details] [review] [1.8] CVE-2015-0245: discard forged ActivationFailure messages Without this code change, non-systemd processes can make dbus-daemon think systemd failed to activate a system service, resulting in an error reply back to the requester. In practice we can address this in system.conf by only allowing root to forge these messages, but this check is the real solution, particularly on systems where root is not all-powerful. --- Better commit message, diff is relative to 1.8, no functional changes. Comment on attachment 112851 [details] [review] [master only] Add a regression test for making systemd activation appear to fail Renaming patch to indicate it is for master only (additionally, it requires part of Bug #88810, and Bug #59752). (In reply to Simon McVittie from comment #10) > [1.8] CVE-2015-0245: prevent forged ActivationFailure from non-root > processes This is going to conflict when applied to master, because master added the Stats interface... but fixing the conflicts is not difficult, and anyone who really cares about security should be running (1.6 or) 1.8 anyway, so the 1.8 patches are better ones to release to distros. I'll do the merge into master and release a new 1.9 version on unembargo day (which is currently undecided; if people review things somewhat soon, then this time next week seems reasonable). Comment on attachment 112912 [details] [review] [1.8] bus_driver_get_owner_of_name: factor out from bus_driver_get_conn_helper Review of attachment 112912 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 112913 [details] [review] [1.8] CVE-2015-0245: discard forged ActivationFailure messages Review of attachment 112913 [details] [review]: ----------------------------------------------------------------- The code looks good to me, though I’m not familiar enough with D-Bus internals to do a security review on this (or other patches on this bug). (In reply to Simon McVittie from comment #1) > Part of a "nice" code fix for master. Opinions welcome on whether I should > backport this to 1.8; I'm certainly not going back as far as 1.6. I'm currently leaning towards using Attachment #112911 [details] as the only fix for both 1.6.30 and 1.8.16, and applying the C-code fixes that Philip just reviewed (which are the proper fix for this, IMO) to only master for now. I can always backport them for 1.8.18 if desired, but it would be easier to do that in public once the embargo has lifted. I would really appreciate review on these from another upstream D-Bus maintainer, but I don't want to sit on this vuln forever; so I intend to contact distros again this time tomorrow, setting 2015-02-05 15:00 UTC (3 days from now) as unembargo day, unless someone tells me not to. I have not tested the patches but they all look good to me. Comment on attachment 112911 [details] [review] [1.8] CVE-2015-0245: prevent forged ActivationFailure from non-root processes Review of attachment 112911 [details] [review]: ----------------------------------------------------------------- Looks like a safe and reasonable hardening of policy. Comment on attachment 112912 [details] [review] [1.8] bus_driver_get_owner_of_name: factor out from bus_driver_get_conn_helper Review of attachment 112912 [details] [review]: ----------------------------------------------------------------- Seems like a reasonable bit of refactoring, especially getting rid of the assert. Comment on attachment 112913 [details] [review] [1.8] CVE-2015-0245: discard forged ActivationFailure messages Review of attachment 112913 [details] [review]: ----------------------------------------------------------------- The discarding and logging seems fine to me, although I have a similar level of familiarity with the D-Bus internals as Philip. (In reply to Simon McVittie from comment #17) > I intend to > contact distros again this time tomorrow, setting 2015-02-05 15:00 UTC (3 > days from now) as unembargo day, unless someone tells me not to. Well this clearly didn't happen. Now intending to sort out the 1.8.16 and possibly 1.9.10 releases tomorrow, for unembargo on Monday. (In reply to Simon McVittie from comment #22) > Well this clearly didn't happen. Now intending to sort out the 1.8.16 and > possibly 1.9.10 releases tomorrow, for unembargo on Monday. Unembargo o'clock is 15:00 UTC on Monday. I have 1.8.16 and 1.6.30 ready for upload; 1.9.10 will follow afterwards (so it isn't too out of sync with the current state of master at the time). Attachment #112911 [details] is the only thing applied to the stable branches right now; the others will initially only go in master, although I might do a 1.8.18 with Attachment #112912 [details] and Attachment #112913 [details] when the dust has settled. Fixed in 1.8.16, 1.6.30, 1.9.10. Unembargoing. |
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.