The IOS goto fail Vulnerability and Code Practices

By now most everyone has read about the IOS vulnerability that was fixed last Friday. Because that portion of code is open source, it didn’t take long for the security community to track down the offending issue and bring it to the light of day. The majority of comments I have read have wondered how such a simple break could make it out in to the wild. I thought it would be fun to speculate as to how it happened and also to do a little code review. A good in depth review of the bug and it’s implications from a user standpoint is here.

*Disclaimer – I don’t consider myself better or above mistakes like this. The code I write today is as good as I understand the technology today, code I wrote last week is always not as good as it could be now. We are constantly learning as developers and continually making our code better. Reviewing the code we wrote last week and contemplating ways to make it better are all a part of becoming that better developer we all strive to be.

As developers we have come up with some best practices when coding, rules of thumb that we should follow. Most of these practices are debated and fought over like religious battles. They are usually a tug of war between readability, succinctness and performance. Most of the battles are grey areas. There is little black and white to software design and architecture. Code statements themselves are black and white, it either works or doesn’t. The composition of statements into useful code is still more art than science. Every program is unique with different user stories and different behaviors.

This code is interesting piece to review because in my opinion not following best practices led to this vulnerability. Individually they do not present a clear issue, but in tandem they allowed a single extra line of code to hide in the file unnoticed by compilers or humans. 99% of the time following the best practice is the best approach because doing so reduces the chances of bad stuff^tm happening, but 1% of the time there is good reason not to. Knowing when is the key.

Following is the code and following that are the practices it breaks.

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
    OSStatus        err;
    SSLBuffer       hashOut, hashCtx, clientRandom, serverRandom;
    uint8_t         hashes[SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN];
    SSLBuffer       signedHashes;
    uint8_t   *dataToSign;
 size_t   dataToSignLen;

 signedHashes.data = 0;
    hashCtx.data = 0;

    clientRandom.data = ctx->clientRandom;
    clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
    serverRandom.data = ctx->serverRandom;
    serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;


 if(isRsa) {
  /* skip this if signing with DSA */
  dataToSign = hashes;
  dataToSignLen = SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN;
  hashOut.data = hashes;
  hashOut.length = SSL_MD5_DIGEST_LEN;
  
  if ((err = ReadyHash(&SSLHashMD5, &hashCtx)) != 0)
   goto fail;
  if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0)
   goto fail;
  if ((err = SSLHashMD5.update(&hashCtx, &serverRandom)) != 0)
   goto fail;
  if ((err = SSLHashMD5.update(&hashCtx, &signedParams)) != 0)
   goto fail;
  if ((err = SSLHashMD5.final(&hashCtx, &hashOut)) != 0)
   goto fail;
 }
 else {
  /* DSA, ECDSA - just use the SHA1 hash */
  dataToSign = &hashes[SSL_MD5_DIGEST_LEN];
  dataToSignLen = SSL_SHA1_DIGEST_LEN;
 }

 hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
    hashOut.length = SSL_SHA1_DIGEST_LEN;
    if ((err = SSLFreeBuffer(&hashCtx)) != 0)
        goto fail;

    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; //here is the problem
    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;

}

Let’s start with a quick discussion of what the code is doing and why it fails. The interesting bits are the if statements from the ReadyHash call down. Basically the code is creating a SHA1 key that it will verify. After the third SSLHashSHA1.update() call the goto fail statement is repeated. This was either caused by the deletion of another function call or simply the duplication of the line. The result is that the implicit code block for that if statement is only the single next statement. The second goto is executed no matter the outcome of the statement above. The variable err is updated in each if statement and returned at the end. Because no error condition actually existed when it hits the extra goto statement, it returns the success result of the previous function.

Each of the best practices not followed are below.

Use of goto

The saying we typically use is never use goto. I don’t like to say never. If  you have to use goto you are probably doing something wrong. Sometimes however, it can make sense. In this case it’s used to short circuit several tests throughout the function and this pattern is used throughout other functions in the class. Each goto statement and it’s corresponding label are contained within the function. Each function still starts at the top and ends at the bottom with the return statement. It’s much better than a complicated nested if statement or compound if statements. This is the easy thing to point to as the major cause of this flaw because of our general rule about using goto, but I don’t think so. I believe it’s warranted here. It makes it extremely easy to read and understand what is happening and why. The goto statements do not jump around creating the spaghetti code, they just skip to the bottom of the function and bypass additional checks if one above does not pass.

