goto fail; was unnecessary

Lots has been said about Apple's SSL/TLS bug (see ImperialViolet for a good write-up).

The version with the bug can be found here. The relevant part of the code is:

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
   OSStatus        err;
 
   ...

        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);
   ...

fail:
   SSLFreeBuffer(&signedHashes);
   SSLFreeBuffer(&hashCtx);
   return err;
}

The relevant portion is the duplicated 'goto fail;'. Since the second one is not guarded by the if statement (the indentation is misleading), it is executed unconditionally and the function returns 0, without actually executing sslRawVerify (which does the actually signature checking).

Apple disclosed this vulnerability when it released the latest iOS update with a fix for this bug. A couple of days later, a Mac OS X update was also released.

Lots of people commented on the use of goto, the lack of braces for the if statement, the fact that the compiler or a static checker could have and code review or testing should have caught it. Some good points are made by codecentric. However, I haven't seen any comments that point out that the gotos are actually unneeded here.

Let's take a step back and see what is happening. The sequence of if statements calculates the SHA1 hash of the concatenation of the data in clientRandom, serverRandom and signedParams. The only way that any of the steps in that calculation can fail is when memory is allocated for the hashCtx buffer in ReadyHash().

The functions in the HashReference struct all return an int status, while they should return void (since they shouldn't fail). A quick look at the implementations in tls_digest.c confirms that the only value that is ever returned is 0. If we change return type to void, the code becomes much simpler.

The if statements are no longer needed and the code becomes:

...
    if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
        goto fail;
    SSLHashSHA1.update(&hashCtx, &clientRandom);
    SSLHashSHA1.update(&hashCtx, &serverRandom);
    SSLHashSHA1.update(&hashCtx, &signedParams);
    SSLHashSHA1.final(&hashCtx, &hashOut);
...

The ReadyHash function allocates a buffer, which might fail (in the debug version that will trigger an assert, in the production version it will return -1, not ideal). So it seems that we can't get rid of the remaining goto.

All is not lost, however. The interface exposed by HashReference is general, and flexible. In many cases we would really like a simpler interface, similar to the one-shot functions like CC_SHA1() in the Common Crypto framework (and similar ones in OpenSSL), but with more than one data buffer. If we add an extra function to HashReference to calculate the digest of a number of SSLBuffers in one step, we can avoid exposing the hashCtx:

typedef void (*HashDigest)(SSLBuffer **bufs, unsigned int len, SSLDigest *digest);

typedef struct {
        ...
        HashFinal final;
        HashDigest digest;
} HashReference;


/* The MD5 implementation */
static void HashMD5Digest(SSLBuffer **bufs, unsigned int len, SSLDigest *digest)
{
        MD5_CTX ctx;

        assert(digest->length >= MD5_DIGEST_LENGTH);

        MD5_Init(&ctx);
        for (unsigned int i = 0; i < len; ++i) {
                SSLBuffer *buf = bufs[i];
                assert(fitsInUint32(buf->length));
                MD5_Update(&ctx, buf->data, (uint32_t)buf->length);
        }
        MD5_Final(digest->data, &ctx);
}

Notice that the MD5_CTX is allocated on the stack, so there is no longer a malloc() that can fail. We introduced the SSLDigest type (which has the same layout as an SSLBuffer) to make sure the compiler catches incorrect use (e.g. swapping the digest and data parameters).

Adding a couple of convenience wrappers is straightforward:

void hashDigest1(const HashReference *, SSLBuffer *, SSLDigest *);
void hashDigest2(const HashReference *, SSLBuffer *, SSLBuffer *, SSLDigest *);
void hashDigest3(const HashReference *, SSLBuffer *, SSLBuffer *, SSLBuffer *, SSLDigest *);

The whole sequence of ifs in SSLVerifySignedServerKeyExchange() is now reduced to:

...
        hashDigest3(&SSLHashSHA1, &clientRandom, &serverRandom, &signedParams, &hashOut);
...

There is no place for the 'goto fail;' to hide anymore. As a bonus, this code is faster (only one call through a function pointer of the HashReference struct instead of six).

After removing signedHashes (never used) and hashCtx (no longer used), fixing the log message, removing the wrongly-named fail (it was also reached in the success case) label, and fixing the indentation, the code becomes a bit clearer:

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
        OSStatus err;
        SSLDigest hashOut;
        SSLBuffer clientRandom, serverRandom;
        uint8_t hashes[SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN];
        uint8_t *dataToSign;
        size_t dataToSignLen;

        clientRandom.data = ctx->clientRandom;
        clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
        serverRandom.data = ctx->serverRandom;
        serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;

        if (isRsa) {
                dataToSign = hashes;
                dataToSignLen = SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN;
                hashOut.data = hashes;
                hashOut.length = SSL_MD5_DIGEST_LEN;

                hashDigest3(&SSLHashMD5, &hashOut, &clientRandom, &serverRandom, &signedParams);
        } else {
                /* DSA, ECDSA */
                dataToSign = &hashes[SSL_MD5_DIGEST_LEN];
                dataToSignLen = SSL_SHA1_DIGEST_LEN;
        }

        hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
        hashOut.length = SSL_SHA1_DIGEST_LEN;

        hashDigest3(&SSLHashSHA1, &hashOut, &clientRandom, &serverRandom, &signedParams);

        err = sslRawVerify(ctx,
                           ctx->peerPubKey,
                           dataToSign,    /* plaintext */
                           dataToSignLen,   /* plaintext length */
                           signature,
                           signatureLen);
        if (err) {
                sslErrorLog("SSLVerifySignedServerKeyExchange: sslRawVerify "
                            "returned %d\n", (int)err);
        }
        return err;
}

