Watch out for missed warnings on vendor C++ toolchains - Graham Sutherland's Blog

I figure we’re overdue a security post on here, so here’s a quick one.

I was recently reviewing some embedded C++ for a lesser-used architecture. There is GCC support for the architecture, but the developers of the thing I was looking at utilise a vendor SDK which only compiles properly with the vendor’s own toolchain. It’s a security-sensitive thing so they turned up their compiler warnings and promoted them to errors, with something along the lines of:

-Werror -Wall -Wextra -Wpointer-arith -Wpointer-sign -Wnarrowing -Wconversion -Wincompatible-pointer-types<br>I didn’t have access to the vendor tools, so I set up a GCC cross-compiler for the architecture and battled the SDK’s incompatibilities until it built. Or, rather, I would have, except a ton of the non-SDK code was also generating warnings which turned into errors due to the compiler flags. This is odd because it apparently compiles just fine under the vendor toolchain. I turned off -Werror just to get it to build, then started triaging the warnings. One of them led me to a function that looked like this:

int memcmp_ct_32(uint32_t* a, uint32_t* b, int words)<br>uint8_t acc = 0;<br>while (words--)<br>acc |= *a ^ *b;<br>a++;<br>b++;<br>return acc;

This is supposed to be a constant-time memcmp implementation for 32-bit aligned blocks of data. It works by computing the xor of each value pair, which will produce zero if and only if the two values are equal, and or’ing the result into an accumulator. At the end of the loop the accumulator should only be nonzero if there was a difference at some point. Since the loop has no early-out optimisation it should be constant-time. This is a pretty typical approach.

Unfortunately there’s a pretty major bug here: the accumulator is a uint8_t. This leads to an implicit narrowing conversion where the 32-bit result of *a ^ *b is truncated to an 8-bit accumulator, throwing away three out of four bytes in the comparison. As such, this memcmp implementation only ends up comparing every fourth byte. Whoops.

The implicit narrowing conversion in this function raises a warning under GCC. This is a perfect use-case for this compiler warning and promoting warnings to errors: a simple copy-paste error from the bytewise variant of the function should be stopped in its tracks as soon as you try to build. However, it turns out that it does not generate a warning under the vendor’s toolchain, even though -Wconversion was enabled. I don’t know if this warning is buggy or simply isn’t implemented and the flag gets silently ignored.

This isn’t the first time I’ve run into a discrepancy like this, and it demonstrates a useful lesson: if you’re reviewing code that only normally gets seen by a vendor toolchain or an older compiler version, you should ideally also try to build the thing under the latest version of gcc or clang with all the warnings turned up to 11, just to see what drops out.

Note: An earlier version of this blog post mentioned -Wnarrowing as the flag in question, not -Wconversion. I wrote this while tired and mixed up the flag names while recalling the issue from memory, but in fairness these flags are horribly named and it makes absolutely no sense that implicit narrowing conversions are not covered by -Wnarrowing. Thanks to Martin Uecker for pointing out this discrepancy.