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

JSSE Oracle Provider Preference of TLS Cipher Suites

Perference Order Value Description 1 0xC0,0x24 TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 2 0xC0,0x28 TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 3 0x00,0x3D TLS_RSA_WITH_AES_256_CBC_SHA256 4 0xC0,0x26 TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA384 5 0xC0,0x2A TLS_ECDH_RSA_WITH_AES_256_CBC_SHA384 6 0x00,0x6B TLS_DHE_RSA_WITH_AES_256_CBC_SHA256 7 0x00,0x6A TLS_DHE_DSS_WITH_AES_256_CBC_SHA256 8 0xC0,0x0A TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA 9 0xC0,0x14 TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA 10 0x00,0x35 TLS_RSA_WITH_AES_256_CBC_SHA 11 0xC0,0x05 TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA 12 0xC0,0x0F TLS_ECDH_RSA_WITH_AES_256_CBC_SHA 13 0x00,0x39 TLS_DHE_RSA_WITH_AES_256_CBC_SHA 14 0x00,0x38 TLS_DHE_DSS_WITH_AES_256_CBC_SHA 15 0xC0,0x23 TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256 16 0xC0,0x27 TLS_ECDHE_RSA_WITH_AES_1
Read more

JSSE Oracle Provider Default Disabled TLS Cipher Suites

The following TLS cipher suites are supported by Oracle provider, SunJSSE. These cipher suites are disabled by default because of one of the following reasons: obsoleted weak cipher suites anonymous cipher suites no encryption cipher suites (null cipher) Kerberos cipher suites Cipher suites for Kerberos (KRB5) need additional KRB5 service configuration, and these cipher suites are not common in practice. You are NOT supposed to use these cipher suites unless you really know what you're doing from a standpoint. Perference Value Description 1 0x00,0x6D TLS_DH_anon_WITH_AES_256_CBC_SHA256 2 0xC0,0x19 TLS_ECDH_anon_WITH_AES_256_CBC_SHA 3 0x00,0x3A TLS_DH_anon_WITH_AES_256_CBC_SHA 4 0x00,0x6C TLS_DH_anon_WITH_AES_128_CBC_SHA256 5 0xC0,0x18 TLS_ECDH_anon_WITH_AES_128_CBC_SHA 6 0x00,0x34 TLS_DH_anon_WITH_AES_128_CBC_SHA 7 0xC0,0x16 TLS_ECDH_anon_WITH_RC4_128_SHA 8 0x00,0
Read more