Bruce Schneier asks today whether the recently discovered IOS SSL programming flaw might have been intentionally inserted to
cause a security weakness. This is a legitimate question, although
the error is likely to have been caused by an accidental extra
keypress. Apple should be able to determine exactly who inserted the
error, and chances are that they will clear the developer of malice.
As a software developer, I wish to take
issue with one of Schneier’s comments. He felt that the error would
be hard to spot. I disagree. An experienced programmer ought to see
the error at once. The only question is whether the code would ever
be reviewed.
But this error could also be flagged by
a program that examined source code for unexecutable lines.
There are many such test programs, and they might, while checking
millions of lines of source in Apple’s repository, notice this
problem. It ought to be routine to run such a test.
Here is the source code. (I’m quoting it from the Guardian, and my formatting might not match the
original.) Below, the duplicate “goto fail” lines stand out as a
stark error. (I aded the red to make it easy for you to find it.) The “if” statement below the duplicate “goto fail”
lines, which is needed, will never be executed.
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;
}There’s another issue. I showed this code to my wife, who has suffered through my life-long romance with software development. She knows little about programming, but I thought, correctly, that she would understand this error. She did, and she had another comment:
“I thought ‘goto’ programming was a thing of the past.”
And she’s almost right. “Goto” programming has been recognized as bad, hard-to-read and hard-to-get-right. Most programming languages offer much better alternatives. But in many programming languages, the “goto” continues to be supported because it is still the best way, even the clearest way, to unravel some complex progamming situations.
The “goto” statements are completely unnecessary in this case, and if the developer had written the code in a gotoless way, it is more likely that the result would have been error-free. For example:
if (((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
|| ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
|| ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)))
{
// process the fail here ...
}... etc...
