(v)s(n)printf hardening

[gvranken]

vsnprintf is a function that can fail. It will always fail for strings
that require over 2 gigabytes of buffer space, for the simple reason
that this size cannot unambiguously be expressed in its return value;
a return value of >= 0 means that that many bytes were written to the
buffer, and a negative return value indicates overall failure. Because
the largest positive value of a signed int is 2 gigabytes, buffer
consumption is limited to that amount.
For some format strings, snprintf will also internally allocate
(usually small) heap buffers, and will return failure if any of these
allocations fail. I believe the FreeBSD internal dtoa() function
allocates heap memory, for instance. A memory leak in the main program
that can grow so large that vsnprintf cannot allocate its private
buffers, will result in failure. In principle, the main program
(OpenVPN) can never assume that a call to vsnprintf succeeds, unless
it returns a non-negative integer.

This is the current openvpn_snprintf in openvpn:

 277 bool
 278 openvpn_snprintf(char *str, size_t size, const char *format, ...)
 279 {
 280     va_list arglist;
 281     int len = -1;
 282     if (size > 0)
 283     {
 284         va_start(arglist, format);
 285         len = vsnprintf(str, size, format, arglist);
 286         va_end(arglist);
 287         str[size - 1] = 0;
 288     }
 289     return (len >= 0 && len < size);
 290 }

The vsnprintf specifications do not give a clear definition on what
the contents of the output buffer may be if the function fails.
Hence, if the call to vsnprintf within openvpn_snprintf fails, the
space str[0..size-1] may be uninitialized data. Many calls to
openvpn_snprintf do not check its return value, so if vsnprintf fails,
and the buffer contents is subsequently written to the peer (or to
any other channel, like a file), this could constitute a significant
data leak.

Also overlapping parameters will produce undefined buffer contents.

man vsnprintf explicitly says:

       Some programs imprudently rely on code such as the following

           sprintf(buf, "%s some further text", buf);

       to append text to buf.  However, the standards explicitly note
that the results are undefined if source and  destination  buffers
overlap  when  calling  sprintf(),  snprintf(),
       vsprintf(), and vsnprintf().  Depending on the version of
gcc(1) used, and the compiler options employed, calls such as the
above will not produce the expected results.

These are definitely corner cases, and I'm not aware of an easy to to
force vsnprintf failure in openvpn. But the stakes are high (private
crypto data and what have you), and other bugs (excessive string
generation that is then sprintf'ed, overlapping strings, memory
shortage) might lead to a significant security problem this way,
so my I'm attaching a patch that ensures that the final contents
of the output string is always a valid string that does not
contain uninitialized or unrelated data (barring any bugs in
the implementation of vsnprintf, which is outside OpenVPN's
responsibility or control).

Guido

[ethancampbell]

spam