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

TLS Server Name Indication Extension and Unrecognized_name

Image
Heavy Pond, Jianfengling National Forest Park, Ledong, Hainan, China It's getting hot that some TLS/HTTPS server failed with "unrecognized_name". For example, the Adobe AIR 3 Code Signing Certificate Problem , the ADT handshake alert , and the jarsigner issue with timestamp.geotrust.com , etc. This entry will discussion some background of the "unrecognized_name" alert, and the TLS Server Name Indication (SNI) extension. Background "Unrecognized_name" is an error alert, define by RFC4366 .  In section 4 of RFC4366 : - "unrecognized_name": this alert is sent by servers that receive a server_name extension request, but do not recognize the server name. This message MAY be fatal. And in section 3.1 of of RFC4366 : If the server understood the client hello extension but does not recognize the server name, it SHOULD send an "unrecognized_name" alert (which MAY be fatal). From above sections, we see that "unrecognized...
Read more

NIST Security Strength Time Frames

Security Strength Time Frames of NIST SP 800-57 Part 1 Security Strength 80 112 128 192 256 applying processing applying processing through 2010 acceptable acceptable acceptable acceptable acceptable acceptable acceptable 2011 through2013 deprecated legacy use acceptable acceptable acceptable acceptable acceptable 2014 through 2030 disallowed legacy use acceptable acceptable acceptable acceptable acceptable 2031 and Beyond disallowed legacy use disallowed legacy use acceptable acceptable acceptable Symmetric Algorithms 2TDEA 3TDEA AES-128 AES-192 AES-256 FFC (e.g., DSA, D-H) L = 1024 N = 160 L = 2048 N = 224 L = 3072 N = 256 L = 7680 N = 384 L = 15360 N = 512 IFC (e.g., RSA) k = 1024 k = 2048 k = 3072 k = 7680 k = 15360 ECC (e.g.,ECDSA) f = 160-223 f = 224-255 f = 256-383 f = 384-511 f = 512+ Digital Signatures and hash-only applications SHA-1, SHA-224, SHA-256, SHA-384, SHA-5...
Read more