Use Braces Even For Single Line Statement



On Feb. 21, 2014, Apple released security update for iOS that affected SSL/TLS connections. The impact is described as "An attacker with a privileged network position may capture or modify data in sessions protected by SSL/TLS." And the CVSS v2 Base Score is 6.8(AV:N/AC:M/Au:N/C:P/I:P/A:P). What's the problem with it?

Here is the Apple code:
static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
    ...

    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;

}

Did you note these lines?
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
        goto fail;
        goto fail;

There are two "goto fail;" lines. The second one will always be executed and jump to "fail" marked block. Therefore, there is no chance to execute the sslRawVerify() method, which is essential to establish a safe SSL connection.  Here comes the security issue. Read here if you want to learn more detailed analysis of the issue.

The lesson I learned:
1. Be patient during code review. Even a minor typo can result in serious security issues.
2. Follow good code conversions. People make fewer mistakes in consistent environments.

Java suggests to always to use braces for "if" statements. C and C++ also have similar conversions, for example the brace policy in C and C++.
Note: if statements always use braces, {}. Avoid the following error-prone form:

if (condition) //AVOID! THIS OMITS THE BRACES {}!
    statement;
Rewrite the Apple code with the braces policy:
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
        goto fail;
    }
    goto fail;
With this reorg, it is easier to find the problem during code review. Or
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
        goto fail;
        goto fail;
    }
With this reorg, the problem disappears.

Good code conversions make a lot of fun!

Popular posts from this blog

Java™ SE 7 Release Security Enhancements - Weak Cryptography Control

JSSE Oracle Provider Preference of TLS Cipher Suites

JEP 114: TLS SNI Extension - SunJSSE Behavior Changes