|
Description
Simon McVittie
2014-10-16 19:05:15 UTC
Created attachment 107944 [details] [review] Set fd rlimit to 64k for the system dbus-daemon Restore the original rlimit for activated services. --- Yeah I know this needs a longer commit message. Ideas? I used a hard-coded 64k fds rather than trying to calculate what we need, as Lennart suggested. If several uids collaborate to use up our fd limit then we lose anyway, afaics. I'm not sure to what extent it's worth handling this as a vulnerability, but I'm giving it the benefit of the doubt. Debian with sysvinit or Upstart (but not systemd) as pid 1 also needs this patch, to start dbus-daemon with enough privileges that it can get the higher rlimit. --- a/debian/dbus.init +++ b/debian/dbus.init @@ -69,7 +69,7 @@ start_it_up() log_daemon_msg "Starting $DESC" "$NAME" start-stop-daemon --start --quiet --pidfile $PIDFILE \ - --user $DAEMONUSER --exec $DAEMON -- --system $PARAMS + --exec $DAEMON -- --system $PARAMS log_end_msg $? } Patch looks good in principle, but I probably wouldn't make it fatal if bumping the limit doesn't work... BTW, the code that in systemd tries to make the best of the situation when bumping is here: http://cgit.freedesktop.org/systemd/systemd/tree/src/shared/util.c#n5185 (In reply to Lennart Poettering from comment #4) > Patch looks good in principle, but I probably wouldn't make it fatal if > bumping the limit doesn't work... Where do you see it being fatal? My intention was that the library code raises a DBusError, and the dbus-daemon catches the DBusError, logs it to syslog and carries on as if nothing had happened. The only place where it's fatal is if we fail to put the old limit back before activating a child service (without using systemd), in which case it's fatal to the child service. I think that's correct: we shouldn't start children with elevated fd-limits. The one possible improvement I can see in that area is that raise_file_descriptor_limit() should probably clear context->initial_fd_limit if _dbus_rlimit_raise_fd_limit_if_privileged() fails, so that we can only call setrlimit() from child_setup() if a previous setrlimit() in _dbus_rlimit_raise_fd_limit_if_privileged() has already succeeded. We should maybe also add a log-level WARNING, corresponding to syslog WARNING, above the current INFO (which is, confusingly, syslog NOTICE). (In reply to Simon McVittie from comment #5) > (In reply to Lennart Poettering from comment #4) > > Patch looks good in principle, but I probably wouldn't make it fatal if > > bumping the limit doesn't work... > > Where do you see it being fatal? My intention was that the library code > raises a DBusError, and the dbus-daemon catches the DBusError, logs it to > syslog and carries on as if nothing had happened. > > The only place where it's fatal is if we fail to put the old limit back > before activating a child service (without using systemd), in which case > it's fatal to the child service. I think that's correct: we shouldn't start > children with elevated fd-limits. yeah this is the occasion i meant. I'd still not make this fatal though. Limits are just, well, limits... and if they are higher than necessary that's certainly much less problematic then the other way round... Created attachment 108299 [details] [review] [PATCH] Set fd rlimit to 64k for the system dbus-daemon (In reply to Lennart Poettering from comment #6) > (In reply to Simon McVittie from comment #5) > > The only place where it's fatal is if we fail to put the old limit back > > before activating a child service (without using systemd), in which case > > it's fatal to the child service. I think that's correct: we shouldn't start > > children with elevated fd-limits. > > yeah this is the occasion i meant. I'd still not make this fatal though. > Limits are just, well, limits... and if they are higher than necessary > that's certainly much less problematic then the other way round... Here is Simon's patch edited to make it not fatal. I deleted the exit(). Comment on attachment 108299 [details] [review] [PATCH] Set fd rlimit to 64k for the system dbus-daemon Review of attachment 108299 [details] [review]: ----------------------------------------------------------------- Your change seems good. ::: bus/activation.c @@ +1702,5 @@ > + !_dbus_rlimit_restore_fd_limit (initial_fd_limit, &error)) > + { > + /* unfortunately we don't actually know the service name here */ > + bus_context_log (activation->context, > + DBUS_SYSTEM_LOG_INFO, This should probably be LOG_WARNING, if we had that, which we don't. Anyone want to write a patch? ::: bus/bus.c @@ +670,5 @@ > + > + if (context->initial_fd_limit == NULL) > + { > + bus_context_log (context, DBUS_SYSTEM_LOG_INFO, > + "%s: %s", error.name, error.message); This too, at least #ifdef DBUS_UNIX? (getrlimit/setrlimit are POSIX.1-2001, so I would regard Unixes without that functionality as being somewhat broken, even if we do our best to keep them working.) Maybe this whole function should be #ifdef DBUS_UNIX. child_setup() won't be called on Windows anyway. @@ +684,5 @@ > + * anything short of multiple uids conspiring against us. > + */ > + if (!_dbus_rlimit_raise_fd_limit_if_privileged (65536, &error)) > + { > + bus_context_log (context, DBUS_SYSTEM_LOG_INFO, And this. FWIW, I tested this on a dbus 1.8.0 system and got the following /proc/*/limits stats: * Without the patch: dbus-daemon (for the system bus) Max open files 2144 2144 files * Without the patch: a system service activated by dbus-daemon (not by systemd) Max open files 2144 2144 files * With the patch: dbus-daemon (for the system bus) Max open files 65536 65536 files * With the patch: a system service activated by dbus-daemon (not by systemd) Max open files 1024 4096 files This test results look good to me. Created attachment 108848 [details] [review] [PATCH 1/2] DBusSystemLogSeverity: add DBUS_SYSTEM_LOG_WARNING (In reply to Simon McVittie from comment #8) > This should probably be LOG_WARNING, if we had that, which we don't. Anyone > want to write a patch? This patch adds LOG_WARNING. Created attachment 108849 [details] [review] [PATCH 2/2] Set fd rlimit to 64k for the system dbus-daemon Fixes after review from Comment #8: - LOG_WARNING - #ifdef DBUS_UNIX - add bugzilla link in commit log Comment on attachment 108848 [details] [review] [PATCH 1/2] DBusSystemLogSeverity: add DBUS_SYSTEM_LOG_WARNING Review of attachment 108848 [details] [review]: ----------------------------------------------------------------- Looks good Comment on attachment 108849 [details] [review] [PATCH 2/2] Set fd rlimit to 64k for the system dbus-daemon Review of attachment 108849 [details] [review]: ----------------------------------------------------------------- Looks good, but... ::: bus/activation.c @@ +1689,5 @@ > } > > +static void > +child_setup (void *user_data) > +{ I was expecting the body of this function to be in #ifdef DBUS_UNIX too? (It isn't executed on Windows anyway.) ::: bus/bus.c @@ +657,4 @@ > return FALSE; > } > > +#ifdef DBUS_UNIX If you move this inside the body, so the function exists but is a no-op on Windows, then the call to it does not need to be #ifdef'd (and will be inlined). We use that trick a lot. Created attachment 108853 [details] [review] [PATCH 2/2] Set fd rlimit to 64k for the system dbus-daemon (In reply to Simon McVittie from comment #13) > Comment on attachment 108849 [details] [review] [review] > [PATCH 2/2] Set fd rlimit to 64k for the system dbus-daemon > > Review of attachment 108849 [details] [review] [review]: > ----------------------------------------------------------------- > > Looks good, but... > > ::: bus/activation.c > @@ +1689,5 @@ > > } > > > > +static void > > +child_setup (void *user_data) > > +{ > > I was expecting the body of this function to be in #ifdef DBUS_UNIX too? (It > isn't executed on Windows anyway.) It is executed on Windows from a thread. But since its content is Unix-specific, I added the #ifdef. > ::: bus/bus.c > @@ +657,4 @@ > > return FALSE; > > } > > > > +#ifdef DBUS_UNIX > > If you move this inside the body, so the function exists but is a no-op on > Windows, then the call to it does not need to be #ifdef'd (and will be > inlined). We use that trick a lot. Ok, done. Comment on attachment 108853 [details] [review] [PATCH 2/2] Set fd rlimit to 64k for the system dbus-daemon Review of attachment 108853 [details] [review]: ----------------------------------------------------------------- ::: bus/bus.c @@ +702,4 @@ > DBusHashTable *service_context_table; > DBusList *watched_dirs = NULL; > > +#ifdef DBUS_UNIX This #ifdef is no longer necessary, and will provoke compiler warnings on Windows for the unused function raise_file_descriptor_limit(). Created attachment 108886 [details] [review] [PATCH 2/2] Set fd rlimit to 64k for the system dbus-daemon Restore the original rlimit for activated services. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=85105 Reviewed-by: Lennart Poettering [Alban Crequy: make failure to restore original rlimit non-fatal] Reviewed-by: Simon McVittie [Alban Crequy: use new DBUS_SYSTEM_LOG_WARNING; skip if !DBUS_UNIX] [Simon McVittie: adjust DBUS_UNIX, add stub _dbus_rlimit_free on Windows] Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk> --- Two changes since Attachment #108853 [details]: * what I said in my last comment * dbus-sysdeps-util-win.c needs an implementation of _dbus_rlimit_free(), although it can be a trivial stub: it is not actually called, and it would be a bug for it to have a non-NULL argument (In reply to Simon McVittie from comment #16) > [Simon McVittie: adjust DBUS_UNIX, add stub _dbus_rlimit_free on Windows] I successfully cross-compiled this for Windows under mingw. I haven't tested the result, but if there's a problem, Windows users could skip this release anyway - this change is irrelevant for Windows. (In reply to Simon McVittie from comment #16) > Created attachment 108886 [details] [review] [review] > [PATCH 2/2] Set fd rlimit to 64k for the system dbus-daemon > > Restore the original rlimit for activated services. > > Bug: https://bugs.freedesktop.org/show_bug.cgi?id=85105 > Reviewed-by: Lennart Poettering > [Alban Crequy: make failure to restore original rlimit non-fatal] > Reviewed-by: Simon McVittie > [Alban Crequy: use new DBUS_SYSTEM_LOG_WARNING; skip if !DBUS_UNIX] > [Simon McVittie: adjust DBUS_UNIX, add stub _dbus_rlimit_free on Windows] > Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk> > > --- > > Two changes since Attachment #108853 [details]: > > * what I said in my last comment > > * dbus-sysdeps-util-win.c needs an implementation of _dbus_rlimit_free(), > although it can be a trivial stub: it is not actually called, and > it would be a bug for it to have a non-NULL argument This new version looks good, except that we forgot to remove this prototype: dbus/dbus-sysdeps.h:548:void _dbus_request_file_descriptor_limit (unsigned int limit); Do you plan to backport the patch to the 1.8 branch? If so, should we use DBUS_SYSTEM_LOG_INFO or backport the _WARNING patch too? (In reply to Alban Crequy from comment #18) > This new version looks good, except that we forgot to remove this prototype: > > dbus/dbus-sysdeps.h:548:void _dbus_request_file_descriptor_limit (unsigned > int limit); Will do. > Do you plan to backport the patch to the 1.8 branch? If so, should we use > DBUS_SYSTEM_LOG_INFO or backport the _WARNING patch too? I was going to backport the _WARNING patch too (there is no actual backporting involved, it works as-is) but now you mention it, yes we'd be better off with a self-contained patch. We can s/WARNING/INFO/ in 1.9. Created attachment 108896 [details] [review] [PATCH] Set fd rlimit to 64k for the system dbus-daemon Restore the original rlimit for activated services. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=85105 Reviewed-by: Lennart Poettering [Alban Crequy: make failure to restore original rlimit non-fatal] Reviewed-by: Simon McVittie [Alban Crequy: use new DBUS_SYSTEM_LOG_WARNING; skip if !DBUS_UNIX] [Simon McVittie: adjust DBUS_UNIX, add stub _dbus_rlimit_free on Windows] Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk> --- Now self-contained for 1.8. Created attachment 108897 [details] [review] [PATCH 2/3 for 1.9] DBusSystemLogSeverity: add DBUS_SYSTEM_LOG_WARNING From: Alban Crequy <alban.crequy@collabora.co.uk> Created attachment 108898 [details] [review] [PATCH 3/3 for 1.9] Use DBUS_SYSTEM_LOG_WARNING for failure to alter fd-limits > [Alban Crequy: use new DBUS_SYSTEM_LOG_WARNING; skip if !DBUS_UNIX]
Alban pointed out that this line is no longer true. I'm going to do a new version of that commit message when I have a CVE ID.
The patches in comment #20 to comment #22 look good. Kurt Seifried gave us a CVE ID. Proposed advisory text below. ---- CVE: CVE-2014-7824 Tracked as: https://bugs.freedesktop.org/show_bug.cgi?id=85105 Impact: local denial of service Access required: local Versions believed to be vulnerable: dbus >= 1.3.0 Credit: discovered by Simon McVittie at Collabora Ltd. D-Bus <http://www.freedesktop.org/wiki/Software/dbus/> is an asynchronous inter-process communication system, commonly used for system services or within a desktop session on Linux and other operating systems. The patch issued by the D-Bus maintainers for CVE-2014-3636 was based on incorrect reasoning, and does not fully prevent the attack described as "CVE-2014-3636 part A", which is repeated below. Preventing that attack requires raising the system dbus-daemon's RLIMIT_NOFILE (ulimit -n) to a higher value. CVE-2014-7824 has been allocated for this vulnerability. To avoid propagating that higher limit to activatable system services, it is desirable to start the system dbus-daemon as root so it can store its previous limit, raise its limit, drop root privileges (which its default configuration will do automatically), and restore the previous limit before launching activatable services. Some operating system distributions, such as anything using the upstream-supplied systemd units, start the system dbus-daemon as root already; others, such as Debian 7, currently start the system dbus-daemon under its less privileged uid and will need minor modifications to their init scripts. Attack details (repeating CVE-2014-3636 part A): By queuing up the maximum allowed number of fds, a malicious sender could reach the system dbus-daemon's RLIMIT_NOFILE (ulimit -n, typically 1024 on Linux). This would act as a denial of service in two ways: * new clients would be unable to connect to the dbus-daemon * when receiving a subsequent message from a non-malicious client that contained a fd, dbus-daemon would receive the MSG_CTRUNC flag, indicating that the list of fds was truncated; kernel fd-passing APIs do not provide any way to recover from that, so dbus-daemon responds to MSG_CTRUNC by disconnecting the sender, causing denial of service to that sender Created attachment 109054 [details] [review] CVE-2014-7824: set fd rlimit to 64k for the system dbus-daemon This ensures that our rlimit is actually high enough to avoid the denial of service described in CVE-2014-3636 part A. CVE-2014-7824 has been allocated for this incomplete fix. Restore the original rlimit for activated services, to avoid them getting undesired higher limits. (Thanks to Alban Crequy for various adjustments which have been included in this commit.) Bug: https://bugs.freedesktop.org/show_bug.cgi?id=85105 Reviewed-by: Alban Crequy <alban.crequy@collabora.co.uk> --- Same content as Attachment #108896 [details], but a better commit message. unembargo Fixed in 1.6.26, 1.8.10, 1.9.2. Older branches are not supported by the D-Bus maintainers, but distribution maintainers who are still stuck on those versions are invited to use the upstream dbus-1.4, etc., branches to share backported patches. |
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.