Wei Liu (刘炜) of Tencent Security Xuanwu Lab discovered an interesting issue in libexif: the code checked for an integer overflow when handling a field in EXIF files, but that check was silently removed by the compiler.
You can find the diff for the fix here: https://android.googlesource.c… . The original code has this line:
if (e->size+sizeof(unsigned short) < e->size) break;
To see whether
e->size is so large that it almost overflows, the programmer thought to add the size of a terminator (UTF+0000 which is an
unsigned short), and then to check whether that total size is actually smaller than the original number, which means that an overflow must have occured.
Compilers are smart however, and they remove unnecessary code. Assuming that
sizeof(unsigned short) is 2 (which is true on most platforms), the compiler sees a check for x+2 > x which it evaluates to a constant value
false. Because something + 2 can never be smaller, right?
And if you put
if(false) whatever(); in your code, the compiler is not going to bother creating binary code for
whatever() — it will be ignored completely.
So what you end up with is code that has the check, and could pass a code review or audit. But the binary, the machine code that actually runs on people’s devices, does not have the check!
So what is the fix? The check above was replaced by this one (omitting, for brevity, the logging code that was also added):
if (e->size > UINT_MAX - sizeof(unsigned short)) break;
This takes the value at which an
unsigned int overflows (
UINT_MAX), takes 2 off the top, and checks that the size of the argument (
e) doesn’t exceed that. The compiler will not be able to fully evaluate this check at compile-time. It will see (again, on most platforms) that
UINT_MAX is 4294967295, and
sizeof(unsigned short) is 2, and get stuck at
if(e->size > 4294967293), leaving it with no choice but to create binary code to do a run-time check.
Bottom line? Update, because there’s a fix. And developers, stop trying to be clever and write down exactly what you mean. :)