Bug 35311

Created attachment 85008 [details] [review]
_dbus_verbose_real: Use snprintf for security reasons

http://stackoverflow.com/questions/7315936/which-of-sprintf-snprintf-is-more-secure mentions to use snprintf instead.

Created attachment 85019 [details] [review]
dbus_verbose_real: Use snprintf for security reasons (update)

minor fix

Comment on attachment 85019 [details] [review]
dbus_verbose_real: Use snprintf for security reasons (update)

Review of attachment 85019 [details] [review]:
-----------------------------------------------------------------

hThe main problem here is that we're going to heroic lengths to avoid malloc() or similar allocators, because libdbus is meant to be "malloc()-returns-NULL"-safe. However, the justification for that is that on Unix, libdbus can be used in important system services (dbus-daemon --system, Upstart, systemd) where it is unacceptable to abort(), even on OOM.

In Windows, it doesn't seem as though that's particularly relevant. We only have "dbus-daemon --session" and other session applications, which basically don't have memory limits anyway, and Windows will keep running whatever happens to libdbus users. So, in Windows-specific code, would we be better off with a GLib-like policy where "out of memory" in a particularly inconvenient place is just fatal? Then we could use the much safer DBusString for this string manipulation, and just crash if it fails. (Common code used on both Windows and Unix would still have to be OOM-safe.)

Another possibility would be to do something like this, at least on Windows:

{
  DBusString buf;
  char stack_buf[1024]; /* for fallback, optional */

  if (!_dbus_string_init (&buf, ...))
    goto oom;

  if (!_dbus_string_append (&buf, ...))
    goto oom;

  if (!_dbus_string_append_printf_valist (&buf, ...))
    goto oom;

  _dbus_string_free (&buf);
  return;

oom:
  _dbus_string_free (&buf);
  OutputDebugStringA ("_dbus_verbose_real: not enough memory to compose a debug string");

  /* optional */
  OutputDebugStringA ("_dbus_verbose_real: here is the best we can do (possibly truncated):");
  _vsnprintf (stack_buf, sizeof (stack_buf), format, args);
  OutputDebugStringA (stack_buf);

  return;
}

Or, I notice in MSDN that OutputDebugString() is described as using DbgPrint(), which can only print 512 bytes anyway, so perhaps our strings are just going to get truncated anyway... if that's the case then we might as well just truncate too (but making sure to add a '\0'), and not worry so much about it?

Nitpicking: I'd prefer libdbus coding style (foo () instead of foo(), "a, b" instead of "a,b") on lines that you're adding/editing anyway, but it isn't particularly important.

::: dbus/dbus-internals.c
@@ +416,5 @@
>    {
> +  char buf[1024+1];
> +  char *p = buf;
> +  int size = sizeof(buf);
> +  int result = snprintf(p, size, "%s", module_name);

If module_name is exactly 1025 bytes long (not including '\0'), this leaves buf without a '\0', so the OutputDebugStringA() would read off the end into uninitialized memory.

(Yes, I realize module_name itself is currently a static buffer of limited size too...)

Minimal correct handling, if you don't mind truncation (which is pretty much unavoidable if you're avoiding malloc()), is to either:

* set buf[sizeof (buf) - 1] = '\0' after each (v)snprintf, so the 1025th byte is 0 whenever we call OutputDebugStringA()

or:

* set buf[sizeof (buf) - 1] = '\0' initially, so the 1025th byte is initially zero
* set "size = sizeof (buf) - 1" and reduce from there, so that the 1025th byte remains zero

@@ +417,5 @@
> +  char buf[1024+1];
> +  char *p = buf;
> +  int size = sizeof(buf);
> +  int result = snprintf(p, size, "%s", module_name);
> +  if (result >= size)

Does snprintf() actually work like this on Windows? I thought it returned -1 if it would have overflowed (see _dbus_printf_string_upper_bound)?

@@ +429,3 @@
>  #ifdef DBUS_CPP_SUPPORTS_VARIABLE_MACRO_ARGUMENTS
> +  result = snprintf (p, size, "[%s(%d):%s] ",_dbus_file_path_extract_elements_from_tail(file,2),line,function);
> +  if (result >= size)

Same two problems again.

@@ +439,2 @@
>  #endif
> +  result = vsnprintf (p, size, format, args);

And again.

I thought Windows (at least with MSVC++) didn't have vsnprintf(), only _vsnprintf()?

"nishta", please do not make random changes to bugs in products you do not maintain. The bug metadata is there to make it easier for the maintainers to triage bugs; before changing it, double-check that your change is correct.

This is a Windows-specific bug (use of fixed-size buffers in code that is only compiled for a Windows host), so changing the OS from "Windows (All)" to "Linux (All)" is clearly incorrect.

Tagging as "patch, negative review" while I'm here. If someone wants to correct the things I raised during my review, please go ahead.

My current position on D-Bus security support is:

* D-Bus on "normal" GNU/Linux platforms is security-supported; security-sensitive bugs there (including denial of service) are a high priority, and will result in a bugfix release from the stable branch if applicable.

* D-Bus on other Unix platforms (*BSD, Solaris, GNU/Linux with LSMs, Android and other non-GNU Linux platforms, etc.) is security-supported to the best of our ability, but if a bug is platform-specific, then users/porters from that platform will need to help. If a fix is available, there will be a bugfix release from the stable branch if applicable.

* D-Bus on Windows is not suitable for use as a security boundary (in particular, no system bus), and should only be used in contexts where untrusted users cannot communicate with it.

I think that's pretty much consensus among the maintainers.

See also Bug #75861, which has a patch that appears to be technically correct, but I'd still prefer something that is *obviously* correct (like DBusString).

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/45.