User: Password:
|
|
Subscribe / Log in / New account

"goto fail;" considered harmful

Benefits for LWN subscribers

The primary benefit from subscribing to LWN is helping to keep us publishing, but, beyond that, subscribers get immediate access to all site content and access to a number of extra site features. Please sign up today!

By Jake Edge
February 26, 2014

A serious flaw in the way Apple's iOS and OS X verify the keys in an HTTPS connection has been a major black eye for the company. The problem is in some of the open-source code that the company releases, so we can actually see the problem—it is eye-opening to be sure. The bug should have been fairly obvious from code inspection/review or could have been found with some intensive testing, so the fact that it went undetected—at least publicly—for so long is rather amazing.

The problem exists in Apple's Secure Transport API that provides access to SSL/TLS services for both OS X and iOS. It was first introduced in iOS 6, which was released in September 2012, and in OS X 10.9, which was released in October 2013. Updates to iOS 6 and 7, as well as to OS X 10.9, have been released, though the OS X problem went unfixed for several days after the problem was disclosed—which was deemed irresponsible by several observers.

Looking at the code in question should make it quickly apparent to those with even limited knowledge of C that something is amiss. In a function called SSLVerifySignedServerKeyExchange() is the following code:

    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;
The second "goto fail;" after the SSLHashSHA1.update() call for &signedParams is a bug. While it is indented to seem like it depends on the preceding if statement, that is not the case. There are no curly braces to turn it into a multi-statement if, so the second goto is unconditionally executed, which skips the rest of the signature verification.

But "fail" isn't quite accurate here. If it had been, the problem would presumably have been noticed quickly as many HTTPS servers would not have passed muster. Instead, the code at the fail label just cleans up a few things and returns err, which is likely to be zero, since the update() probably did not fail. That means that instead of verifying the signed key that the server sent over, the function will just succeed—for any key offered.

The key in question here is the ephemeral session key that is exchanged using the Diffie-Hellman and Elliptic-curve Diffie-Hellman ephemeral (DHE and ECDHE) key exchange protocols. DHE and ECDHE are used to provide forward secrecy. That key should be signed by the private key corresponding to the public key in the server's certificate. The signature is the proof that the server is actually in possession of that private key—without it, the link between certificate and identity (loosely defined) is broken. The bug allows any signature (thus any key) to validate. That means that a malicious server could use any certificate to spoof that site with impunity—it doesn't need to sign the ephemeral key with the private key it does not possess.

Google's Adam Langley has a nice analysis of the bug. As he noted, servers get to choose what cipher suites they support, so an attacker can force clients to use DHE or ECDHE to trigger the bug (if the client refuses to use one of those, it can't connect at all). The most recent revision of Transport Layer Security (TLS), 1.2, is not affected because the API uses a different function to verify those keys. But earlier versions of TLS and all versions of its predecessor, Secure Sockets Layer (SSL), are affected. Clients could work around the bug by requiring TLS 1.2 (or, less preferably, by disabling the DHE/ECDHE cipher suites)—that would mean they couldn't connect to some servers, perhaps, but they wouldn't run afoul of this problem either.

Evidently, code inspection/review did not turn up this bug (there is some speculation by John Gruber that it is the result of a botched merge). What is perhaps more surprising is that no testing with invalid signatures on the ephemeral keys was done. Langley, who works on the Chrome/Chromium browser, noted that the condition is kind of difficult to test for, because that exchange happens well into TLS/SSL handshake. On the other hand, Gruber also speculated that the NSA may well have known about the flaw from its testing, given that it added Apple to the list of companies participating in the PRISM surveillance program shortly after iOS 6 was released.

It is tempting to recite "Linus's Law" ("given enough eyeballs, all bugs are shallow") and believe that this kind of thing could never happen in free software. Tempting, but wrong. The truth of the matter is that plenty of free software only gets cursory (or no) code review, so something like this could slip through. In this case, Apple's code was available and no one ever publicly complained about it.

As Langley noted, compilers don't generally complain about unreachable code, which is unfortunate, for sure, but warnings tend to have a high false-positive rate, so they are ignored—or suppressed. Code that implements security protocols clearly needs a higher level of scrutiny, though, so one would hope warnings are actually being used by Apple (and OpenSSL, OpenSSH, ...). An incident like this is clear evidence that delivering bug-free code is a never-ending battle.


