Collaborator8.2-Blog-CTA-Demo

GOTO (Still) Considered Harmful

apple-goto-fail

Seriously Apple? Seriously? GOTOs? In your Secure-Socket Layer implementation? What were they thinking?

Apple, Apple! When Ed Post wrote in 1983 that Real Programmers aren’t afraid to use GOTOs he was kidding!

No one should ever use go-to statements in any program. As programming genius Edsger W. Dijkstra put it, all the way back in 1968, “goto statement should be abolished from all ‘higher level’ programming languages.” And by higher-level, he meant anything from Assembly language on up!

Why did he say this? Because the goto statement as it stands is just too primitive: “It is too much an invitation to make a mess of one’s program,” wrote Dijkstra. Which, of course, is exactly what Apple did.

In case you don’t follow security or the vagaries of Apple operating system programming, here’s what happened.

On February 21, Apple released a new security update for late model iPhones, iPod touch devices, and iPads. This was to fix a situation where “An attacker with a privileged network position may capture or modify data in sessions protected by SSL/TLS Secure Sockets Layer/Transport Layer Security.”

Anytime someone can break SSL/TLS, the underpinnings of almost all Web-based security, it’s bad news. It turns out that the breech enabled anyone with a certificate signed by a “trusted CA” to do a MITM attack (MITM). With an SSL/TLS MITM, the attacker controls the horizontal and the vertical, a la the Outer Limits. Heck, they control everything including your credit-card numbers.

goto-fail-shirt

Photo source: TeeSpring.com

Worse news was to come: The same security hole was in Mac OS X. Aldo Cortesi, CEO of the security  company Nullcube, revealed that he had modified the MITM proxy program mitmproxy so that he was able to confirmfull transparent interception of HTTPS traffic on both IOS (prior to 7.0.6) and OSX Mavericks.”

What was the root problem of this fatal TLS/SSL flaw? Adam Langley, Google Senior Staff Software Engineer and HTTPS specialist, took what little Apple had to say about the problem and it didn’t take him long to find it. See how long it takes:

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

Yeah, that doubled “goto fail” kind of jumps out at you doesn’t it?

As Langley explains, “Note the two goto fail lines in a row. The first one is correctly bound to the if statement but the second, despite the indentation, isn’t conditional at all. The code will always jump to the end from that second gotoerr will contain a successful value because the SHA1 update operation was successful and so the signature verification will never fail.”

Or, in other words, if intercepted these “secure” connections can always be broken. That single line of code, since it isn’t a conditional, causes the entire procedure to terminate. It wrecks the security routine’s entire purpose.

Langley kindly says, “This sort of subtle bug deep in the code is a nightmare. I believe that it’s just a mistake and I feel very bad for whoever might have slipped in an editor and created it.”

I’m not so forgiving. This code is at the heart of Mac OS X and iOS Web security. And, what may I ask, are those goto statements doing there in the first place!?

Where was the code review? Apple doesn’t appear to practice even basic code review not just for one of its operating systems but both of them. How can programmers for anything but the most trivial work not do this?!

Yeah, I get it. Code review can be time-consuming. So what? This is the most mission-critical code of mission-critical code for an Internet-connected device.

Besides, code review doesn’t have to eat up man-hours. Tool-based code reviews are much faster than having someone looking over your shoulder. It’s also a heck of a lot more through about spotting the bad lines than even the most expert programmer.

Even if that unconscionable coding error wasn’t caught in code review, how in the world did it ever get past quality assurance? How hard could it be to notice that all of Apple’s family with the newest operating system never met a SSL/TLS site it wouldn’t shake hands with!?

Indeed this is such a fundamentally stupid mistake that the security pro’s pro, Bruce Schneier, thinks it’s possible that the security hole was deliberately introduced. Schneier says, “Was this done on purpose? I have no idea. But if I wanted to do something like this on purpose, this is exactly how I would do it.” He adds, “It would have been trivially easy for one person to add the vulnerability.”

