Bug 90414 - potentially uses rand() where a cryptographically strong PRNG is required
Summary: potentially uses rand() where a cryptographically strong PRNG is required
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords: patch, security
Depends on:
Blocks:
 
Reported: 2015-05-12 09:41 UTC by Simon McVittie
Modified: 2015-05-14 13:49 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[master, 1.8] Security hardening: force EXTERNAL auth in session.conf on Unix (3.90 KB, patch)
2015-05-12 11:51 UTC, Simon McVittie
Details | Splinter Review
_dbus_server_new_for_socket: raise a DBusError (4.26 KB, patch)
2015-05-12 11:52 UTC, Simon McVittie
Details | Splinter Review
_dbus_server_init_base: raise a DBusError (4.75 KB, patch)
2015-05-12 11:52 UTC, Simon McVittie
Details | Splinter Review
Make UUID generation failable (11.41 KB, patch)
2015-05-12 11:52 UTC, Simon McVittie
Details | Splinter Review
Fail to generate random bytes instead of falling back to rand() (17.91 KB, patch)
2015-05-12 11:53 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2015-05-12 09:41:39 UTC
Ralf Habacker noticed that dbus contains calls to rand().

There are two classes of calls to PRNGs:

* those where "weak" randomness is acceptable, e.g. when used for a simulation;
* those where cryptographically strong pseudorandom numbers are required, i.e. the security model assumes that an attacker cannot predict future output.

dbus uses _dbus_generate_pseudorandom_bytes[_buffer]() (or plain rand()) for the former, and _dbus_generate_random_bytes[_buffer]() for the latter. Unfortunately, _dbus_generate_random_bytes[_buffer]() currently falls back to calling _dbus_generate_pseudorandom_bytes[_buffer]() if it cannot generate random bytes for whatever reason.

This means that if we are in one of these situations:

* on Unix: a dbus-daemon or other DBusServer that cannot read /dev/urandom
* on Windows: a dbus-daemon or other DBusServer that encounters some failure in CryptAcquireContext() or CryptGenRandom()
* either way: a dbus-daemon or other DBusServer that encounters malloc() returning NULL at the crucial moment

then an attacker could potentially use their ability to predict rand() output to bypass the DBUS_COOKIE_SHA1 and/or nonce-tcp authentication mechanisms.

Mitigations:

* the bad situations all seem like rather unlikely situations in which dbus is probably the least of your worries
* the bad situations are things that an attacker cannot readily cause
* the default configuration for the system bus specifically requires EXTERNAL authentication (unforgeable credentials-passing) so only the session bus would normally be vulnerable even if an attack is possible
* on Linux + systemd systems where dbus was configured with --enable-user-session, the bus socket for the session bus is protected by filesystem permissions, so the session bus is not vulnerable either
Comment 1 Simon McVittie 2015-05-12 10:16:02 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.
Comment 2 Simon McVittie 2015-05-12 11:51:54 UTC
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.
Comment 3 Simon McVittie 2015-05-12 11:52:24 UTC
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.
Comment 4 Simon McVittie 2015-05-12 11:52:40 UTC
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.
Comment 5 Simon McVittie 2015-05-12 11:52:55 UTC
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.
Comment 6 Simon McVittie 2015-05-12 11:53:09 UTC
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.
Comment 7 Simon McVittie 2015-05-13 10:40:51 UTC
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 8 Ralf Habacker 2015-05-13 13:43:11 UTC
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 9 Ralf Habacker 2015-05-13 20:53:15 UTC
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 10 Ralf Habacker 2015-05-13 20:56:04 UTC
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 11 Ralf Habacker 2015-05-13 21:07:48 UTC
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 12 Ralf Habacker 2015-05-13 21:13:17 UTC
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
Comment 13 Ralf Habacker 2015-05-13 21:14:51 UTC
(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.
Comment 14 Simon McVittie 2015-05-14 12:30:33 UTC
(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.
Comment 15 Simon McVittie 2015-05-14 13:49:35 UTC
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.