| Summary: | potentially uses rand() where a cryptographically strong PRNG is required | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Simon McVittie <smcv> |
| Component: | core | Assignee: | Simon McVittie <smcv> |
| Status: | RESOLVED FIXED | QA Contact: | D-Bus Maintainers <dbus> |
| Severity: | normal | ||
| Priority: | medium | CC: | thiago, walters |
| Version: | git master | Keywords: | patch, security |
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
[master, 1.8] Security hardening: force EXTERNAL auth in session.conf on Unix
_dbus_server_new_for_socket: raise a DBusError _dbus_server_init_base: raise a DBusError Make UUID generation failable Fail to generate random bytes instead of falling back to rand() |
||
|
Description
Simon McVittie
2015-05-12 09:41:39 UTC
I think the right way to fix this is to make _dbus_generate_random_bytes[_buffer]() just fail if it cannot generate cryptographically random bytes.
Unfortunately, this involves adding a lot of DBusError parameters to functions that could previously only fail under OOM conditions, and adding error-handling to a couple of functions that could previously not fail at all. So I think this approach is only suitable for the 1.9 branch - it's just too intrusive for 1.8.
For the 1.8 branch, I think a better approach is likely to be to add <auth>EXTERNAL</auth> to the default session.conf on all non-Windows platforms.
This will require action from anyone who is relying on the tcp: or nonce-tcp: transports on Unix (undoing that change), but those transports already require special configuration, and are not secure anyway (unless you use a VPN or something).
It will also require action from anyone on a Unix where we do not know how to pass credentials, but I'm not convinced that such platforms really have serious users: we support credentials-passing on Linux, {Free,Net,Open,Dragonfly}BSD, Solaris, anything with getpeereid(), and apparently even semi-recent GNU/Hurd.
Created attachment 115714 [details] [review] [master, 1.8] Security hardening: force EXTERNAL auth in session.conf on Unix DBUS_COOKIE_SHA1 is dependent on unguessable strings, i.e. indirectly dependent on high-quality pseudo-random numbers whereas EXTERNAL authentication (credentials-passing) is mediated by the kernel and cannot be faked. On Windows, EXTERNAL authentication is not available, so we continue to use the hard-coded default (all authentication mechanisms are tried). Users of tcp: or nonce-tcp: on Unix will have to comment this out, but they would have had to use a special configuration anyway (to set the listening address), and the tcp: and nonce-tcp: transports are inherently insecure unless special steps are taken to have them restricted to a VPN or SSH tunnelling. Users of obscure Unix platforms (those that trigger the warning "Socket credentials not supported on this Unix OS" when compiling dbus-sysdeps-unix.c) might also have to comment this out, or preferably provide a tested patch to enable credentials-passing on that OS. Created attachment 115715 [details] [review] _dbus_server_new_for_socket: raise a DBusError This can currently only fail due to OOM, but I'm about to make it possible to fail for other reasons. Created attachment 115716 [details] [review] _dbus_server_init_base: raise a DBusError This can currently only fail from OOM, but I'm about to make it possible to fail from insufficient entropy. Created attachment 115717 [details] [review] Make UUID generation failable Previously, this would always succeed, but might use weak random numbers in rare failure cases. I don't think these UUIDs are security-sensitive, but if they're generated by a PRNG as weak as rand() (<= 32 bits of entropy), we certainly can't claim that they're universally unique. Created attachment 115718 [details] [review] Fail to generate random bytes instead of falling back to rand() This is more robust against broken setups where we run out of memory or cannot read /dev/urandom. I would very much appreciate code review for this. I'd like to do a 1.8.18 and 1.9.16 somewhat soon with this fixed. My inclination is to treat this as security hardening (i.e. mitigating a class of potential flaws, rather than fixing a specific known flaw), because we are not aware of any way in which an attacker could actually cause the bad code path to be taken. For 1.8 I'm just planning to do the <auth>EXTERNAL</auth> bit, which makes any flaws in DBUS_COOKIE_SHA1 irrelevant for the Unix session bus; for 1.9 I would like to land the whole set of patches. As such, I'm not intending to go through the whole CVE-ID/vendor-sec/coordinated-release dance. But I'm keeping this bug private for the moment, in case other maintainers object to that approach; if there is no particular feedback either way, I will make it public when I do the 1.8.18 release. Comment on attachment 115714 [details] [review] [master, 1.8] Security hardening: force EXTERNAL auth in session.conf on Unix Review of attachment 115714 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 115715 [details] [review] _dbus_server_new_for_socket: raise a DBusError Review of attachment 115715 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-server-socket.c @@ +283,5 @@ > _dbus_server_new_for_socket (int *fds, > int n_fds, > const DBusString *address, > + DBusNonceFile *noncefile, > + DBusError *error) The parameter error is not documented. @@ +288,3 @@ > { > DBusServerSocket *socket_server; > DBusServer *server; The code fragment following this lines do not set the error parameter in failure case. int i; socket_server = dbus_new0 (DBusServerSocket, 1); if (socket_server == NULL) return NULL; Comment on attachment 115716 [details] [review] _dbus_server_init_base: raise a DBusError Review of attachment 115716 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-server.c @@ +110,5 @@ > dbus_bool_t > _dbus_server_init_base (DBusServer *server, > const DBusServerVTable *vtable, > + const DBusString *address, > + DBusError *error) The parameter error is not documentated. Comment on attachment 115717 [details] [review] Make UUID generation failable Review of attachment 115717 [details] [review]: ----------------------------------------------------------------- looks good except the missing parameter documentation. ::: dbus/dbus-internals.c @@ +648,5 @@ > * @param uuid the uuid to initialize > */ > +dbus_bool_t > +_dbus_generate_uuid (DBusGUID *uuid, > + DBusError *error) The parameter error is not documentated. @@ +866,5 @@ > * @returns #FALSE if no memory > */ > dbus_bool_t > +_dbus_get_local_machine_uuid_encoded (DBusString *uuid_str, > + DBusError *error) dito ::: dbus/dbus-uuidgen.c @@ +116,4 @@ > */ > dbus_bool_t > +_dbus_create_uuid (char **uuid_p, > + DBusError *error) parameter error is not documentated Comment on attachment 115718 [details] [review] Fail to generate random bytes instead of falling back to rand() Review of attachment 115718 [details] [review]: ----------------------------------------------------------------- looks good except the missing parameter documentation. ::: dbus/dbus-sysdeps.c @@ +512,5 @@ > */ > +dbus_bool_t > +_dbus_generate_random_bytes_buffer (char *buffer, > + int n_bytes, > + DBusError *error) parameter error is not documentated @@ +545,5 @@ > */ > dbus_bool_t > _dbus_generate_random_ascii (DBusString *str, > + int n_bytes, > + DBusError *error) dito (In reply to Ralf Habacker from comment #10) > Comment on attachment 115716 [details] [review] [review] > _dbus_server_init_base: raise a DBusError > > Review of attachment 115716 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-server.c > @@ +110,5 @@ > > dbus_bool_t > > _dbus_server_init_base (DBusServer *server, > > const DBusServerVTable *vtable, > > + const DBusString *address, > > + DBusError *error) > > The parameter error is not documentated. otherwise looks good. (In reply to Ralf Habacker from comment #9) > The code fragment following this lines do not set the error parameter in > failure case. > > int i; > > socket_server = dbus_new0 (DBusServerSocket, 1); > if (socket_server == NULL) > return NULL; Good catch! I've made it "goto failed_0", which frees socket_server (harmless, it's NULL after all), and sets the OOM error if necessary. (In reply to Ralf Habacker from comment #9) > The parameter error is not documented. Anyone who needs documentation for this parameter should not be modifying dbus. :-) But, sure, since I'm modifying it anyway, I've revised those. Preparing releases now. Fixed and released in 1.8.18. Fixed in git for 1.9.16, release in progress. |
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.