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

Java™ SE 7 Release Security Enhancements - Weak Cryptography Control

Weak cryptographic algorithms can now be disabled in Java SE 7 release. The MD2 Message-Digest Algorithm was disabled by default in Sun PKIX provider and SunJSSE provider. The MD2 algorithm is a cryptographic hash function developed by Ronald Rivest in 1989 , and was published in 1992 as an Informational RFC (RFC 1319) .; RFC 6149 moves RFC 1319/MD2 to historic status, "Since its publication, MD2 has been shown to not be collision-free, albeit successful collision attacks for properly implemented MD2 are not that damaging. Successful pre-image and second pre-image attacks against MD2 have been shown." Although MD2 is no longer considered secure, it remains in use in public key infrastructures as part of certificates generated with MD2 and RSA. An a countermeasure of the vulnerability, Java SE has disabled MD2 algorithm in certification path building and validation. You may wonder, Java SE has disabled MD2 algorithm in certification path building and validation in ...
Read more

JEP 115: AES-GCM CipherSuites in JDK 8

Image
Chengdu, China RFC 5288 describes the use of AES in Galois Counter Mode ( GCM ) (AES-GCM) with various key exchange mechanisms as a cipher suite for TLS. AES-GCM is an authenticated encryption with associated data (AEAD) cipher (as defined in TLS 1.2 ) providing both confidentiality and data origin authentication. Java SE had already defined the AES-GCM interfaces in Java SE 7 .  In the coming Java SE 8, as an implementation of JEP 115 , AES-GCM algorithms is implemented in SunJCE provider, and AES-GCM cipher suites are implemented in SunJSSE provider. The following SSL/TLS AEAD/GCM cipher suites, in preference order, are enabled by default in SunJSSE provider for TLS version 1.2: TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 (RFC 5289) TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 (RFC 5289) TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (RFC 5289) TLS_RSA_WITH_AES_256_GCM_SHA384 (RFC 5288) TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384 (RFC 5289) TLS_ECDH_RSA_WITH_AES_...
Read more