David Cantrell

Apple's most recent iOS software update which fixes a horrible security flaw has been all over the interwebs recently. This is yet another post about it. Here's the buggy code:

static OSStatus

SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,

                                 uint8_t *signature, UInt16 signatureLen)

{

    OSStatus        err;

    ...



    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;

    ...



fail:

    SSLFreeBuffer(&signedHashes);

    SSLFreeBuffer(&hashCtx);

    return err;

}

The bug is, of course, the line in red, which isn't conditional - the indentation is misleading. It's probably a cut n paste error. If we start reading from the first 'if' statement, you see that if there is no error, the 'err' variable gets set to 0, and we continue to the second 'if'. Again, this sets the variable to 0 if there's nothing wrong. In both cases, the code then jumps to the 'fail' label if there was a problem. If both checks are OK, the code then jumps to that label anyway (the non-conditional 'goto') and the next 'if' is never executed, so some error conditions are never checked. Finally, the code returns whatever was in the variable - and if we've hit the bug it will return 0, meaning "there was no error". The current version of the code can be found here. I think that this exposes several errors, the bug itself being the least important of them. Let's look at them in detail:

1. Bad naming: If you look at the current code you can see that if no errors are found, the code eventually ends up at the 'fail' label anyway, even though it hasn't failed. The label is therefore misleading. The same advice should apply to label names as to function and variable names: make them meaningful. This one isn't.
2. Bad coding style: In C/C++, conditional code can either be a single statement or a block surrounded by { braces }. In some other languages that have C-like syntax, the braces are mandatory. However, if Apple's coding standard had mandated them then it is likely that the error would have been spotted sooner or would not have mattered, because you'd end up with either something like this:

       if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
   
           goto fail;
   
       }
   
           goto fail;
   
       if ...
   
   

   which at least makes the error a bit more visually distinct, or like this:

       if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
   
           goto fail;
   
           goto fail;
   
       }
   
       if ...
   
   

   which gets rid of the error entirely by making both 'goto's conditional.
3. Bad coding style part 2: All of the error conditions are independent, so you could use 'else if'. Indeed you should use it, because it makes clear that they are independent. Note that this code:

       if(1)
   
           printf("Wibble\n");
   
           printf("Wibble\n");
   
       else if(2)
   
           printf("Wobble\n");
   
   

   is a syntax error because you have an 'else' without an immediately preceding 'if' so not only would it have made the code a bit easier to understand, it would also have made this particular bug obvious when the code failed to compile.
4. Ignoring warnings: Modern compilers will emit warnings when they try to build this code - they'll warn about some of the code being unreachable. Either the developers have turned warnings off (or haven't turned them on - it varies from one compiler to another), or they did have them turned on but ignored them. Warnings exist for a reason. They are always (in my arrogant opinion) a sign that your code has a bug.
5. Lack of unit testing and code coverage checking: I assume that Apple do test their code, but this bug lies very deep down in a complex software stack, and it's always tempting to just hit the public interfaces to make sure the whole stack works. To find this particular bug would require some very carefully crafted tests. A unit-testing culture would be used to writing those sort of tests, and would do it fairly easily. But crafting the input to an end-to-end test to tickle this particular bug would be quite a bit harder. Of course, analysing code coverage from tests would also have found the bug - but unless you are doing unit tests, you're probably used to having your code only partially tested. The bug would mean that only two lines of code weren't covered, and that's just 0.1% of the file. Even unit-testing fanatics will often accept this level of non-coverage even if we really shouldn't. However, I like to think that non-coverage in an area of code as critical as libsecurity_ssl/lib/sslKeyExchange.c might ring an alarm bell or two!