apple's bad coding guidelines...or: why you should always use squiggly brackets with if-then-else|xyzzy xyzzy|you are in a twisty maze of little passages... there's a link a here.

On Friday Apple released an iOS bug fix for an issue with their SSL implementation. Adam Langley analyzed the patch and found out that the same problem is also present in the Mac OSX implementation. For so far unknown reasons (though the rumors range from merge error to NSA induced bug) a goto fail; line got duplicated (highlighted below):

    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;

(code excerpt taken from Apple’s open-sourced code)

The effect is that the sslRawVerify code never gets called — allowing an easy MITM attack on iPhone, iPads, Macs. Unfortunately, Apple did something rather stupid and only released the iOS patch and not concurrently the Mac OSX patch. As a consequence all Maverick Macs are currently potential victims…

Anyhow, had Apple enforced cautious coding guidelines and mandated that if-then-else blocks always have to use squiggly braces, the code in question would not have been an issue:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
        goto fail;
        goto fail;
}

By encapsulating the then-block, the second goto fail would have been dead code and nothing would have happened (the compiler might even have flagged this).

So: always use squiggly braces for your if-then-else statements!