Reflections on Curly Braces – Apple’s SSL Bug and What We Should Learn From It

Everyone’s shaking their heads

First of all, I assume that by now, everyone who has ever read a single tweet in his/her life has heard about Apple’s instantly infamous “gotofail” bug by now, and most of you have probably already read Imperial Violet’s analysis of it.

To sum up the debacle in short: A duplicate line of code, goto fail;, causes a critical SSL certificate verification algorithm to jump out of a series of validations at an unexpected time, causing a success value to be returned, and thus rendering the service vulnerable to attacks.

Bad. To say the least.

Now it seems that people unanimously agree in blaming missing curly braces around the if statement in this piece of code

1if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
2    goto fail;
3    goto fail;

for the entire mess, and the common conclusion from this fiasco is “always put curly braces around your if statements, and this will never happen to you”.

Or will it? I mean, I find it rather curious that everyone seems to blame the mouse, while there’s a giant elephant in the room…

Now let’s look at that code again

Here’s the entire method, taken from Apple’s open source publication :

1static OSStatus
2SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
3                                 uint8_t *signature, UInt16 signatureLen)
4{
5    OSStatus        err;
6    SSLBuffer       hashOut, hashCtx, clientRandom, serverRandom;
7    uint8_t         hashes[SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN];
8    SSLBuffer       signedHashes;
9    uint8_t            *dataToSign;
10    size_t            dataToSignLen;
11 
12    signedHashes.data = 0;
13    hashCtx.data = 0;
14 
15    clientRandom.data = ctx->clientRandom;
16    clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
17    serverRandom.data = ctx->serverRandom;
18    serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
19 
20 
21    if(isRsa) {
22        /* skip this if signing with DSA */
23        dataToSign = hashes;
24        dataToSignLen = SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN;
25        hashOut.data = hashes;
26        hashOut.length = SSL_MD5_DIGEST_LEN;
27 
28        if ((err = ReadyHash(&SSLHashMD5, &hashCtx)) != 0)
29            goto fail;
30        if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0)
31            goto fail;
32        if ((err = SSLHashMD5.update(&hashCtx, &serverRandom)) != 0)
33            goto fail;
34        if ((err = SSLHashMD5.update(&hashCtx, &signedParams)) != 0)
35            goto fail;
36        if ((err = SSLHashMD5.final(&hashCtx, &hashOut)) != 0)
37            goto fail;
38    }
39    else {
40        /* DSA, ECDSA - just use the SHA1 hash */
41        dataToSign = &hashes[SSL_MD5_DIGEST_LEN];
42        dataToSignLen = SSL_SHA1_DIGEST_LEN;
43    }
44 
45    hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
46    hashOut.length = SSL_SHA1_DIGEST_LEN;
47    if ((err = SSLFreeBuffer(&hashCtx)) != 0)
48        goto fail;
49 
50    if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
51        goto fail;
52    if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
53        goto fail;
54    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
55        goto fail;
56    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
57        goto fail;
58        goto fail;
59    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
60        goto fail;
61 
62    err = sslRawVerify(ctx,
63                       ctx->peerPubKey,
64                       dataToSign,                /* plaintext */
65                       dataToSignLen,            /* plaintext length */
66                       signature,
67                       signatureLen);
68    if(err) {
69        sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
70                    "returned %d\n", (int)err);
71        goto fail;
72    }
73 
74fail:
75    SSLFreeBuffer(&signedHashes);
76    SSLFreeBuffer(&hashCtx);
77    return err;
78 
79}

I find it amusing that the first thing anyone would think when looking at this code is “there should have been curly braces”. To make a point, here’s how that would look:

1static OSStatus
2SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
3                                 uint8_t *signature, UInt16 signatureLen)
4{
5    OSStatus        err;
6    SSLBuffer       hashOut, hashCtx, clientRandom, serverRandom;
7    uint8_t         hashes[SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN];
8    SSLBuffer       signedHashes;
9    uint8_t            *dataToSign;
10    size_t            dataToSignLen;
11 
12    signedHashes.data = 0;
13    hashCtx.data = 0;
14 
15    clientRandom.data = ctx->clientRandom;
16    clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
17    serverRandom.data = ctx->serverRandom;
18    serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
19 
20 
21    if(isRsa) {
22        /* skip this if signing with DSA */
23        dataToSign = hashes;
24        dataToSignLen = SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN;
25        hashOut.data = hashes;
26        hashOut.length = SSL_MD5_DIGEST_LEN;
27 
28        if ((err = ReadyHash(&SSLHashMD5, &hashCtx)) != 0) {
29            goto fail;
30        }
31        if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0) {
32            goto fail;
33        }
34        if ((err = SSLHashMD5.update(&hashCtx, &serverRandom)) != 0) {
35            goto fail;
36        }
37        if ((err = SSLHashMD5.update(&hashCtx, &signedParams)) != 0) {
38            goto fail;
39        }
40        if ((err = SSLHashMD5.final(&hashCtx, &hashOut)) != 0) {
41            goto fail;
42        }
43    }
44    else {
45        /* DSA, ECDSA - just use the SHA1 hash */
46        dataToSign = &hashes[SSL_MD5_DIGEST_LEN];
47        dataToSignLen = SSL_SHA1_DIGEST_LEN;
48    }
49 
50    hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
51    hashOut.length = SSL_SHA1_DIGEST_LEN;
52    if ((err = SSLFreeBuffer(&hashCtx)) != 0) {
53        goto fail;
54    }
55    if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0) {
56        goto fail;
57    }
58    if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0) {
59        goto fail;
60    }
61    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) {
62        goto fail;
63    }
64    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
65        goto fail;
66    }
67        goto fail;
68    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
69        goto fail;
70    }
71 
72    err = sslRawVerify(ctx,
73                       ctx->peerPubKey,
74                       dataToSign,                /* plaintext */
75                       dataToSignLen,            /* plaintext length */
76                       signature,
77                       signatureLen);
78    if(err) {
79        sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
80                    "returned %d\n", (int)err);
81        goto fail;
82    }
83 
84fail:
85    SSLFreeBuffer(&signedHashes);
86    SSLFreeBuffer(&hashCtx);
87    return err;
88 
89}

Can you spot the error? I mean: That, obviously, would have made one hell of a difference, wouldn’t it?

What seems to be the problem?

There’s no arguing around the fact that the duplicate line of code is a horrible, horrible bug. But as to how it got there, I beg to differ from the conclusion that everyone else seems to agree on:

Those braces aren’t at fault. A lazy programmer is.

Alright, maybe not entirely. A minor part in this mess can be attributed to the IDE (Xcode, I would assume) not catching the fact that a sizeable portion of the code is unreachable. A modern IDE should really show a warning in such cases, and as Peter Nelson points out, even Xcode seems to have an option for that, though it isn’t on by default – strangely enough, I might add.

But how do we fix it?

Now what can we learn from this? Here’s a number of things we can do to avoid this kind of disaster:

1. Just try the damn thing and see if it works

   Duh. I mean: really. Why wouldn’t you? And since the purpose of this code is obviously not to allow a key exchange, but rather to deny it, if anything is not according to protocol, you should be throwing all the stuff that could be forged at it, not just valid values.

2. Write an automated test

   This is the next obvious step, and like the manual test, it should verify all the possible ways the certificate validation could fail, first of all. Landon Fuller wrote up an example to show it is quite possible to run an integration test against this method.

3. Have someone else review your code

   Another obvious one. This is a hugely critical piece of code at a very, very exposed position in a fundamental part of an operating system – no way this should have ever seen the light of day without at least a second pair of eyes having a look at it. No. Way.

4. Pair program

   One step up from code reviews: Two brains are smarter than one. Four eyes see more than two. Your code will instantly get better in every way if you agree to share ownership of it. Even if you overlook something like this when hacking away at your code, your pairing partner most likely won’t. Also, they might have better ideas on how to do things, such as:

5. Conditionals should express what you actually want to check

   That, to me, is one of the most valuable pieces of advice you can take from Uncle Bob Martin :

   If-statements should encapsulate code that is executed only when a condition is true –
   not jump out of an algorithm or method, if otherwise.

   In this case, instead of employing if(err != 0) and what looks like ten million goto fail; commands, the broken part of the method should have checked for (err == 0), and thus looked at least like this:

   1if ((err = SSLFreeBuffer(&hashCtx)) == 0)
   2     if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) == 0)
   3          if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) == 0)
   4               if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) == 0)
   5                    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) == 0)
   6                         err = SSLHashSHA1.final(&hashCtx, &hashOut);
   7 
   8if (err) 
   9    goto fail;

   which, then, can be simplified even further to

   1if ((err = SSLFreeBuffer(&hashCtx)) == 0 &&
   2    (err = ReadyHash(&SSLHashSHA1, &hashCtx)) == 0 &&
   3    (err = SSLHashSHA1.update(&hashCtx, &clientRandom)) == 0 &&
   4    (err = SSLHashSHA1.update(&hashCtx, &serverRandom)) == 0 &&
   5    (err = SSLHashSHA1.update(&hashCtx, &signedParams)) == 0 )
   6        err = SSLHashSHA1.final(&hashCtx, &hashOut);
   7 
   8if (err) 
   9    goto fail;

   Notice how this kind of structure shows what we really want to do: Execute a sequence of steps, and proceed to the next step only if the current step did not return an error. If it did, then any subsequent statements won’t execute, and if err is not 0 after the whole block, there’s one goto fail;, which also states the programmer’s original intent more precisely: If anything went wrong, exit the method.

