gotofail
February 25th, 2014

What Apple's "goto fail" teaches us as developers at P'unk Avenue

Tom Boutell
Chief Software Architect

You have probably heard that there is a massive security hole in Mac OS X thanks to a nasty mistake in Apple's implementation of SSL (secure sockets layer, the thing that powers online shopping).

If you're a programmer in any language, the bug is actually easy to see once you know it's there. You don't have to know Objective C at all. The same thing would be a nasty bug in JavaScript or PHP:


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;

I bolded that puppy for you.

They tested something ("if") and if it's true, they did something ("goto fail"). Then, because they had too much coffee, they typed "goto fail" again. Then they never noticed. The human brain stinks at noticing a repetition of the same thing. Copy editors never miss a misspelling but they often have a tough time spotting a repeated sentence.

Why does it matter? Written this way, "if" runs the next statement if the condition is true... then the computer plows ahead with the next statement regardless. And they accidentally added a second copy of that statement.

As a result of which, anybody can pretend to be any "secure" website in Safari and other programs if they can hijack the IP address. Ow ow ow.

But there's a better way to write if statements. And if you write them properly, this sort of bug is much esier to see:

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

}

goto fail; // I stick out like a sore thumb now

 

So for programmers, the simplest moral of this story is: don't use "if" without the curly braces. Ever. It adds absolutely zero value beyond showing off and confusing the next person to touch your code.

Some other possible morals:

"They should have written a unit test for this possible problem." Yes they darn well should have for a vital matter of security like this.

"goto is evil?" Well... often, yes. But in this case it's being used to handle an exceptional condition, and in C code there isn't much of an alternative. I don't know for sure if they had the option of using Objective C's exception handling in this sytem library, so we have to give them a pass there. Generally speaking this is the least evil kind of goto.

"Pay attention to compiler warnings and/or jshint?" Yes! Many coding tools can be set to warn you when you do things that are technically legal but not very bright.

Which leads me to a question specifically for JavaScript fans like us:

Are you still not using jshint in your editor to tell you about 99% of your JavaScript bugs before they happen?

For the love of pete, why not?

Get on that, please! The quality of my code has improved dramatically since I started using it. There are no accidentally global variables hanging about, and no accidental uses of "=" (assignment) or "==" (kinda equal-ish) where I meant to use "===" (really equal dammit). I have been super happy with it. It has a few warnings I don't agree with, but as long as I read them, I can feel free to disregard them.

Everyone can stand to pay attention to the tools that remind them of silly mistakes. Heck, the smarter you are, the less sense it makes to waste time with silly mistakes. You have better things to do. I'm sure the folks at Apple are suddenly remembering they have better things to do.

T-Shirt available from teespring.

 

Tom Boutell
Chief Software Architect