Lack of Obvious Test Case Considered Harmful (Was: GOTO Considered Harmful)
Here's the original piece of ... code from Apple. If I had been the code reviewer, I would have sent the coder back to the drawing board to eliminate the "goto" statements. As a tester, I would have insisted on reproducible test suites that hit every line of code.
Actually the lack of a test case is the real problem with this code. The use of "goto" is not the most important issue. I never use goto myself, because it makes it much harder to refactor functions. However, the biggest problem here is that the producers and distributors of this code never ran a test case which actually called the sslRawVerify function! Considering that the whole point of the code is to bloody well call sslRawVerify, it baffles me how this could have gone unnoticed. I know I should "never attribute to malice that which can be adequately explained by stupidity," but I really wonder if this can be adequately explained by stupidity.
Listen, I know we're all human and I can understand making errors. I've made errors. I can think of two that had memorably bad consequences. I don't care to repeat that experience. There are worse things in life than losing money, but not many. The baffling thing about this error is that it's in code that does security, and it's right at the heart of many products that millions of people use, and was produced by a huge corporation, and yet there was not a single test case that hit the sslRawVerify call. My trust in the software stack, especially anything related to SSL, is highly diminished.
Notice the two "goto fail" lines in a row. That's the bug, which may have been inserted deliberately to create a security vulnerability.
Then scroll down to see my refactored version.
Original Code from Apple
/*
* Copyright (c) 1999-2001,2005-2012 Apple Inc. All Rights Reserved.
*
* @APPLE_LICENSE_HEADER_START@
*
* This file contains Original Code and/or Modifications of Original Code
* as defined in and that are subject to the Apple Public Source License
* Version 2.0 (the 'License'). You may not use this file except in
* compliance with the License. Please obtain a copy of the License at
* http://www.opensource.apple.com/apsl/ and read it before using this
* file.
*
* The Original Code and all software distributed under the License are
* distributed on an 'AS IS' basis, WITHOUT WARRANTY OF ANY KIND, EITHER
* EXPRESS OR IMPLIED, AND APPLE HEREBY DISCLAIMS ALL SUCH WARRANTIES,
* INCLUDING WITHOUT LIMITATION, ANY WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE, QUIET ENJOYMENT OR NON-INFRINGEMENT.
* Please see the License for the specific language governing rights and
* limitations under the License.
*
* @APPLE_LICENSE_HEADER_END@
*/
static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
uint8_t *signature, UInt16 signatureLen)
{
OSStatus err;
SSLBuffer hashOut, hashCtx, clientRandom, serverRandom;
uint8_t hashes[SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN];
SSLBuffer signedHashes;
uint8_t *dataToSign;
size_t dataToSignLen;
signedHashes.data = 0;
hashCtx.data = 0;
clientRandom.data = ctx->clientRandom;
clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
serverRandom.data = ctx->serverRandom;
serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
if(isRsa) {
/* skip this if signing with DSA */
dataToSign = hashes;
dataToSignLen = SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN;
hashOut.data = hashes;
hashOut.length = SSL_MD5_DIGEST_LEN;
if ((err = ReadyHash(&SSLHashMD5, &hashCtx)) != 0)
goto fail;
if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0)
goto fail;
if ((err = SSLHashMD5.update(&hashCtx, &serverRandom)) != 0)
goto fail;
if ((err = SSLHashMD5.update(&hashCtx, &signedParams)) != 0)
goto fail;
if ((err = SSLHashMD5.final(&hashCtx, &hashOut)) != 0)
goto fail;
}
else {
/* DSA, ECDSA - just use the SHA1 hash */
dataToSign = &hashes[SSL_MD5_DIGEST_LEN];
dataToSignLen = SSL_SHA1_DIGEST_LEN;
}
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;
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;
}
My Version, Refactored to avoid "goto"
I systematically refactored Apple's code to remove the gotos. It took me, what, 15 minutes? I even included a handy flag "insert_bug_for_NSA" in case you really want the security vulnerability.
Note well: All I have done is refactor Apple's code into a form that does not involve "goto". If each of my refactoring steps was correct, then the resulting code should have exactly the same behavior as the original. That means the bug is still there, except now it's easier to spot. Scroll down to the third section below to see the code with the bug removed.
static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
uint8_t *signature, UInt16 signatureLen)
{
OSStatus err;
SSLBuffer hashOut, hashCtx, clientRandom, serverRandom;
uint8_t hashes[SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN];
SSLBuffer signedHashes;
uint8_t *dataToSign;
size_t dataToSignLen;
signedHashes.data = 0;
hashCtx.data = 0;
clientRandom.data = ctx->clientRandom;
clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
serverRandom.data = ctx->serverRandom;
serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
int insert_bug_for_NSA = 1; /* option flag */
if(isRsa) {
/* skip this if signing with DSA */
dataToSign = hashes;
dataToSignLen = SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN;
hashOut.data = hashes;
hashOut.length = SSL_MD5_DIGEST_LEN;
err = ReadyHash(&SSLHashMD5, &hashCtx);
if (err == 0) err = SSLHashMD5.update(&hashCtx, &clientRandom);
if (err == 0) err = SSLHashMD5.update(&hashCtx, &serverRandom);
if (err == 0) err = SSLHashMD5.update(&hashCtx, &signedParams);
if (err == 0) err = SSLHashMD5.final(&hashCtx, &hashOut);
}
else {
/* DSA, ECDSA - just use the SHA1 hash */
dataToSign = &hashes[SSL_MD5_DIGEST_LEN];
dataToSignLen = SSL_SHA1_DIGEST_LEN;
err = 0;
}
if (err == 0)
{
hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;
err = SSLFreeBuffer(&hashCtx);
}
if (err == 0) err = ReadyHash(&SSLHashSHA1, &hashCtx);
if (err == 0) err = SSLHashSHA1.update(&hashCtx, &clientRandom);
if (err == 0) err = SSLHashSHA1.update(&hashCtx, &serverRandom);
if (err == 0) err = SSLHashSHA1.update(&hashCtx, &signedParams);
int deliberately_skip_the_raw_verify = 0;
if (err == 0)
{
if (insert_bug_for_NSA)
deliberately_skip_the_raw_verify = 1;
}
if (!deliberately_skip_the_raw_verify)
{
if (err == 0)
err = SSLHashSHA1.final(&hashCtx, &hashOut);
if (err == 0)
{
err = sslRawVerify(ctx,
ctx->peerPubKey,
dataToSign, /* plaintext */
dataToSignLen, /* plaintext length */
signature,
signatureLen);
if (err != 0)
{
sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
"returned %d\n", (int)err);
}
}
}
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;
}
My Version, Fixed to Avoid Apple's Bug
Here I remove Apple's bug by setting insert_bug_for_NSA to 0 and refactoring accordingly.
static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
uint8_t *signature, UInt16 signatureLen)
{
OSStatus err;
SSLBuffer hashOut, hashCtx, clientRandom, serverRandom;
uint8_t hashes[SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN];
SSLBuffer signedHashes;
uint8_t *dataToSign;
size_t dataToSignLen;
signedHashes.data = 0;
hashCtx.data = 0;
clientRandom.data = ctx->clientRandom;
clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
serverRandom.data = ctx->serverRandom;
serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
if(isRsa) {
/* skip this if signing with DSA */
dataToSign = hashes;
dataToSignLen = SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN;
hashOut.data = hashes;
hashOut.length = SSL_MD5_DIGEST_LEN;
err = ReadyHash(&SSLHashMD5, &hashCtx);
if (err == 0) err = SSLHashMD5.update(&hashCtx, &clientRandom);
if (err == 0) err = SSLHashMD5.update(&hashCtx, &serverRandom);
if (err == 0) err = SSLHashMD5.update(&hashCtx, &signedParams);
if (err == 0) err = SSLHashMD5.final(&hashCtx, &hashOut);
}
else {
/* DSA, ECDSA - just use the SHA1 hash */
dataToSign = &hashes[SSL_MD5_DIGEST_LEN];
dataToSignLen = SSL_SHA1_DIGEST_LEN;
err = 0;
}
if (err == 0)
{
hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;
err = SSLFreeBuffer(&hashCtx);
}
if (err == 0) err = ReadyHash(&SSLHashSHA1, &hashCtx);
if (err == 0) err = SSLHashSHA1.update(&hashCtx, &clientRandom);
if (err == 0) err = SSLHashSHA1.update(&hashCtx, &serverRandom);
if (err == 0) err = SSLHashSHA1.update(&hashCtx, &signedParams);
if (err == 0) err = SSLHashSHA1.final(&hashCtx, &hashOut);
if (err == 0)
{
err = sslRawVerify(ctx,
ctx->peerPubKey,
dataToSign, /* plaintext */
dataToSignLen, /* plaintext length */
signature,
signatureLen);
if (err != 0)
{
sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
"returned %d\n", (int)err);
}
}
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;
}
Another Suggestion
Jerry Leichter on the crypto list suggested a minimal change to the original code (i.e. no massive refactoring) which would have caught the error. He says "As far as I'm concerned, the best recommendation here, which avoids changing code that's worked correctly for years prior to this bug, is to add a sanity check:"
... goto cleanup; fail: assert(err != 0); // Whatever you want here to die *in production code*! cleanup: SSLFreeBuffer(...) ... return err;
Another Suggestion
If I were writing this from scratch, I might consider using an error flag inside the SSLBuffer and SSLContext structures. Each function would check the error flag and return immediately if set. Then you could write more straight-line code without having to check errors in the calling code every time.
ReadyHash(&SSLHashMD5, &hashCtx); SSLHashMD5.update(&hashCtx, &clientRandom); SSLHashMD5.update(&hashCtx, &serverRandom); SSLHashMD5.update(&hashCtx, &signedParams); SSLHashMD5.final(&hashCtx, &hashOut); ... return ctx->err;
Weird thing is, I don't see the obvious connection between hashOut and any of the parameters passed to sslRawVerify. Hmm.
See Also
The Apple TSL bug, and coding guidelines
2014-02-28