Naming and the Apple SSL Bug

There is a severe SSL/TLS Vulnerability in iOS and Mac OS X (that is at least fixed in iOS 7.0.6, although not yet in Mac OS X). For a good analysis, look at this article. One recommendation that I would make to improve the code is a simple matter of naming.

Let's look at the code:

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;
}

As you see, the code needs to ensure that SSLFreeBuffer is called on the signedHashes and hashCtx variables, hence they are using goto all over the place. Say what you want about goto, but in absence of a built-in try..finally (there is a Microsoft extension, but, well, that's not standardized), this may be good - it's certainly common to see goto in C code, and if someone knows a good alternative, that would be a benefit for many.

My concern is that there is a label called "fail" that is also used to return success (that is, whenever err is 0).This seems like a bug waiting to happen since it's not actually failing (well, except that it's failing to fail). One idea is to make sure that fail forces a failure state, use a second label for cleanup and jump over the failure in case of success:

if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;

err = sslRawVerify(.....);
if(err) {
    ...
    goto fail;
}

// If we get here without an error, we're golden!
goto cleanup;

fail:
    if(err == 0) err = ERR_SHOULD_NEVER_HAPPEN;
cleanup:
    SSLFreeBuffer(&signedHashes);
    SSLFreeBuffer(&hashCtx);
    return err;

This is still using a whole bunch of goto spaghetti, but it will make sure that fail actually fails, at least with some error state that says "The code did something it shouldn't".

There might also be a case to be made to assume that the function should always return a failure unless it's proven the verification succeeded. Right now, the function does kinda the opposite: It leaves err uninitialized then then runs it through a gauntlet of checks that bail out once err is ever not equal to zero. Bailing out early is good, but I wonder if there should be a secondary variable success that is initialized to false and only set to true in the one spot we know there's not an error.

That could be additional complexity without any real gain though. Personally, I try to write security-related code in a way where bugs tend to make valid cases fail rather than invalid cases pass (in other words: Give valid users too little access instead of invalid users too much), but that's of course easier said than done.

In any case, I highly recommend calling variables, functions, labels after what they actually do, and in this case, fail is an inaccurate label.