On 21/2/2014, Apple pushed a security update for iOS with a rather scary description:
Description: Secure Transport failed to validate the authenticity of the connection.
My Twitter timeline was already up in arms before I had a chance to view the description, at which point I mentioned it to a colleague. I added it was issued for iOS 6 and 7, and he wondered, prophetically: “Did they fuck SSL up?”.
Earlier in the day, we’d spotted the bug in Safari but we were fooled, handwaving its origin to network misconfiguration in an attempt to get out of the office by 17:00. :(
As you probably have already read in other places by now, the bug is this:
Notice the two
goto fail; statements in a row. The second will always jump to the end of
the function, returning the value of
err, which signals a successful operation.
The amount of things that went wrong here seems near incredible to me:
- Earlier in the same
you can see the author(s) defensively using brackets around a single statement in an
ifblock. I know it’s hard to get a big group of people to use a convention, but come on.
- If Apple is using clang to compile their own code, there’s an option to warn on
unreachable code (aptly named
-Wunreachable-code). Maybe somebody ignored it. Maybe somebody thought it’s included in
-Wall, but it isn’t - and that’s undocumented.
Maybe nobody never even saw the code. I’ll be the first to admit hindsight is 20/20, yet this is still easy to catch the eye in a code review, for two reasons:
- It’s not subtle. It stands out both in style and intent.
- The error checking is not idiomatic for C. Typically, one connects the statements
else ifblocks to further protect against this sort of thing - in which case the compiler would’ve failed to parse the subsequent
else ifstatement even without
- Additionally, it’s not subtle in behaviour, either. Unless I’m missing something, which I might because I’m already on my way to Saturday night intoxication, a test which supplies a certificate with an invalid ephemeral key signature (the code is used in DHE & ECDHE cipher suites) to a client and checks for acceptance can spot it. Does that sound right?
- The commit that introduced it seems like it copy/pasted the
goto fail;line there, possibly by accident. If someone skimmed through the patch it’s quite unlikely they wouldn’t wonder about its presence.
More than anything, this makes me rethink about the tools people trust to do their jobs every day. I know for a fact acceptance testing in certain organizations uses typical browsers like Chrome, Safari, Firefox and the like to test features and security requirements such as: “The URL to this and that should be an HTTPS one exclusively, and yadda yadda”. Is it perhaps “too trusting” to assume those tools are doing their job right? Acceptance testing is one thing; perhaps QA or security testing would test this with something like TLSLite.
At any rate, I guess we’ll all have a full Monday with this one. Take care.« Past Future »