Recently a security flaw in Apple OSes was made public, and made some fuss. In short, this flaw would allow a “man in the middle” type of attack on a SSL/TLS connection. This post intent is not to dwell on the security issues of this security hole. Instead, I’d like to discuss the reasons why this kind of things do happen.
Adam Langley details the specifics of this bug, and that is the part that interests me the most:
static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
uint8_t *signature, UInt16 signatureLen)
{
OSStatus err;
...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;
...fail:
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;
}
Look at the two consecutive “goto fail;” lines. The indentation may mislead you. At a first glance one can say that the second goto is dead code. But if you look more closely you can see that the if is a single statement. The result is that all code below the second goto is dead code, meaning that all validations after it will not run.
Well, why do this happens? Taking apart the specifics of each language and IDE, there are some reasons that may explain why this kind of bug can reach production systems.
First, a badly indented file. Setting automatic code formatters should help on this. Enforcing auto formatting to all team members with the same set of rules is a start. Eclipse, for example, lets you configure automatic file formatting on every save.
Second, coding conventions. I remember a conversation I had last month with one of my team’s junior members, explaining him why our coding conventions forbid single line if’s, and the justification I gave to him was exactly this: it is too easy to have a bad indented line of code mislead you while looking at code. Once again, there are tools like Checkstyle to help enforce code conventions in your dev and even in the build system.
Warnings are not errors, but most of the time they should be. An “unreachable code detected” warning should be treated as an error, not a warning. There should be a reason for every piece of code’s existence. If it will never run, then it shouldn’t be there. In many cases, an “unreachable code detected” means “potential bug detected: please look at it and FIX IT”. Programmers have a natural tendency to ignore warnings (“if it is not a red icon, don’t bother looking at it…”). Raising all warnings to error level may not be the best solution for this problem, but certainly there are a bunch of warnings that will save you some nights of sleep if they are treated as errors, from the very beginning of your project.
The last reason for this kind of bug’s existence is probably the root of all other reasons: programmers are only humans. And as such, they will make mistakes. The baseline here is what one can do to minimise the probability of errors happening, while keeping coders happy doing what they like the most: coding. That’s where defining best practices, code conventions and automating everything that can be automated comes into place. And these are some of the most important aspects of a Software Factory. That and continuously formatting your team so everything runs smoothly.
Bad things happen. Unfortunately, they happen too many times. Our mission as software builders is to setup the mechanisms that may avoid errors that can be avoided. Because programmers are just… human. Fortunately!.
