03 March 2014

Why choosing the right tools for software development is très important, and why you should turn on compiler informational messages.

goto fail;

So, by now about half the country is familiar with what is being called the "goto fail" bug in Apple's Secure Socket Layer implementation in both OS X and iOS. And, frankly, I'm surprised that this took forever to be noticed.

Why? Because any good C compiler would have caught it!!

The code in question:

Here's the code. It's pretty simple to understand, actually, no knowledge of C is required. (Formatting and comment taken from Naked Security's article on the subject.)

hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
    goto fail;
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;

err = sslRawVerify(...);
The code performs several functions, and if any of the return codes from the functions is not 0 (a recognized programming standard, non-zero means something bad happened), the code goes to label fail (not pictured here). Note the commented goto fail line, however. There's no condition there, so the computer is going to branch to fail, and the err variable will be 0, because of the successful return of the prior function. Big oops. Major oops. Catastrophic oops.

My take

Billions of bits and CPU cycles have already been burned on this. Heck, even some code analysis ISVs are screaming from the rooftops that their products would have caught it. And that's a good thing, because software applications are so complex these days that issues exist across multiple source code units, and a compiler + linker/binder isn't sophisticated enough to see those. But this bug is so simple that a good compiler should have caught it and issued a warning message, or even an informational message. But no one seems to have either noticed this, or run a compile with informational messages turned on. Informational messages can be a nuisance during the majority of the development cycle, but as you work out those final little nits, a developer should turn them on, because they can point out scenarios that can lead to critical issues down the road. 

I have a suspicion that with today's DevOps, Agile, Scrum, and other "modern" development methodologies, the time for going through the code with a fine-toothed comb is thought of as unnecessary, because we have that two-week cycle deadline coming up, and frankly we need to get the code out, bugs and all.

But then of course you have to have a compiler that checks for this kind of stuff. And in my experience I have not seen this come out of any other C compiler except the IBM System z C compilers. This is probably because the prior IBM mainframe compilers, such as COBOL, have checked for this sort of thing for decades. For example, from Enterprise COBOL 4.2:

IGYXX3091-W     Code from "?" to "?" can never be executed and was therefore discarded.      

And I remember this exact message (although its prefix was IGC) from the ANS Full COBOL V4 compiler when I was in college 35 years ago.

If other compilers have this checking available, this helps cement my feeling that code quality is being ignored in the interest of expediency, because developers aren't using it, obviously!

(If you do know how to generate these messages out of a particular compiler, please let us know in the comments. Some developers may not know how to get them.)

Sadly, as long as this misguided of time-versus-money paradigm continues in the software development world, you're going to see stupid mistakes like this, or hard-coded admin IDs and passwords inside software (the persistent rumors about 2013's data breach at Target Stores keep saying this), or encryption set aside because it's too darned expensive (which is why the USA once again is playing catch-up to Europe in the credit card business).

Informational messages: they're put there for a reason. Use them to create better software.

1 comment:

  1. I could not agree with you more. For example, turn Wall (warn all) option on, and the release standard should be "no warnings". None. But, that's a different mindset from that of the programmers (sorry, "developers") who write a rough draft, throw it into a debugger and if it runs, then it's done. Unit testing does NOT solve this mindset problem, either; passing unit tests doesn't mean it's right, it just means that some unit tests pass.


Feel free to leave a comment or ask questions.