Bug 35311

Summary: sysdeps-win: abuse of sprintf into fixed-size buffers
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: D-Bus Maintainers <dbus>
Status: RESOLVED MOVED QA Contact: D-Bus Maintainers <dbus>
Severity: major    
Priority: medium CC: msniko14, ralf.habacker
Version: 1.5Keywords: patch, security
Hardware: All   
OS: All   
Whiteboard: review-
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 68741    
Attachments: _dbus_verbose_real: Use snprintf for security reasons
dbus_verbose_real: Use snprintf for security reasons (update)

Description Simon McVittie 2011-03-14 12:11:16 UTC
I happened to notice that the Windows version of _dbus_system_logv calls sprintf into a fixed 1024-byte buffer. Stack smashing, anyone? (I'm not treating this as a security hole as such, though, since D-Bus on Windows isn't a security boundary.)

Most of our strings are limited to 255 bytes, but you only need a format-string with 5 of those to smash the stack.

I looked a bit further, and found these:

* _dbus_poll in verbose mode calls sprintf into a fixed-size buffer on
  two separate occasions; in the first one it detects stack smashing afterwards,
  but that's too late

* _dbus_verbose_real does similarly

although those are only present in verbose mode.

There are also a couple of uses of sprintf in Unix code; those are made into buffers guaranteed to be large enough (assuming vsnprintf is POSIX-compliant, Bug #11668), but I'm tempted to open a bug for them anyway. "Never use sprintf" is easier to enforce than "use sprintf securely".
Comment 1 Ralf Habacker 2013-09-01 13:14:45 UTC
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.
Comment 2 Ralf Habacker 2013-09-01 18:44:51 UTC
Created attachment 85019 [details] [review]
dbus_verbose_real: Use snprintf for security reasons (update)

minor fix
Comment 3 Simon McVittie 2013-09-02 10:54:07 UTC
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()?
Comment 4 Simon McVittie 2014-02-13 11:18:24 UTC
"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.
Comment 5 Simon McVittie 2014-04-30 19:08:20 UTC
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).
Comment 6 GitLab Migration User 2018-10-12 21:08:53 UTC
-- 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.

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.