Not putting blocks with if statements

When an if statement is not followed by curly braces to create a code block, the single next command is executed if true. Writing if statements without curly braces can make the code more succinct. Adding braces would have added 11 lines of code to this one function. In my opinion using braces with one statement better expresses the logic of the code. The extra typing and extra space it takes is more than justified by the gains in readability and explicitness of the code. If there were a code block here chances are the second goto would have been enclosed in that code block and the issue would have been avoided. At the very least, if an if statement were deleted it would have been noticeable that a code block with braces remained. In my opinion this was the best practice that would have had the best chance of preventing the issue.

The naming convention is confusing

At first glance the code block makes sense. If there is an error it goes to the fail tag and exits the function. But the fail tag in this case doesn’t truly mean fail, it means exit. The return code is being set by an assign statement within the if statement. The first thing you would think when looking at this error for the first time would be that it’s going to always fail because it will always “goto fail”. The first thing you should think is it is always going to “goto exit” and the verify function will never be called. The fail label doesn’t accurately portray what the code below it is doing and should be called exit, endfunction, cleanup or something of that nature. This would not fix the issue, but it would make the intentions of the code clear. In fact, err is misleading as well. The err variable is also used for a success status.

 Proper indentation

This code is pulled from GitHub and I’m not sure what editor is used to edit the code. It’s entirely possible this is a non-issue in the IDE that the developers use. I do see the issue when browsing the code online as well as when it’s downloaded to a Windows machine. Obviously the line in question is a mistake and is not properly indented because of the mistake, but several lines of code are not indented properly and can cause confusion. Improper indentation can lead to easily misread code. 
Edit: The code formatter I am using does not properly show the indentation. It is viewable in the source for sslKeyExchange.c.

It is easy to misinterpret:

    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;
 }

as

        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;
 }
}

      Not unit testing

Unit tests are designed to catch this exact issue. Two unit tests would have found this issue immediately, one checking for successful results and one checking for errors. This would have been an easy catch. Unit testing would not prevent the bug, but it would catch it before promoting the code.

Not using an IDE that highlighted the issue or ignoring the warning if the IDE did give one.

I have not researched this to find out what IDE they use, so it’s impossible for me to say they ignored a warning or simply weren’t given one. Visual Studio will give a warning if compiling code with the above issue. Alan Coopersmith has an interesting blog post about this and shows that gcc as of at least 4.5.2 will not give a warning for unreachable code using the default settings. Specifying –Wall (give me all warnings) still doesn’t warn on unreachable code. –Weverything does, but also gives so many other warnings that it is typically not used.

Scope in JavaScript

Scope in JavaScript can be difficult to understand if you come from a block scoping language like C# or Java, but a few simple examples can make sense of it. This is a very simplistic explanation and glosses over the how and why, but does give good examples of the behavior to expect.

Braces do not determine scope in JavaScript, function definitions do.

function someFunction() {
    var a = 1;
    //prints 1
    console.log(a);
}
someFunction();
//undefined exception
console.log(a);

As you can see, a is visible within the function, but not outside.

function someFunction() {
    if (true) {
        var a = 1;
    }
    //prints 1
    console.log(a);
}
someFunction();
//undefined exception
console.log(a);

In the above snippet, b is visible outside the scope of the if statement, but is still visible within the function declaration, but not outside of the function.

Variables declared outside functions are visible from within.

//declare in global scope
var a = 1;
function someFunction() {
    //print 1
    console.log(a);   
}
someFunction();
//also prints 1
console.log(a);

Variable references can travel up the function chain as far as needed to find the declaration.

var a = 1;
function someFunction1() {
    function someFunction2() {
        //prints 1
        console.log(a);
    }
    someFunction2();
}
someFunction1();

But cannot travel back down the function chain at any step.

function someFunction1() {
    var a = 1;
    //prints 1
    console.log(a);
}
function someFunction2() {
    var b = 1;
    //prints 1
    console.log(b);
    //undefined exception
    console.log(a);
}
//undefined exception
console.log(a);
//undefined exception
console.log(b);

