Re: ISO C11 issues for glibc

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Wed, 21 Dec 2011, Marek Polacek wrote:

> Which reminds me, what is your opinion on those *_s bounds-checking
> functions, e.g, vsnprintf_s etc, as specified in Annex K?
> I'd say we don't really need them since we have _FORTIFY_SOURCE
> and thus *_chk functions...

Apart from the reentrant functions (generally quite similar to existing 
POSIX functions, but under different names), the functions are generally 
designed for use in retrofitting large bodies of existing code to improve 
its security without needing a detailed understanding of that code.  
_FORTIFY_SOURCE is another way of retrofitting that requires no changes to 
the code at all, but I think it can be useful to have both.

There is certainly a case that, for example, it's better coding style to 
use snprintf rather than sprintf even when you *also* dynamically (or 
statically) size your buffers so that snprintf should never cause 
truncation.  Silent truncation is bad, and could be a security hole, but 
it's less likely to be a security hole than a buffer overrun, so if you 
write your code to avoid arbitrary limits and silent truncation, a coding 
style that always uses snprintf instead of sprintf can reduce the risk of 
a bug in the code turning into a security hole by changing a buffer 
overrun into truncation (if you passed the correct size; it can't solve 
all problems, but can at least encourage you to think about sizes 
everywhere).

Much the same applies to a few of the functions in Annex K, where the 
corresponding existing functions do not have buffer sizes passed 
explicitly; there is a use for coding styles that ensure that explicit 
buffer sizes are always passed (and personally I would have allowed in 
strlcpy/strlcat long ago for that reason; you don't want truncation ever 
to happen, but they still help reduce the risk of some bugs, and glibc has 
traditionally included functions that are widely used in libc on other 
systems, which by now applies to those functions).

Other bits are rather stranger, such as tmpfile_s (adding a runtime check 
that a pointer is not NULL) and tmpnam_s (which leaves a race condition 
still present, so certainly shouldn't be used in new code; presumably it 
was added on the basis that otherwise legacy code might not have tmpnam 
calls retrofitted at all, and some fixing was better than none).

Then there's rsize_t and RSIZE_MAX, intended to limit size_t parameters to 
less than SIZE_MAX (probably no more than SIZE_MAX >> 1).  There is a real 
issue here.  With normal implementations of C, it is impossible for 
objects half the address space or more in size to work reliably; pointer 
subtraction within such an object will produce values that cannot be 
represented in ptrdiff_t, so silently resulting in undefined behavior, and 
in practice even if the pointer target type is larger than one byte, the 
division happens after the subtraction and the results will be wrong (see 
<http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45779>).  And of course 
pointer subtraction might be generated internally even if not in the 
user's source code.

I don't think there's anything that can sensibly be done in the compiler 
about this issue; I think the only way to avoid security problems there is 
for malloc and other allocation functions to refuse to allocate objects 
using half or more of the address space (so it would no longer be possible 
to allocate 2GB or more in a single allocation on a 32-bit system; for 
64-bit systems there would be little practical effect).  (It's already the 
case that GCC will give an error if you attempts to declare an object half 
the address space or larger in (constant) size, whether statically 
allocated or on the stack.)  You don't need rsize_t for that, of course - 
you can and should change the existing allocation functions to put in this 
limit - and once you have such limits no objects can exceed RSIZE_MAX 
anyway so the checks (in various Annex K functions) for passing sizes 
bigger than RSIZE_MAX become just checks for some random possible bug in 
the program, much like the checks for NULL.

-- 
Joseph S. Myers
joseph@codesourcery.com