SSL Failure because of a Copy-Paste typo.
Apple's been in the news today because of a recent OS update to fix an SSL problem.
http://opensource.apple.com/source/Security/Security-55471/libsecurity_ssl/lib/sslKeyExchange.c?txt
retrieved 2/22/2014
in the function SSLEncodeSignedServerKeyExchange() ends with the lines:
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;
err = sslRawVerify(ctx,
ctx->peerPubKey,
dataToSign, /* plaintext */
dataToSignLen, /* plaintext length */
signature,
signatureLen);
if(err) {
sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
"returned %d\n", (int)err);
goto fail;
}
fail:
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;
}
When I'm writing in
C I never write my code this way.
- There should be a unit test to verify that this routine behaves correctly with known good values.
- There should be a unit test to verify that this routine behaves correctly, i.e, returns an error, with known bad values.
- An automated code analyzer should have warned about the unreachable code after the extra goto.
- In my if statements, I never write
bare then statements. I always put the then part of the if in
{ curly braces }.
- When I'm calling a series of subroutines that return error codes, I use a structure I call an error ladder:
I write code like this:
if (noErr == err) { err = ReadyHash(&SSLHashSHA1, &hashCtx); }
if (noErr == err) { err = SSLHashSHA1.update(&hashCtx, &clientRandom); }
if (noErr == err) { err = SSLHashSHA1.update(&hashCtx, &serverRandom); }
if (noErr == err) { err = SSLHashSHA1.update(&hashCtx, &signedParams); }
if (noErr == err) { err = SSLHashSHA1.final(&hashCtx, &hashOut); }
if (noErr == err) { err = sslRawVerify(ctx,
ctx->peerPubKey,
dataToSign, /* plaintext */
dataToSignLen, /* plaintext length */
signature,
signatureLen);
}
/* error cleanup */
if (noErr != err) {
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
}
return err;
Error Ladder
An Error Ladder is a series of if statements that
test a status code variable, and if it's good, set it with the error status return of the next step.
OSStatus err = noErr;
if (noErr == err) { err = FunA(); }
if (noErr == err) { err = FunB(); }
...
if (noErr == err) { err = FunC(); }
/* error cleanup */
if (noErr != err) {
Cleanup();
}
return err;
The programmer counter walks down the ladder rung by rung,
until the first error occurs, then falls through to the bottom.
The uniformity of the line prefixes makes it easy to read.
Some Apple sample code uses a macro that expand to
if (noErr != (err = OPERATION(ARGS))) goto fail; but, since
that macro isn't a standard part of the language, whenever I see it in a new-to-me part
of the source code I have to double check its definition. Further the goto label is
built in to the macro, so I have to read the definition to to see its name this time.
One criticism: in the rare case an error does occur, we'll be executing the predicate
(noErr == err) many times redundantly. Isn't that inefficient?
My answer: in the usual case, where no error occurs, we are testing the variable
err exactly as many times as we should. Errors should be rare,
and when they do occur the extra tests can't make any significant difference to the run time.
https://twitter.com/i0n1c/status/437181648698765313 pointed me at the source of Apple's error.