CVE-2013-2168: _dbus_printf_string_upper_bound: copy the va_list for each use

Using a va_list more than once is non-portable: it happens to work
under the ABI of (for instance) x86 Linux, but not x86-64 Linux.

This led to _dbus_printf_string_upper_bound() crashing if it should
have returned exactly 1024 bytes. Many system services can be induced
to process a caller-controlled string in ways that
end up using _dbus_printf_string_upper_bound(), so this is a denial of
service.

Reviewed-by: Thiago Macieira <thiago@kde.org>

2 files changed, 20 insertions, 5 deletions

diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c
index fc677990..e31c7355 100644
--- a/dbus/dbus-sysdeps-unix.c
+++ b/dbus/dbus-sysdeps-unix.c
@@ -3118,14 +3118,17 @@ int
 _dbus_printf_string_upper_bound (const char *format,
                                  va_list     args)
 {
   char static_buf[1024];
   int bufsize = sizeof (static_buf);
   int len;
+  va_list args_copy;
 
-  len = vsnprintf (static_buf, bufsize, format, args);
+  DBUS_VA_COPY (args_copy, args);
+  len = vsnprintf (static_buf, bufsize, format, args_copy);
+  va_end (args_copy);
 
   /* If vsnprintf() returned non-negative, then either the string fits in
    * static_buf, or this OS has the POSIX and C99 behaviour where vsnprintf
    * returns the number of characters that were needed, or this OS returns the
    * truncated length.
    *
@@ -3135,14 +3138,18 @@ _dbus_printf_string_upper_bound (const char *format,
   if (len == bufsize)
     {
       /* This could be the truncated length (Tru64 and IRIX have this bug),
        * or the real length could be coincidentally the same. Which is it?
        * If vsnprintf returns the truncated length, we'll go to the slow
        * path. */
-      if (vsnprintf (static_buf, 1, format, args) == 1)
+      DBUS_VA_COPY (args_copy, args);
+
+      if (vsnprintf (static_buf, 1, format, args_copy) == 1)
         len = -1;
+
+      va_end (args_copy);
     }
 
   /* If vsnprintf() returned negative, we have to do more work.
    * HP-UX returns negative. */
   while (len < 0)
     {
@@ -3152,13 +3159,16 @@ _dbus_printf_string_upper_bound (const char *format,
 
       buf = dbus_malloc (bufsize);
 
       if (buf == NULL)
         return -1;
 
-      len = vsnprintf (buf, bufsize, format, args);
+      DBUS_VA_COPY (args_copy, args);
+      len = vsnprintf (buf, bufsize, format, args_copy);
+      va_end (args_copy);
+
       dbus_free (buf);
 
       /* If the reported length is exactly the buffer size, round up to the
        * next size, in case vsnprintf has been returning the truncated
        * length */
       if (len == bufsize)
diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c
index bc4951b5..c42316f1 100644
--- a/dbus/dbus-sysdeps-win.c
+++ b/dbus/dbus-sysdeps-win.c
@@ -535,28 +535,33 @@ int _dbus_printf_string_upper_bound (const char *format,
                                      va_list args)
 {
   /* MSVCRT's vsnprintf semantics are a bit different */
   char buf[1024];
   int bufsize;
   int len;
+  va_list args_copy;
 
   bufsize = sizeof (buf);
-  len = _vsnprintf (buf, bufsize - 1, format, args);
+  DBUS_VA_COPY (args_copy, args);
+  len = _vsnprintf (buf, bufsize - 1, format, args_copy);
+  va_end (args_copy);
 
   while (len == -1) /* try again */
     {
       char *p;
 
       bufsize *= 2;
 
       p = malloc (bufsize);
 
       if (p == NULL)
         return -1;
 
-      len = _vsnprintf (p, bufsize - 1, format, args);
+      DBUS_VA_COPY (args_copy, args);
+      len = _vsnprintf (p, bufsize - 1, format, args_copy);
+      va_end (args_copy);
       free (p);
     }
 
   return len;
 }