Why code style is important
Over at ImperialViolet, there's an interesting argument observation on Apple's recent SSL/TLS bug in iOS. This is the code in question:
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;
}
See that third goto fail? The first two take advantage of the fact that in C, you can have a single statement following an if, or you can use a curly-brace-delimited block. That third goto, positioned as it is, becomes an unconditional branch.
So the SSL signature verification never happens - the session is happily accepted and off we go to who knows where. If the block syntax had been used, there would simply have been a second unreachable goto in the block (and if -Wall was on there'd have been a warning about that) - and there would have been no security bug!
Blogging about Perl, wandering off into compatibility issues with other things, like Python and Django.
I don't understand the point - this is more a question of code review instead of style
Jean-Damien, it is a comparison to Perl's syntax of if-else blocks where a single statement after an if needs a {} block definition. In C, a single statement after an if condition without any {} delimiters is considered part of the if-statement.
This is more a problem with copy-paste style coding and bad testing of MITM attacks.
I believe this is part of the reason C/C++ programmers put the braces on a new line, to make them stand out. Whereas when you program in Perl, you know the brace is there otherwise it wouldn't compile.
With postfixed ifs something like this is very unlikely to happen.
I find it funny that this would be warned about just by setting -Wall.
Our Wall would not accept such code.
And this is why, when I'm hacking on Javascript, I always put the curly braces on if statements, even if the body is only a single line.
This looks like something that could have happened from a git merge, without someone manually reviewing the result of the merge.
The same thing would have happened if braces were used like this:
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
{ goto fail; }
{ goto fail; }
Only if the braces were on a different line would we either have gotten a
detectable merge conflict, or a second goto operation that did no harm:
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
goto fail;
goto fail;
}
But the thesis of this blog post missed something even more important about this security flaw: WHERE WERE THE TESTS? If there was a unit test covering this function, it would have been immediately obvious that the outcome was not the same as the intention, by way of a test failing as soon as the developer ran the tests himself or an integration server ran the tests.