| From 954d75b2b64e4799f360d2a6bf9cff6d9fee37e7 Mon Sep 17 00:00:00 2001 |
| From: Simon McVittie <simon.mcvittie@collabora.co.uk> |
| Date: Mon, 10 Jun 2013 18:06:47 +0100 |
| Subject: [PATCH] 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> |
| --- |
| dbus/dbus-sysdeps-unix.c | 16 +++++++++++++--- |
| dbus/dbus-sysdeps-win.c | 9 +++++++-- |
| 2 files changed, 20 insertions(+), 5 deletions(-) |
| |
| diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c |
| index fc67799..e31c735 100644 |
| --- a/dbus/dbus-sysdeps-unix.c |
| +++ b/dbus/dbus-sysdeps-unix.c |
| @@ -3121,8 +3121,11 @@ _dbus_printf_string_upper_bound (const char *format, |
| 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 |
| @@ -3138,8 +3141,12 @@ _dbus_printf_string_upper_bound (const char *format, |
| * 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. |
| @@ -3155,7 +3162,10 @@ _dbus_printf_string_upper_bound (const char *format, |
| 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 |
| diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c |
| index bc4951b..c42316f 100644 |
| --- a/dbus/dbus-sysdeps-win.c |
| +++ b/dbus/dbus-sysdeps-win.c |
| @@ -538,9 +538,12 @@ int _dbus_printf_string_upper_bound (const char *format, |
| 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 */ |
| { |
| @@ -553,7 +556,9 @@ int _dbus_printf_string_upper_bound (const char *format, |
| 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); |
| } |
| |
| -- |
| 1.8.3 |
| |