(Log in to post comments)

"goto fail;" considered harmful

Posted Feb 27, 2014 5:45 UTC (Thu) by flewellyn (subscriber, #5047) [Link]

This is why I think using if statements in C-like languages without explicit braces around the conditionally-executed code is ALWAYS a bad idea.

Well, not the only reason. But it's why I never leave them off.

"goto fail;" considered harmful

Posted Feb 27, 2014 8:15 UTC (Thu) by reubenhwk (guest, #75803) [Link]

I generally find myself adding more lines in that one line if block anyway, so I always pro-actively add the curly braces to save myself time (and typing) later.

"goto fail;" considered harmful

Posted Feb 27, 2014 10:37 UTC (Thu) by epa (subscriber, #39769) [Link]

I think if you keep your code correctly indented, for example by running a C pretty-printer and checking against that before allowing code to be checked in, then you get much the same visual check as requiring explicit braces. The second 'goto fail' would be at the same indentation level as the 'if'. OK, braces might be a bit clearer still, but you can still write confusingly-indented code even with braces; I suggest that an indentation check is always a good idea.

I would also have expected the compiler to emit a dead code warning after the unconditional 'goto fail'. Any reason why it did not, or why the developers ignored the warning?

"goto fail;" considered harmful

Posted Feb 27, 2014 12:00 UTC (Thu) by cladisch (✭ supporter ✭, #50193) [Link]

> I would also have expected the compiler to emit a dead code warning after the unconditional 'goto fail'.

Apple uses clang, which does not enable this warning by default (not even with -W -Wall -Wextra).

And gcc would not have helped because this warning was removed altogether.

"goto fail;" considered harmful

Posted Feb 27, 2014 13:28 UTC (Thu) by NAR (subscriber, #1313) [Link]

Great. It's too bad, because (in other language) the "unreachable code" warning always signaled fishy code. In most cases it was botched error handling in hard to test (so not exercised) code like this one. I wonder if static analyzers can help with code like this.

"goto fail;" considered harmful

Posted Feb 28, 2014 10:41 UTC (Fri) by jezuch (subscriber, #52988) [Link]

> It's too bad, because (in other language) the "unreachable code" warning always signaled fishy code.

In at least one "other [C-based] language" some instances of unreachable code are a compilation error, not a warning. In code like:

if (condition) return a;
else return b;
return c;

the last line would not compile. But it won't complain about "return b;" if the condition can be determined at compile time to always be true. And I guess it gets much, much less deterministic if we allow goto (which this language does not, even though it reserves it as a keyword).

"goto fail;" considered harmful

Posted Feb 27, 2014 15:56 UTC (Thu) by epa (subscriber, #39769) [Link]

That removal seems thoroughly silly. When was it ever promised that warnings would be stable from one compiler version to the next?

"goto fail;" considered harmful

Posted Feb 27, 2014 17:59 UTC (Thu) by mathstuf (subscriber, #69389) [Link]

I'd rather a warning that the flag does nothing to avoid having a false sense of security. (If you really care, you're looking at warnings anyways, so this would most likely be picked up on.)

"goto fail;" considered harmful

Posted Feb 28, 2014 8:29 UTC (Fri) by IkeTo (subscriber, #2122) [Link]

Coverage test will detect it, though.

"goto fail;" considered harmful

Posted Feb 28, 2014 16:01 UTC (Fri) by mathstuf (subscriber, #69389) [Link]

It would, but it would only really stand out if the file previously had high coverage already or the project overall is over 85% or so. Both of those usually take lots of discipline which seems to be lacking here since their review process seems lax.

"goto fail;" considered harmful

Posted Feb 27, 2014 15:09 UTC (Thu) by raven667 (subscriber, #5198) [Link]

> you can still write confusingly-indented code even with braces; I suggest that an indentation check is always a good idea

That's essentially the rational for Python's use of indentation as the canonical indicator of structure and scope.

"goto fail;" considered harmful

Posted Feb 27, 2014 15:53 UTC (Thu) by epa (subscriber, #39769) [Link]

Yes - probably the best feature of Python. Though I do also like the Haskell way of allowing either whitespace or explicit braces.

"goto fail;" considered harmful

Posted Feb 27, 2014 18:21 UTC (Thu) by iabervon (subscriber, #722) [Link]

I think that's actually one of Python's worst flaws: it's far too easy for a line to get misindented and do the wrong thing. For example, if you change a block of code to add an if statement at the beginning, it's really easy to neglect to increase the indentation of the last line. This is especially true when merging.

Put it this way: in the C example, the code with a bug is obviously wrong and the style shows that the programmer didn't mean what the compiler will do. In Python examples, the code with a bug is not obviously wrong, and the style doesn't show that the programmer didn't mean what the compiler will do.

The right way to do it is probably to include in your build a check that the code structure that your indentation implies matches the code structure that the punctuation indicates. Give an error if the programmer (or merge resolver) has failed to make the two ways to indicate nesting agree, because that suggests some sort of editing error. The simple redundancy is worthwhile as an error-detection mechanism.

"goto fail;" considered harmful

Posted Feb 27, 2014 21:23 UTC (Thu) by epa (subscriber, #39769) [Link]

This is especially true when merging.
That seems to argue for better merge algorithms that work on the syntax tree of the code rather than on just lines of text. The bug in the parent article may have been caused by bad merging of C code.

"goto fail;" considered harmful

Posted Feb 28, 2014 5:16 UTC (Fri) by mathstuf (subscriber, #69389) [Link]

They exist[1], but then you get into the language barrer. That one only supports Java and C# with plans for C/C++ and JS coming (probably with a higher price point as well). Python shouldn't be too bad since you can use the ast module on the two files then do…the hard part.

[1]http://semanticmerge.com/

"goto fail;" considered harmful

Posted Feb 27, 2014 21:59 UTC (Thu) by cesarb (subscriber, #6266) [Link]

The libreoffice project has a clang plugin which detects incorrectly indented code. Something like that would be useful here.

"goto fail;" considered harmful

Posted Mar 2, 2014 13:13 UTC (Sun) by Lovechild (guest, #3592) [Link]

Similarly Chromium also has a set of useful Clang extensions to detect style problems and other common problems. There is a great presentation (though now perhaps a bit dated) of how Google uses LLVM in Chromium.

http://m.youtube.com/watch?v=IvL3f8xY7Uw

I think a lot could be gained by adopting similar technics for many, especially large, projects.

"goto fail;" considered harmful

Posted Feb 27, 2014 22:05 UTC (Thu) by flewellyn (subscriber, #5047) [Link]

Sure, the indentation SHOULD have made the second "goto fail" obvious.

But with the explicit braces, it would have been more than just obvious: it would have been a no-op, and thus harmless (though still incorrect and worth removing).

"goto fail;" considered harmful

Posted Feb 27, 2014 9:27 UTC (Thu) by koch (subscriber, #55163) [Link]

Not completely unrelated, it seems that the Underhanded C Contest is back this year.

"goto fail;" considered harmful

Posted Feb 27, 2014 18:41 UTC (Thu) by jackb (guest, #41909) [Link]

They should rename it the "PRISM Compliance Plausible Deniability Contest"

"goto fail;" considered harmful

Posted Feb 28, 2014 8:30 UTC (Fri) by jem (subscriber, #24231) [Link]

Speaking of innocent subtle errors: "this year's" Underhanded C Contest was announced on January 4, 2013. :)

"goto fail;" considered harmful

Posted Feb 27, 2014 9:40 UTC (Thu) by mfuzzey (subscriber, #57966) [Link]

I think this is more failure of the "no braces for single statement blocks" rule than "goto fail".

That is the one part of Documentation/CodingStyle I really don't like.
I always use braces *except* when writing kernel code where omitting them is mandated - it always feels dangerous to me though.

"goto fail;" considered harmful

Posted Feb 27, 2014 15:58 UTC (Thu) by vonbrand (guest, #4458) [Link]

If it really was a merge botch, this might have ended up as two braces with `goto fail;` nestled inside...

"goto fail;" considered harmful

Posted Feb 28, 2014 14:35 UTC (Fri) by paulj (subscriber, #341) [Link]

Is there any reason to think this bug would have been prevented by braces? The double goto stands out to me even without the braces. At least, it seems no more obvious if there are braces:


if (...) {
    goto ...
}
if (...) {
    goto ...
}
    goto ....
if (....) {
    goto ....
}
etc.

If the original bug wasn't caught in whatever review (if any) that was done, I don't see why this variant would have been any more likely to be caught?

I think the braces are irrelevant and there are much better ways to guard against this kind of thing. Both in terms of the programme checking and testing.

"goto fail;" considered harmful

Posted Feb 28, 2014 15:59 UTC (Fri) by mathstuf (subscriber, #69389) [Link]

IMO, that stands out much more and would likely have been avoided during the merge resolution. The other chance was that the second goto was on the inside of the braces, making the bug have no actual impact.

"goto fail;" considered harmful

Posted Feb 28, 2014 16:15 UTC (Fri) by paulj (subscriber, #341) [Link]

As far as I know, it's not generally known exactly how the extra goto got in, is it? I've heard someone else make the merge argument, but I still suspect it will depend on the details of the merge.

"goto fail;" considered harmful

Posted Feb 28, 2014 19:01 UTC (Fri) by mathstuf (subscriber, #69389) [Link]

I agree, but the most plausible I've seen is a botched merge resolution. Whether the resolution was botched on purpose or not is still a question though. Apple is notoriously tight-lipped about security issues, so I doubt we'll ever know for sure.

"goto fail;" considered harmful

Posted Feb 27, 2014 13:11 UTC (Thu) by canatella (subscriber, #6745) [Link]

I often use gotos for error handling, but I always setup my code so that it returns error by default. I also try to always log something in case of failure.
int check_this(void)
{
    int rc = -1;
     
    if (do_that() < 0) {
        log("do_that failed");
        goto error;
    }

    rc = 0;

  error:
    do_some_cleanup();

    return rc;
}

"goto fail;" considered harmful

Posted Feb 28, 2014 0:32 UTC (Fri) by JdGordy (subscriber, #70103) [Link]

having the intention to do it always and actually doing it always are very different. The only way you can guarantee you get it right is with a thorough code review (or better, static analyses).

If we can assume the goto fail; bug was an accident then it shows Apple's internal process is obvious broken because something as obvious as this (made 100x more obvious with a coloured diff) should never have passed the developers review (or peer review or whatever review they should be doing).

"goto fail;" considered harmful

Posted Feb 28, 2014 2:17 UTC (Fri) by rusty (subscriber, #26) [Link]

No! Please do *not* initialize the return code:
{
    int rc;
     
    if (do_that() < 0) {
        rc = -1;
        goto error;
    }

    rc = 0;
error:
    do_some_cleanup();

    return rc;
}
This way gcc will *warn* you if some path doesn't set it explicitly. I use this because people add new exit paths and classically forget to update the error code return.

"goto fail;" considered harmful

Posted Feb 28, 2014 15:26 UTC (Fri) by dgm (subscriber, #49227) [Link]

This is a very good tip I'm going to start using NOW. Thank you!

"goto fail;" considered harmful

Posted Mar 3, 2014 13:06 UTC (Mon) by jwakely (subscriber, #60262) [Link]

GCC *might* warn you, its -Wuninitialized warnings are notoriously unreliable.

"goto fail;" considered harmful

Posted Mar 5, 2014 8:59 UTC (Wed) by canatella (subscriber, #6745) [Link]

Then you could get the same problem as Apple. I prefer having my function do no harm by default. Unit testing can then be use to check it is behaving correctly.

"goto fail;" considered harmful

Posted Feb 27, 2014 13:59 UTC (Thu) by error27 (subscriber, #8346) [Link]

I've posted this several places so it's probably likely that I am going to be proved wrong in an embarrassing way but this code wouldn't happen in the kernel under x86 allmodconfig.

Coccinelle would complain about the missing curly braces and Smatch would complain about the unreachable code. We run these regularly with the kbuild zero day project and maintainers run them and I run them.

There will always be stupid bugs but this one was avoidable.

"goto fail;" considered harmful

Posted Feb 27, 2014 22:55 UTC (Thu) by PaXTeam (guest, #24616) [Link]

how many warnings do these tools generate? (any public URL to them perhaps?) how many get looked at? how many get fixed? how much is the noise?

so yeah, there are good reasons why unreachable code warnings (and many others) aren't on by default anywhere...

"goto fail;" considered harmful

Posted Feb 28, 2014 12:59 UTC (Fri) by error27 (subscriber, #8346) [Link]

It's actually a bit more complicated than I said... The zero day build checker tests for these because it is using an older version of Smatch. The current version of Smatch does have this test turned off by default but that's a temporary thing and I am going to turn it on again. But I test with it on and I would have sent an email about this bug.

I only look at new bugs so it's not overwhelming. If I want to look at all the unreachable code in the kernel then I use a script to strip out most of the false positives:
http://repo.or.cz/w/smatch.git/blob/HEAD:/smatch_scripts/...

Since I only use Smatch on the kernel then I add a lot of hacks to silence false positives. I will do this before I turn the check on again.

Here is one email I sent recently:
http://www.spinics.net/lists/linux-iio/msg12243.html

The situation with missing curly braces is also a little bit more complicated. Coccinelle has been missing some. I added this to Smatch a couple weeks ago. I didn't find any real bugs, just stuff like this:
http://www.spinics.net/lists/linux-kernel-janitors/msg182...

Also there are other checkers which people run which catch both these kinds of bugs. The kernel is lucky like that to have a lot of people testing...

Was it deliberate?

Posted Feb 27, 2014 15:11 UTC (Thu) by mkerrisk (subscriber, #1978) [Link]

Interesting short discussion from Bruce Schneier about whether this might have been deliberate: "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."

https://www.schneier.com/blog/archives/2014/02/was_the_io...

"goto fail;" considered harmful

Posted Feb 27, 2014 18:30 UTC (Thu) by iabervon (subscriber, #722) [Link]

I wonder if it would be worthwhile to have a macro like:

#define ERR_RETURN(err) do { \
  if (!err) undefined_external_function(); \
  return err; \
} while (0)

So that you get a link failure if the compiler can't prove that err is not 0 when you return it as an error.

"goto fail;" considered harmful

Posted Feb 28, 2014 3:21 UTC (Fri) by mathstuf (subscriber, #69389) [Link]

You're breaking -O0 here.

security standards should provide these sort of tests

Posted Feb 27, 2014 19:41 UTC (Thu) by tialaramex (subscriber, #21167) [Link]

Surely the most directly useful thing to come out of this is:

SSL (and TLS) should never have been standardised without supplying a set of test tools that could exercise the intended security function. Most likely that means a client and a server which can trivially be configured to get things wrong in each individual way that the standard calls out. So here for example, you'd get to the piece of code in the test server that should send back a signed ephemeral session key, and it can optionally just send all zeroes, or some plausible but wrong value instead, or a real one.

If making such tools seemed impractical due to the sheer size of the standard that's immediately a red flag - the standard is too complicated to correctly implement, don't ship it.

If those tools had existed on day zero, a lot of people (and certainly anyone at all security-minded) would have been inclined to write their test cases using the provided tools, and we'd get a lot more good quality software as a result. Also this sort of goof could be trivially spotted by third parties, it wouldn't need somebody to develop tools for the purpose because the test tools would already exist ready to go.

security standards should provide these sort of tests

Posted Feb 27, 2014 20:04 UTC (Thu) by raven667 (subscriber, #5198) [Link]

Maybe that's how this was discovered, as far as I know there hasn't been any release of information of who discovered and reported this issue, so it is possible that a third parties test suite failed and they notified the vendor but didn't themselves want attribution in the CVE.

security standards should provide these sort of tests

Posted Mar 1, 2014 18:52 UTC (Sat) by peschmae (guest, #32292) [Link]

I have to admit I find it quite amazing that so many people are going on about braces here and everywhere else. And the evilness of goto. Or code review.

This type of stuff can easily creep in during a merge. Or be added by someone intentionally. Or be caused by someone changing the meaning of some other function somewhere else...

The real issue, it seems to me, is the lack of testing - we have here a library that is supposed to check certificates for basically any software running on our operating system. Great. Let's set up some tests that try to cheat and see if the library catches this as intended...

security standards should provide these sort of tests

Posted Mar 1, 2014 19:31 UTC (Sat) by raven667 (subscriber, #5198) [Link]

It would have been great if Apple, or someone, had written an SSL testing framework that would have exercised all of the failure paths to make sure they all worked properly but that is probably more code, developer work and cost than implementing the library in the first place, and no one seem to have ponied up to do so. In a perfect world everyone would check all error cases and write all the test code, especially for security-critical components like an SSL library, but that is not the world we live in.

"goto fail;" considered harmful

Posted Feb 28, 2014 1:51 UTC (Fri) by ras (subscriber, #33059) [Link]

Personally, I don't lay the blame at Apple's feet or GCC. I lay it at the compiler writers.

In this case, the problem sticks out like dogs balls. But is doesn't always. This sort of thing can be near impossible to spot:

  if (long condition going past right margin);
    goto fail;

There are two things that would have found this. Static dead code analysis. Ie, -Wunreachable-code. I first I thought it was Apple's fault for turning if off, or worse ignoring warnings. But no: everyone turns it off and now GCC has removed it entirely. Why? Because they used the optimiser to find the dead code. It does a remarkably good job of finding dead code, but a lousy job of reporting it. One reason is if you don't optimise, you don't get the error. And when don't you optimise - when you are developing, of course. So debugging is easier.

The other reason is dead code arises naturally when inlining functions. In some places the inlined code is used, in others it isn't. Result: spurious complier warnings, depending on the level of optimisation used.

In this day and age, were the comparison between the cost of a CPU cycle and programmers time this is unforgivable. There is a reason every sane programmer compiles with -Wall -Wextra - we need the help.

The other thing that would have found it was insistence on 100% unit test coverage. In security code in particular, I would not have thought that was a big ask.

"goto fail;" considered harmful

Posted Feb 28, 2014 10:49 UTC (Fri) by jezuch (subscriber, #52988) [Link]

> In this case, the problem sticks out like dogs balls. But is doesn't always. This sort of thing can be near impossible to spot:
> if (long condition going past right margin);
> goto fail;

Easy:

"Warning: empty statement."

"goto fail;" considered harmful

Posted Feb 28, 2014 10:59 UTC (Fri) by ras (subscriber, #33059) [Link]


  $ echo "int main() { if (0); goto x; x: return 0; }" > x.c
  $ gcc -Wall x.c
  $

"goto fail;" considered harmful

Posted Feb 28, 2014 11:54 UTC (Fri) by hummassa (subscriber, #307) [Link]

-Wextra

or, even more specifically,

-Wempty-body

"goto fail;" considered harmful

Posted Feb 28, 2014 13:14 UTC (Fri) by jezuch (subscriber, #52988) [Link]

I wasn't talking about C ;) That's what Eclipse spits out when it finds this in Java code, and what I find a disgrace that this is not considered a serious warning either by GCC or the default compiler in JDK...

"goto fail;" considered harmful

Posted Mar 3, 2014 7:53 UTC (Mon) by dlang (subscriber, #313) [Link]

> It is tempting to recite "Linus's Law" ("given enough eyeballs, all bugs are shallow") and believe that this kind of thing could never happen in free software. Tempting, but wrong.

was this discovered by people inside Apple who would have access to the source code no matter what? or was this discovered by someone outside Apple who could only see it because the code was available.

If the latter, I'd argue that Linus's law may still be applicable, how much longer could this have continued to exist without the source being available.

I agree that source that's available, but only used for review is far less valuable than source that's available and use (compiled, modified, updated, etc) because code that's used gets FAR more eyeballs looking at it than code that's read-only (and especially code from a company who likes to use lawyers to go after others for "copyright violations")


Copyright © 2014, Eklektix, Inc.
This article may be redistributed under the terms of the Creative Commons CC BY-SA 4.0 license
Comments and public postings are copyrighted by their creators.
Linux is a registered trademark of Linus Torvalds