Re: on memory usage with the 64bit patch, and interpretation of various numbers
On Wed, May 14, 2014 at 6:39 PM, Pierre Joye <pierre.php@gmail.com> wrote:
> ## Rationale for size_t (string lengths):
>
> This has significant advantages. There are some costs to doing it, but
> they are not as significant as they may appear on the surface. Let's
> dive into it:
>
> ### It's The Correct Data Type
>
> The C89 spec indicates in 3.3.3.4 (
> http://port70.net/~nsz/c/c89/rationale/c3.html#size-95t-3-3-3-4
> ) that
> the size_t type was created specifically for usage in this context. It
> is always, 100% guaranteed to be able to hold the bounds of every
> possible array element. Strings in C are simply char arrays.
> Therefore, the correct data type to use for string sizes (which really
> are just an offset qualifier) is size_t.
>
I don't have a copy of the C standard handy, so I'll quote what C++ has to
say on size_t:
> The type size_t is an implementation-defined unsigned integer type that
is large enough to contain the size in bytes of any object
So a size_t type can contain the size of *any* object. If we want to
support *any* object, then we must use the size_t type, that's correct.
However, we may reasonably choose to disallow creating objects of certain
types (e.g. strings) with sizes that are larger than a uint32_t, to save
memory. If we chose to implement such a restriction, the uint32_t type
would provide us with the *exact* same guarantees as size_t does - it is
large enough to contain the size of any object that we allow to be created.
### It's The Secure Data Type
>
> size_t (and ptrdiff_t) are the only C89 types that are 100% guaranteed
> to be able to hold the size of any possible object that the compiler
> will support. Other types will vary depending on the data model that
> the compiler supports, as the spec only defines minimum widths.
>
> This is so important that CERT issued a coding standard for it:
> INT01-C (
> https://www.securecoding.cert.org/confluence/display/seccode/INT01-C.+Use+rsize_t+or+size_t+for+all+integer+values+representing+the+size+of+an+object
> ).
>
> One of the reasons is that it's difficult to do overflow checks in a
> portable way. See VU#162289: https://www.kb.cert.org/vuls/id/162289 .
> In there, they recommend using the C99 uintptr_t type, but suggest
> using size_t for platforms that don't have uintptr_t support (and
> since we target C89 for the engine, that's out).
>
Overflow checks are an issue that is independent of the chosen size storage
type. They always you need to occur. The addition of two size_t values can
overflow in just the same way as the addition of two uint32_t values. There
is no difference in security between using size_t and uint32_t. The
security issue lies in absence of proper overflow checks (or in the
presence of incorrect overflow checks).
If anybody can come up with an example where usage of uint32_t over size_t
(in a context where we ensured that allocations do not surpass that size)
causes security or other safety issues, please speak up. I wasn't able to
find anything in that direction.
### ZPP Changes
>
> The ZPP changes are critical. The reason is that varargs is casting an
> arbitrary block of memory to a type, and then writing to it. So
> existing code that does zpp("s", str, &int_len) would wind up with a
> buffer overflow. Because zpp would be trying to write a 64 bit value
> to a 32 bit container. The other 32 bits would fall off the end, into
> who knows what. At BEST this can result in a segfault. At worst,
> memory corruption and MASSIVE security vulnerabilities.
>
> Also note that the compiler *can't* and actively doesn't catch these
> types of errors. That means that it's largely luck and testing that
> will lead to it.
>
> So, I chose to break BC and rename the ZPP symbols. Because that WILL
> error, and provide the developer with a meaningful indication that an
> improper data type was provided. As I considered a fatal error that an
> invalid type was supplied was a better way of identifying to the
> developer that "HEY, THIS NEEDS TO BE CHANGED ASAP" than just letting
> them hit random segfaults at runtime.
>
> If there is a way to get around this by giving the compiler more
> information, then do it. But to just leave the types there, and leave
> it to chance if a buffer overflow occurs, is dangerous. Which is why I
> made the call that the ZPP types **needed** to be changed.
>
Afaik Johannes has written a Clang-based static analyzer that detects type
mismatches between the zpp format string and the passed arguments (gcov
also seems to have some kind of analyzer for that). That seems like a good
way to find incorrect declarations without having to rename everything -
and this also covers checks for all types that weren't changed :)
Nikita
Thread (10 messages)