6. Don’t copy and paste code

   The most blatant thing I noticed when I glanced over the rest of the source file that contains the bug is the amount of duplicate or nearly duplicate code that can be found. Clearly, someone tried to go the easy way and copied/pasted the same code all over the place. I found minor variations of the defective if-goto-sequence

   1if ((err = SSLFreeBuffer(&hashCtx)) != 0)
   2        goto fail;
   3    if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
   4        goto fail;
   5    if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
   6        goto fail;
   7    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
   8        goto fail;
   9    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
   10        goto fail;
   11    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
   12        goto fail;

   in at least 5 places – all virtually identical, and all equally horrid; a definite sign of gratuitous and unreflected use of copy and paste.

   In fact, you can eliminate the bug, its cause and about a third of the code in the SSLVerifySignedServerKeyExchange() method by extracting this rather uniform sequence of calls to a HashReference into its own method:

   1static OSStatus SSLApplyHash( const HashReference *hash,
   2                             SSLBuffer *hashCtx,
   3                             SSLBuffer *clientRandom,
   4                             SSLBuffer *serverRandom,
   5                             SSLBuffer *signedParams,
   6                             SSLBuffer *hashOut) {
   7    OSStatus        err;
   8    if ((err = SSLFreeBuffer(hashCtx)) == 0 &&
   9        (err = ReadyHash(hash, hashCtx)) == 0 &&
   10        (err = hash->update(hashCtx, clientRandom)) == 0 &&
   11        (err = hash->update(hashCtx, serverRandom)) == 0 &&
   12        (err = hash->update(hashCtx, signedParams)) == 0 )
   13        err = hash->final(hashCtx, hashOut);
   14    return err;
   15}

   which can then be called from anywhere using a single line, such as:

   1err = SSLApplyHash(&SSLHashMD5, &hashCtx, &clientRandom, &serverRandom, &signedParams, &hashOut);

   I bet that would eliminate at least 50 lines of crappy code from the source file.

   [Update]
   As pg (see comments section) points out, cleaning up this part of the code should have gone even further. Please refer to his very good post to find out just how far.

7. Make your methods do only one thing

   If you put it in natural language, the SSLVerifySignedServerKeyExchange() method “does a lot of magical things with hashes (no need to get into details), which need to be done in a specific order, but only if all steps in the sequence run without error, and with slight differences depending on which kind of keys are used, and log errors otherwise.”
   Clearly, that is quite a lot more than one thing. And that is a very distinct indicator that it should be significantly shorter, and more precise.
   To me, it should really be split up into several individual methods:

   • The above-mentioned SSLApplyHash utility method
   • A utility method to initialize SSLBuffer variables from the passed in context data, to be used somewhat like this:
     SSLBuffer clientRandom = SSLRandomBuffer( ctx->clientRandom );
   • Two more methods to encapsulate the RSA and non-RSA execution paths
   • Finally, the original API method, which should basically only decide whether to execute the RSA or non-RSA paths, and write the log message in case of an error.

   Apart from getting rid of a lot of code duplication (not just within this method, but in many other places within the file, as well), this would make the algorithm a lot more readable and thus overlooking errors far less likely.

8. Use readable, descriptive variable names

   Try to explain the meaning of these without reading the context of the surrounding code:
   hashes, hashOut, hashCtx, ctx, clientRandom, serverRandom, signedParams, dataToSign.

   Wouldn’t it have been a lot more understandable to call them, say,
   appliedHashResults, hashOutputBuffer, tmpHashContext, sslContext, randomClientValueForSharedSecret, randomServerValueForSharedSecret, messageToSign?

   And these were only the first quick ideas I came up with while writing this paragraph, not even a well-reflected choice of names that originated from hours of work on the code that contains these variables…

Conclusion / tl;dr

To make it clear once again: This should never have happened.

It shouldn’t have happened, because this bug is so obvious, that any reasonable amount of scrutiny, which should always be applied to critical pieces of code, will catch it.

It shouldn’t have happened, because even if the humans missed it, the tooling should have complained.

It shouldn’t have happened, because a simple test would have shown that the code never actually did what it was intended to do.

But first and foremost, it shouldn’t have happened, because making the algorithm terse and readable would have forced the programmer to think about flow, structure and intent of his/her code more thoroughly, and at that point, the superfluous goto fail; would have stuck out like a sore thumb.

There’s only one thing to blame here, and that has nothing to do with code style (as which I would consider where the brackets and braces go, whether to add empty lines, etc. etc.), but rather with craftsmanship and professional attitude.

And I have to say it: It’s not worthy of a company like Apple, which prides itself to sell only the highest quality products, to produce such sloppy, unchecked, untested and obviously uncared for code, least of all in a critical part of the foundations of the operating system that runs on all of its devices.