If it was anyone except Schneier I’d think they’re ready to join the tin-foil hat club, but I have to take him seriously.

That said, good code review and even basic quality assurance should have caught this problem. Some people have suggested that Apple’s goto failure shows a programming staff that needs a massive culture change to fix. I must agree.

The lesson here is clear. Code review is vital. Bad coding practices—and they don’t get much worse than using gotos—have to be found and eradicated. Even if the code itself works and isn’t buggy, bad code practicing is just asking for trouble down the road.

Apple needs to get its programming, code review, and quality assurance acts together. If you have even the faintest fear that your company’s developer team may have similar problem you must get on top of these issues now. Apple’s reputation is strong enough that it can handle even such complete cluster-foul ups as this one and stay in business. Your company may not be so lucky.

See also:

 

subscribe-1

  • Stuart

    Visual Studio would produce a compiler warning – which I would set to fail the build. Problem solved in C#/.NET anyhow.

  • Jingqi Xie

    I wish there were a JavaScript IDE that integrates code review. I know there’s been JSLint and JSHint and I wish them to be integrated into IDEs such as Visual Studio.

    • Artemis311

      Try http://brackets.io/ it’s the easiest way to use built in JSLint among many other wonderful features. It’s especially nice if you have to work on the front end with some of the CSS editing features.

    • Artemis311

      Try out brackets.io it is the most straight forward way I found to run built in JSLint and JSHint among many other wonderful features. It also provides great support for front end Devs.

      • Jingqi Xie

        I tried it and found it hard to use.

  • Dean Schulze

    My understanding is that the problem with goto isn’t the goto itself, but the labeled statement that receives control from a goto. You can always tell how a goto is going to transfer flow, but the labeled statement is global: It can receive control from anywhere in the program and it can be very hard to tell how the program got to that state.

  • ohengel

    This error was not caused by the use of the goto command. The error is a line duplication, and there are various commands that can cause an error when needlessly duplicated.

    I generally agree that many uses of the goto command are undesirable, but not all are. A good counter-example is the state machine, sometimes also used and known as a syntax machine. It works very well and with unmatched performance when you use goto commands.

    Generally you cannot force a bad programmer to write good code by limiting the language. And you don’t have to force a good programmer—they will write good code anyway, and they will use a goto command only when it is truly advantageous. To believe that a general ban of the goto command will automatically produce better code is childish.

    I also agree that the program code shown here is an excellent example for a backdoor that looks like an error. Actually the backdoor is not camouflaged very well, but it is at least deniable. The responsible programmer can still pretend that it was a simple copying error. While few serious programmers will believe that, it can still not be disproved.

    • RSL256

      I completely agree that goto is dangerous, but occasionally useful in judicious hands. Another style option which might have helped here is the practice of always using explicit brackets to denote the scope of an if clause. Such a practice decreases that chances for this type of error, and would also make a deliberate “error” harder to slip in.

      You can’t legislate bad programmers into doing a good job. Better to teach them to be good.

  • Audio Tosell

    Add GnuTLS to the list of GOTO errors
    A few more of these and the exception will prove the rule

  • pirlouwi

    >> and so the signature verification will never fail.”
    I supposed the author meant : and so the signature verification will fail forever.”

  • Jim Mischel

    That bug should have been caught by the build process. Any decent C or C++ compiler has a warning for unreachable code, and a good build process would compile at the highest warning level and the “treat warnings as errors” option enabled. That standard procedure would have prevented this bug from escaping. That it wasn’t caught there says volumes about Apple’s development practices.

  • Lamaur Cheetum

    These new quick rams are working though. this is funny I like them when they don’t then you can try them.

  • Bharath Iyer

    It is more of a Developer Ego I would blame this on. At a certain level, Developers love boasting about coding using plain text editors and writing flawless code. To err is human and sometimes even the best of Developers make mistakes. One such case I would presume. And, instead of a goto, I would rather recommend a try catch block or a function call if need be.