I sometimes take some stick for having a very defensive coding style. When ever I find myself making an assumption I throw in code to test it. “This function will only ever be called with a positive number”, OK, then add a test to throw an exception should a negative number be passed. You don’t want bad data ricocheting through your code because goodness knows what damage it will do! Similarly, my style is to always use explicit syntax, and, to avoid syntax shortcuts – sure, the ternary operator takes up less space on the screen, but there’s a price to pay for that terseness – it makes your code harder to read and hence to debug and maintain.

However, one of my very biggest bug-bears is the failure to brace control statements like conditionals and loops when they’re operating on a single line of code. This is the trap Apple fell into so spectacularly this week.

When it comes to online security, one of the single most important protocols is TLS/SSL, it’s what puts the S into HTTPS. Without TLS/SSL there is no e-commerce. SSL/TLS shows up in many places, but probably it’s most prominent use is in our browsers. Apple use WebKit as the engine that drives both the mobile and desktop versions of their Safari browser, and that’s where Apple got badly stung by not bracing their if statements. WebKit contains an implementation of SSL/TLS so as to enable HTTPS, and the SSL/TLS code contained the following lines if C Code:

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;
    goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;

The generic definition of an if statement in C would look something like this:

if(CONDITION){
  /* a block of code here that will only execute if the condition evaluates to true */
}

The braces ({ & }) contain the code that is controlled by the condition. By tradition the code in the braces is indented, but that’s purely to aid readability, that indentation has no meaning.

When your condition is controlling a block of multiple lines of code the braces are mandatory, and can’t be left out. However, when you only want to control execution of a single line of code, C allows you to omit the braces, so, the following two statements are effectively identical:

/* The Braced Version */
if(day_of_week > 5){
    printf("Hurray - it's the weekend!");
}

/* The Un-braced Version */
if(day_of_week > 5)
    printf("Hurray - it's the weekend!");

Here the braced version is one line longer than the unbraced version, but that’s because I used the K&R style of bracing, many people use the Allman style, which takes up even more room:

/* The Allman-style Braced Version */
if(day_of_week > 5)
{
    printf("Hurray - it's the weekend!");
}

It’s tempting to think “oh sod the braces, they’re just superfluous fluff”. BUT, the key point is that the indentation is meaningless, so if you add a second line ‘inside’ an unbraced if statement, that line is NOT doing what it looks like it does. E.g.

if(day_of_week > 5)
    printf("Hurray - it's the weekend!");
    printf("Carpe Diem!");

We’ve indented the second line, so mentally it looks like it’s being controlled by the if statement, but it is not, and unbraced if only works on ONE line, so what we’ve really written is:

if(day_of_week > 5){
    printf("Hurray - it's the weekend!");
}
printf("Carpe Diem!");

The second line will ALWAYS happen, because the if statement is not controlling it.

Now, lets take another look at part of Apple’s code:

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

Someone has obviously accidentally duplicated a line, so at first glance you might think the second goto fail will never execute because the first one will redirect control to the line labeled fail. That would be true IF Apple had bothered to brace their if statements, but because they didn’t something much more dangerous happened, the code is equivalent to:

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

It’s not the second pointless goto that is unreachable, it’s all the rest of the code after that line that’s become unreachable. In this case, the unreachable code includes vital security tests, hence this tiny woopsie is a massive security blunder putting all Mac and iOS users at risk!

Now, I’m advocating defensive coding, so how do I think the original code should have been written, and how would that have helped?

How would I have written it differently, I would have braced the if statements, so my code before the accidental line duplication would have looked like:

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;
}

Then, after the woopsie, the code would have been left in this state:

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;
    goto fail;
}
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0){
    goto fail;
}

The end result – had this code been written defensively, there would have been no security vulnerability, but because it was written in a brittle way, there was, and millions of supposedly secure connections were actually vulnerable to Man-in-the-middle attacks.

Bottom line – good habits matter, and if you write code, you should get into the habit of writing defensive code!