If two variables in the chain are declared and named the same, they both exist and are visible within their own scope.

//declared globally
var a = 1;
function someFunction() {
    //declared locally
    var a = 2;
    //prints 2 from local variable
    console.log(a);
}
someFunction();
//prints 1 from global variable
console.log(a);

If two variables are declared and named the same in the same scope, the second overwrites the first.

function someFunction() {
    var a = 1;
    //prints 1
    console.log(a);
    var a = 2;
    //prints 2
    console.log(a);
}
someFunction();

No matter where a variable is declared, if it is declared without the var keyword, it is scoped globally.

function someFunction() {
    a = 1;
    //prints 1
    console.log(a);
}
someFunction();
//prints 1
console.log(a);

Passed parameters are local in scope

function someFunction(a) {
    //prints 1
    console.log(a);
}
someFunction(1);
//undefined exception
console.log(a);

Passed parameters are still available in the chain

function someFunction(a) {
    function someFunction2() {
        //prints 4
        console.log(a);
    }
    someFunction2();
}
someFunction(4);

Default Microsoft Access 2010 encryption and connecting to Excel

I recently found myself supporting an Excel spreadsheet created by business users using Office 2010 while waiting for my next project. I have been tasked with updating a new Access database that was created by one of the business users with data from the sheet as it is processed. Should be a simple task, right?

The Access database is broken apart into a frontend consisting of the forms and queries and the backend that contains the tables. One of the requirements of the Access database is that the tables are password protected. When a password is applied to an Access database, the file is encrypted so the file simply can’t be opened in another application and read. A password is required when using either Access directly or using another product and importing data.

Beginning with Office 2007, Microsoft began using their encryption library “Microsoft Enhanced Cryptographic Provider v1.0” (or CryptAPI) as described in this post by Garry Robinson and this post by Wayne Phillips. By default the file is encrypted with RC4 using a 40 bit key and the password is hashed using SHA-1. The big takeaway from this new encryption method in Access is that Microsoft stopped embedding the password in the file as it had done in Office 2003 and earlier, so it is no longer trivial to decrypt the file without a password. A brute force approach should be necessary. Unfortunately, RC4 is susceptible to attack and decryption. A short explanation is available at Wikipedia.

In Office 2010 another new Cryptography library was introduced, the Cryptography API: Next Generation. It is designed to be a long term replacement to the CryptoAPI. By default, Access 2010 is encrypted with this library and uses AES-128 for the file encryption and SHA-1 for the password hashing. This provides a much stronger encryption level. While it is susceptible to cracking at speeds greater than a brute force attack, it is still quite slow to break the encryption. This Wikipedia article explains in more detail.

So the big question is do we really need an encryption lesson in order to connect Excel to Access and insert some data? Shouldn’t all that be handled behind the scenes? The answer to the last question is yes it should, but no it’s not. While the AES-128 works seamlessly while you are in Access or Excel independently, while trying to connect the two platforms it is not working as it should. In my case, using either ADO or DAO to try to make a database connection in VBA to connect to the Access database, a “Not a valid password” error is thrown. Apparently the Next Generation library is used in Access and in Excel to encrypt and decrypt their own files, but was not implemented (or incorrectly implemented) in the ADO and DOA data libraries. This Technet thread shows the official answer by Microsoft is that it is not possible to connect via ODBC and my tests and others have shown the same issues with DAO and ADO.

Is there a solution to this? In Access under File -> Options -> Client Settings is a selection for Encryption Method. Changing this from Use default encryption (Higher Security) to Use legacy encryption (good for reverse compatibility and multi-user databases) switches from the Next Generation library and AES-128 back to CryptAPI and RC4-40. While this is less secure, it does allow you to connect to the Access database through VBA. I have read that users using Windows XP have not had this issue, but in reading over the Next Generation library details, it seems that the Next Generation was shipped as a part of Server 2008 and Windows 7 and greater, so my suspicion is that Windows XP is already defaulting back to CryptAPI as it is the latest cryptography library installed by default.

The majority of my testing has been on Windows 7 32-bit with Office 2010. I have also tested with Windows 8.1 64-bit and Office 2013 with the same results.