Apple Fail Bug

February 28, 2014
You can go to https://gotofail.com/ to see if you are vulnerable to this bug.

When I first saw the now pop culture Apple goto fail bug resulting in a critical SSL vulnerability on iOS, I immediately had dark flashbacks of seeing more compiler warnings than lines of code on projects I've worked on. So, I was expecting to demonstrate that a simple compiler warning would have exposed this problem, but I was wrong. Even though I knew gcc had an unreachable code warning and in fact I'd had warnings with it in the past, it turns out the past is the past.

Here's the snippet of code containing the bug, which has since been fixed in sslKeyExchange.c.

if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
		goto fail;
	if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
		goto fail;
		goto fail;
	if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
		goto fail;

The double goto fail; results in failure when it shouldn't. How was this error was introduced? It smells of a merge error. This type of merge error is fairly common. I doubt it's malicious because it's so obvious and the indentation is inline.

So, on to trying to see what tools could have prevented this, instead of focusing on processes.

To make things simpler, I created an example that basically has the same problem.

#include <stdio.h>
 
int main(int argc, char** argv)
{
   if (argc > 1)
      goto fail;
      goto fail;
 
   printf("success\n");
 
   return 0;
 
 fail:
   printf("fail\n");
   return 1;
}

And then I went compiling away with gcc.

gcc -O2 -Wall -Wunreachable-code -pedantic warning.c -o warning

To my surprise, this produces no warning on gcc 4.6.3. In fact, -Wunreachable-code is no longer supported in gcc. Well, that's good to know.

I ran cppcheck 1.52 on the code. Nothing.

Then, I ran splint on the code and finally some success.

$ splint warning.c 
Splint 3.1.2 --- 03 May 2009
 
warning.c: (in function main)
warning.c:9:4: Unreachable code: printf("success\n")
  This code will never be reached on any possible execution. (Use -unreachable
  to inhibit warning)
warning.c:3:27: Parameter argv not used
  A function parameter is not used in the body of the function. If the argument
  is needed for type compatibility or future plans, use /*@unused@*/ in the
  argument declaration. (Use -paramuse to inhibit warning)
 
Finished checking --- 2 code warnings

Also, I'm almost positive FlexeLint would have found the issue just as well. There likely some other commercial solutions that would have found it too. Apple could surely afford some nice static analysis tools, but to each their own I guess.

It turns out, goto is a favorite source of causing security bugs in all operating systems. You should use it as much as you can against all recommendations.

Related Posts