goto fail;

Apple’s SSL/TLS goto fail bug

On Friday Apple released a security update for iOS devices.
Their brief bulletin states that the “Secure Transport failed to validate the authenticity of the connection.”
This vulnerability leaves users of affected devices open to a man-in-the-middle attack by which a hacker could potentially intercept, decrypt and alter communication being made over SSL connections using Apple’s SecureTransport library.

The affected system versions are detailed under CVE-2014-1266 on the U.S. Government National Security Vulnerability Database, which also contains links to some of the key sources of early information about the exact details. But, for your quick reference they are listed below, together with links to the security update notices:
iOS 6.x before version 6.1.6
iOS 7.x before version 7.0.6
Apple TV 6.x before version 6.0.2
Apple OS X 10.9.x before version 10.9.2

Assuming that you have updated your devices (if you haven’t, go do it now!), what is interesting is that the bug occurred in a library which is open source, and is available for anyone to view.
It didn’t take long after the announcement on Friday for someone to find the code line responsible for the issue.
You don’t need to by a cryptography expert (in fact you don’t need to be an expert at all) to spot the mistake. Anyone familiar with programming languages should be able to understand the reason why this code fails.

The full source code listing for sslKeyExchange.c in libsecurity_ssl is available to view here.
The error is in the function SSLVerifySignedServerKeyExchange at line 631.

Do Not Pass Go, Do Not Collect $200
Go Directly To Fail. Do Not Pass Go, Do Not Collect $200

See that duplicated goto fail; line?

It could have been a copy-paste error, or the result of a merge conflict incorrectly resolved.

If we reformat the code, adding the implied brackets around the conditional code blocks, and correct the indentation, then the error should be understandable even to non-programmers.

Part way through the various checks it hit the erroneous goto fail, and jumps straight to the final lines that clean up and return the error code, skipping crucial verification steps that ensure the authenticity of the certificate being used to secure the connection.
The value of the variable err, which was not initialized with a specific value at the start of the function but has been assigned the return value (zero) of the successful call to SSLHashSHA1.update() that precedes the extra goto, is then used as the return value from this function – with zero indicating success.

As details of the bug spread, the term “goto fail” was rapidly coined to describe the incident, and #gotofail began trending on Twitter.
Someone also rapidly put up a website where you can test whether your system and browser are affected by this issue.

Alex Yakoubian posted a diff comparing sslKeyExchange.c between OS X 10.8.5 (Security-55179.13) and 10.9 (Security-55471), which shows the erroneous goto line being added to the file along with various other changes.
Here is the relevant bit:
sslKeyExchange.c goto fail diff

Leaving aside the more technical cryptography discussions, and speculation or conspiracy theories, what remains is for us as developers to examine our own coding practices and learn what lessons we can from the mistakes of others.

For many developers coming from a background in modern languages such as Java and C#, the mere use of a goto may seem alarming. However, having worked with Objective-C on an iPhone app project previously, I had seen this style of coding even if I don’t myself use it.

The first thing that leapt out at me when looking at the full source of sslKeyExchange.c was a lack of consistency.
It bears all the hallmarks of a file which has been worked on by multiple developers who were not bound by a single set of comprehensive coding standards.
Immediately noticeable when the file is opened in an ordinary text editor (not a code editor or IDE), is that the indentation inconsistently mixes tabs and spaces.

The second thing I realised (relevant given the nature of this bug) is the inconsistency with regard to single-line conditional blocks. There are places where a conditional statement is enclosed in braces even when it is only a single line, and other places where it is not. Compare the following snippet from later in the same file, and see how much easier it is to see the intent of the code when casually skimming through.

The easier your code is to read, the easier it is for someone to review, understand, and spot mistakes.

After the obvious, there are a few other things of which we can remind ourselves:

  1. Unit Testing. Vital to ensuring the quality of your code, and preventing errors like this from slipping in unnoticed.
  2. Code Reviews. Don’t underestimate the value of a second pair of eyes to challenge your assumptions, question your choices, and hopefully spot the inevitable slip-up before it reaches production code.
  3. Fail by Default. Does your code go bool success = true; followed by if (resultOfAction == error) success = false; Or do you it the other way around? Success should not be assumed prematurely.
  4. Use a compiler that generates warnings for unreachable code.
  5. Never ignore compiler warnings. If possible (and especially on green-field projects) set your release build to treat warnings as errors.

Well, that’s all from me for today. Code well, stay safe, and try not to goto fail!
And if you enjoyed reading, please share using the buttons below. 😉

2 thoughts on “Apple’s SSL/TLS goto fail bug”

  1. Nice post. However, there is one small correction. You have mentioned: “The value of the variable err, which was not initialized with a specific value at the start of the function (and so has the value zero)..”
    Actually, the value of err is not zero because it was not initialized. It is because the statement “(err = SSLHashSHA1.update(&hashCtx, &signedParams))” succeeded and returned a zero value which was then assigned to err. So while the uninitialized variable is not pretty, that is not why the value is zero.

Leave a Reply to Duncan Worthy Cancel reply