The code is still not as straightforward as it could be. There are two cases. If isRsa is true, we calculate two digests of the same data, an MD5 and a SHA1, and put them one after the other in hashes. If isRsa is false, we only calculate a SHA1 digest and put that in hashes. The code jumps through some hoops to make sure that the SHA1 is put in the same place in hashes in both cases. The SHA1 calculation can then be shared, but it obscures the intent. Notice that in the first case, calculating two digests is the same thing as calculating one combined digest. So, if we create a helper functions createSSLBuffer and createSSLDigest, we can define HashReference SSLHashMD5SHA1 as follows:

static SSLBuffer
createSSLBuffer(uint8_t *data, size_t len)
{
        return (SSLBuffer){ len, data };
}

static SSLDigest createSSLDigest(uint8_t *data, size_t len); /* similar */

static void HashMD5SHA1Digest(SSLBuffer **bufs, unsigned int len, SSLDigest *md);

static const HashReference SSLHashMD5SHA1 = {
        .digestSize = MD5_DIGEST_LEN + SHA1_DIGEST_LEN,
        ...
        .digest = HashMD5SHA1Digest
};

static void
HashMD5SHA1Digest(SSLBuffer *digest, SSLBuffer **bufs, unsigned int len)
{
        uint32_t md5Len;
        SSLDigest md5, sha1;

        assert(digest->length >= SSLHashMD5SHA1.digestSize);
        md5Len = SSLHashMD5.digestSize;

        md5 = createSSLDigest(digest->data, md5Len);
        SSLHashMD5.digest(bufs, len, &md5);

        sha1 = createSSLDigest(digest->data + md5Len, SSLHashSHA1.digestSize);
        SSLHashSHA1.digest(bufs, len, &sha1);
}

If we extract the function SSLCalculateKeyExchangeHash (which can be used in SSLVerifySignedServerKeyExchangeTls12, SSLSignServerKeyExchangeTls12, SSLSignServerKeyExchange in the same file), the code becomes:

static void
SSLCalculateServerKeyExchangeHash(const HashReference *hash,
                                  SSLContext *ctx, SSLBuffer signedParams,
                                  uint8_t buf[SSL_MAX_DIGEST_LEN])
{
        SSLBuffer clientRandom, serverRandom;
        SSLDigest md;

        assert(hash->digestSize <= SSL_MAX_DIGEST_LEN);

        clientRandom = createSSLBuffer(ctx->clientRandom, SSL_CLIENT_SRVR_RAND_SIZE);
        serverRandom = createSSLBuffer(ctx->serverRandom, SSL_CLIENT_SRVR_RAND_SIZE);
        md = createSSLBuffer(buf, hash->digestSize);
        hashDigest3(hash, &clientRandom, &serverRandom, &signedParams, &md);
}

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
        OSStatus err;
        uint8_t buf[SSL_MAX_DIGEST_LEN];
        const HashReference *hash;

        hash = isRsa ? &SSLHashMD5SHA1 : &SSLHashSHA1;

        SSLCalculateKeyExchangeHash(hash, ctx, signedParams, buf);

        err = sslRawVerify(ctx,
                           ctx->peerPubKey,
                           buf,
                           hash->digestSize,
                           signature,
                           signatureLen);
        if (err) {
                sslErrorLog("SSLVerifySignedServerKeyExchange: sslRawVerify "
                            "returned %d\n", (int)err);
        }
        return err;
}

I think this communicates the intent much more clearly, is easier to test, has fewer places for bugs to hide, and is faster.

The final version of the code in this article (and some tests) is available on github.