In what possible reality was this not the inevitable outcome? The "enough eyeballs" aphorism makes for a nice soundbite but in practice was always a red herring.
Clearly sarcasm. With open source, it is not a matter of whether these mistakes (or whatever this one was) happen or not, it is a matter of likelihood of these mistakes being spotted. It's a statistical thing. It's unfortunate this has to be explained.
Outside of python, for obvious reasons, it should never be allowed. The slightly nuts conditionals in Erlang are justifiable for avoiding exactly this kind of problem.
Agreed. Mandatory curlies might have helped avoid disaster here, but the code in question is still utter rubbish. If blatant nonsense makes it past review (or if there is no review), no coding style rule in the world is going to help.
Couldn't you just set a 'failed' boolean and wrap the fail: statements in a conditional check on it? Not that I'm saying all uses of goto, particularly this one, are necessarily absolute evil.
I agree, I was thinking about what this situation would look like in other langs and when I turned to Go, I realized:
While Go has goto for tricky situations like this, because it has defer you don't have to use it often, assuming the free calls were needed (and the vars were not going to be GC'd):
I would probably flag the code above in a security review because it hides the key part at the end of a complex line. Unless you're coding on VT100 terminal it's worth the extra line to make the test logic incredibly obvious:
True, but I've definitely noticed that particular style of writing if tests using a one-line assignment and obscured test condition seems to be pretty common in the Go community and it's a bad habit for understanding code.
Is there objective evidence for this? As a Go programmer a semicolon in an if statement screams to me. I can see it possibly being in issue for new Go programmers- but I don't remember it being one for me.
I didn't do a survey but I remember that and frequently punting on error handling (`res, _ = something_which_could_error()`) showing up enough in the projects I saw on Github to stand out as a trend when I was writing a few first programs. I certainly hope that's just sampling error.
The `else if` is useless here, since you return anyway. It only adds noise.
I rarely use the one-line `if err := ...; err !=nil ` idiom because its quite a mouthful. However when I do, I try to make sure it's not too much to grasp at once. Here the extra `else` goes against that.
Alright, I know this is just a quick snippet on HN and all, I just thought I'd mention it anyways. Maybe next time you actually write that in code you'll think about my point. ;)
This is what goto statements are good for. The Linux kernel also uses goto statements a lot for similar reasons.
This is not spaghetti code or what Dijkstra talked against.
It's one of the two things one hears novices: that gotos are to always be avoided (because they heard that they are bad),
and that we should write stuff in assembly (cause they heard that it's fast).
I cringe whenever I hear people advocating that it's okay to avoid braces with single-statement conditionals/loops/etc.
This is a perfect example of just how bad that suggestion is, and just how disastrous it can be. It's totally worth typing a few extra characters here and there to basically avoid these kind of situations completely.
I hate omitting braces, but Javascript semicolon omission is a totally different argument. It's easy to show simple cases where omitting braces causes problems. It's actually quite difficult and artificial to find cases where (especially with "use strict") semicolons confer any benefit at all, and in some cases they make things worse (e.g. multi-line variable declarations). Also, "correct" use of semicolons in Javascript makes code less consistent (e.g. some function declarations end with a semicolon, others do not) whereas consistent use of braces is consistent.
I certainly see far more bugs caused by an improperly inserted semicolon than an improperly omitted semicolon, but then I'm looking at jshinted code most of the time.
I used to believe as you do that semicolon issues were contrived and unlikely in JavaScript. But over the years I've been bitten by it enough times to know better.
IIFEs are the most common source of semicolon problems:
for(i = 0; i < 10; i++){
x = arr[i]
(function(x) {
setTimeout(function(){console.log(x)})
})()
}
A less common sort of pattern that I still use pretty often as a DRY measure:
This is clean and readable, and without semicolons it's completely wrong.
Now sure, you can prepend semicolons to these specific cases, and that will work. Here's why I dislike that:
1. Those leading semicolons look weird. To the uninitiated, it looks like a typo, and that's a significant hazard if anyone else is going to touch your code.
2. While there are only a handful of special rules like this to remember, there's only one rule to remember if you don't rely on ASI.
Good examples actually. (Makes me feel better about my ingrained semicolon habit.)
Your examples will crash immediately and at the right spot though. The problems I see caused by excessive use of semicolons are often far weirder.
That said, inadvertent errors caused by semicolon insertion are still more common and baffling (especially by people addicted to jslint who use a variable declaration idiom particularly easy to screw up with errant semicolons).
The first example won't crash if the rvalue preceding the IIFE is a higher-order function (specifically, one that outputs a function).
A relatively rare scenario, but a brutal one to debug.
As for errors caused by extra semicolons, they can be weird, but I don't think I've ever actually hit one in practice. They'd also be a little easier to spot, since you tend to develop a reasonable instinct for where semicolons belong.
I'd say it's a lot easier to be suspicious of lines of code that start with '(' or '[' than find semicolons at the end of the wrong line. It's incredibly easy to put a semicolon where a comma is expected and vice versa. I do it all the time (usually causing an immediate error so it's not a huge deal).
...just overwrote c in a different scope. This kind of bug is common, idiomatic, baffling, and actually more likely among coders subscribing to javascript "best practices".
jslint won't stop it if there's a variable (c in this case) in the outer scope -- it's perfectly fine code as far as jslint is concerned. And jslint encourages that declaration style (actually encourages is too weak a word).
> I do write single statement conditionals without braces, but I write them on one line
That's fine until you write
if (something) do_something(); do_something();
or worse:
if (something); do_something();
I've actually seen something very similar to that one.
You haven't really changed the dimensions of the problem by putting it all one one line. Only the whitespace is different. Sure it looks wrong to you; but so does the incorrect code from apple.
I don't understand the need to omit braces; it doesn't make the compiled program any smaller. And denser source code is not always more readable, or we'd prefer "? :" to "if" every time.
This is true; there is no silver bullet. So we rely on defence in depth.
The first line of defence is consistent code layout; e.g. always use "{ }" after an if, and only one statement per line. This aims to make simple typos less likely to compile, make the code more readable and so make errors stand out.
Then there is using and paying attention to the linter / compiler warnings / jshint + 'use strict' / static analysis or whatever it’s called in your language of choice.
Then there are unit tests or integration tests.
Any of these might have caught the apple error, but there are no absolute guarantees.
IMHO it's a professionalism issue – gifted amateurs are great, and they can succeed... most of the time. But a professional engineer would put aside ego "I won’t make mistake like that" and laziness "I don’t need to type braces this time" and do the careful thing every time. Because each step raises the bar, and makes it harder to ship code with bugs like this. Because sometimes it matters.
Interesting that, if there was a source code formatting tool (like gofmt for go), it would've made the bug easier to spot because the 2nd goto would lose it's misleading indentation.
Funny thing is, I was finally looking at Rust the other day and thought, hey, that's nice that if statements are forced to have braces, I never liked one-liner if statements in C. And here we have a perfect example of what can happen with the brace-less if statements.
Requiring braces is the wrong solution to the problem. It's the indentation that misleads here; indentation is what people look at to see what's part of which if, so have the language look at the same thing, like Python does.
That's the thing - with an indentation-based syntax, the code would have been okay! The second goto would have been part of the if statement, so it would have had no effect beyond the aesthetic.
I came here to say this. Yes, testing is important. Don't ignore it. But let's not lose sight of the high value of smart, time-tested coding conventions.
They exist because of the long, bloody history of mistakes by very smart people.
If you have a programmer on your team who is likely to modify
if (condition)
{
doSomething();
}
into
if (condition)
{
doSomething();
}
{
doAnotherThing();
}
instead of
if (condition)
{
doSomething();
doAnotherThing();
}
then that person needs some serious mentoring right away. 'Cuz. . . just wow.
As far as the original example goes, if it's an error it's most likely a copy/paste error. Curly braces help there, too. With three times as many lines in the block, the odds of a paste error resulting in code that even compiles is greatly reduced, and a compiler error should call attention to the issue.
Even assuming the bug was malicious, the curly braces would increase the odds of it being caught by another developer. This is particularly the case now that offside rule languages are common. A large chunk of younger devs cut their teeth on languages where
if (condition)
doSomething();
doAnotherThing();
doesn't look odd in the slightest. But I think that the 2nd example above would still look immediately bizarre to nearly everyone.
As far as the original example goes, if it's an error it's most likely a copy/paste error.
Right, and this demonstrates the major problem with verbosity in languages and APIs and design patterns. When you have to repeat yourself many times, it's very easy to make a mistake in one of the near-copies, and you or a code reviewer can miss it because you'll tend to quickly skim over the boilerplate.
For cases like this, using exceptions rather than manual error value checking would make your code shorter, less redundant, and not susceptible to this particular bug. (Not possible in C, I know).
I understand that there are sequence points at those || divisions, and I appreciate that you've been careful with your layout. Even so, my spider sense is tingling horribly at having not just one assign-and-test in the condition for an if statement but a whole series of them that each reassign to the same variable.
If there were a language feature available to do this more elegantly, whether exceptions or something else such as a Haskell-style monad with fail semantics, I'd almost certainly agree with using it in preference to a series of manual checks, though.
>> Is there a case where the compiler won't short-circuit the || statement on the first failure?
Left-to-right, short-circuit evaluation of the logical operators is so deeply rooted in C idiom that a mainstream compiler that doesn't respect it would be practically unusable. But perhaps it wouldn't work in a hobbyist compiler, or a non-standard dialect of C.
Indeed, the code as presented there had well-defined behaviour according to the standard. Note to lunixbochs: This is why I mentioned sequence points before.
While valid according to the language definition, assignments within conditional expressions do have a reputation for being error-prone and not always the easiest code to read. Sometimes they're still neater than the available alternatives.
However, IMHO in this case, it feels a bit too easy to break the code during later maintenance edits. For example, someone might simplify the code by removing the explicit comparisons with 0 that are unnecessary here, and then later someone else might "fix" an "obvious bug" by replacing the "(err = x) || (err = y)" construction with the common construction "(err == x) || (err == y)" that they assumed was intended, perhaps prompted by a compiler warning. Obviously they shouldn't if they didn't properly understand the code, but when has that ever been a reliable guarantee? :-)
It's interesting watching all the speculation about "was it a backdoor, or just a bug?"
Lots of points in favor or against:
1) It's a huge compromise, and "open" to anyone to exploit, which would ultimately get caught and fixed faster. But it's also not targeting anything specific, so there's less of a signature of the attacker.
2) Incredibly simple, and thus a plausible mistake.
3) Hidden in plain sight
I'd generally come down on the side of "accident". The better question is if an systematic testing system for exploitable weaknesses (or a lucky chance finding) could find something like this (either a regression, or a badly implemented new feature) and exploit it -- basically assuming there are going to be errors in implementations. That's understood to be a standard technique in the IC, and a whole industry built around it... and then, the scale of compromise.
There are lots of mitigation techniques for vulnerabilities like this (essentially, devices which are too fast-changing and too difficult to fully trust, but which have to touch sensitive data), but it's not as if people can carry around a firewall in their pocket these days, sadly.
I'm certainly re-generating any keys which touched iOS or OSX devices (thankfully very few), and reinstalling OSes, in the interim.
My gut says the probability of a random typo executing without raising an error or exception is rather low. The probability of it doing so in a way that well aligns with the interests of nation states, large corporations, and/or criminal enterprise is even lower.
This might explain DROPOUTJEEP particularly considering how much more efficient it is than breaking messages after they are encrypted.
Sure, this aligns with interests, but the bug's existence is predicated on the entire code base being written with substandard style rules and no static analysis or tests, which suggests to me that incompetence got here first.
It is mind blowing that they may not run static analysis on something as big as OS code bases, for something as simple as a duplicated switch break. Easily could have been a copy paste fail but that is why on a project this big you need to have that. Or if they do use static analysis on builds and it failed or by-passed this area, there is another possible hole.
Whether it was incompetence or malice, whoever was responsible for the extra goto, as well as whoever was responsible for ANY of that code not following common practices including ALWAYS using brackets, should be fired.
And that fact that this bug and terrible coding style was in the publicly available source code for so long totally disproves ESR's "many eyes make all bugs shallow" myth. Thanks for the false sense of security, Eric. The chickens have come home to roost again.
If "many eyes make all bugs shallow" were true, somebody would have raised the flag "Hey everybody, these Bozos are actually omitting brackets in their SSL code! Somebody could carelessly insert a statement and accidentally (or maliciously) introduce a hard to see bug some day!"
Hardly anybody actually bothers to read code in the real world. So there aren't "many" eyes, and even if there were, many bugs aren't shallow even to expert eyes, and "all" bugs will never be shallow to most eyes.
That's why it's important to pay competent security professionals to actually take the time and effort to audit source code, which is difficult work that requires much time and effort that takes them away from other valuable, high paying, less tedious and mind numbing work.
We begin therefore where they are determined not to end, with the question whether any form of democratic self-government, anywhere, is consistent with the kind of massive, pervasive, surveillance into which the Unites States government has led not only us but the world.
This should not actually be a complicated inquiry.
> My gut says the probability of a random typo executing without raising an error or exception is rather low
In many code editors, numerous fat fingered shortcuts could produce such a compilable line duplication/deletion (deletion because maybe that's not a goto line duplicated but a test line deleted).
I'm baffled that neither clang nor gcc spits a warning for the unreachable code.
Not trying to shift blames or anything, but rather than do some finger pointing, I'd rather see this as a warning for all of us to ramp up our game and better our tools so that everyone benefits, reducing the risk of both honest and intentional or covert[1] malfunctions.
In general, NSA is not my most serious threat (in terms of actual harm; they are the most powerful by far though) -- I agree they overreach, and for some people are a serious threat, but for me my primary concerns are Chinese/other foreign intel (who are documented as going after industrial/economic material much more than NSA) and independent/criminal/etc. types.
The scary thing is the bar is so low; even I could turn this into a nice weaponized thing to go after the long tail of people who don't upgrade, for the next months. The "diode" etc. infrastructure NSA built isn't that different from botnet C&C or the kind of relays people have when hacking.
I wish more people understood this. Not to let NSA off the hook (what they are doing is awful), but the threat posed by the NSA is a higher-level down the road/slippery slope threat.
There are other more immediate and real dangers out there that are actively trying to steal whatever they can find.
What I would like more people to understand is, the NSA planting backdoors into everyone's accounts and software makes everyone less safe to both interior and exterior attacks. So no, it's not a case of NSA spying being a lesser threat to eastern spying, it's a case of the NSA punching holes in your walls letting everyone see what's inside.
That's because "for (;;) ;" is and idiom that's commonly used in a programming pattern called the "for(;;);ce field", to work around other C and C preprocessor quirks, by syntactically insulating and protecting statements in C preprocessor macros from outside interference.
As a matter of fact, by deploying one of those the beginning of the macro, one in the middle, and one at the end, you can set up what's called a "trifor(;;);ce field", an anti-anti-pattern that is documented on the c2 wiki thusly:
The trifor(;;);ce has the power to grant the wish of whomever touches it, and molds the Sacred Realm to reflect that person's heart. The trifor(;;);ce does not discriminate between "good" or "evil". However, if a person without an equal balance of power, wisdom, and courage makes a wish, the trifor(;;);ce will split into its three separate parts: the piece that best personifies the wish-maker will be the only piece to remain in hand, whilst the other two will take residence in whosoever most personifies them. Reassembly is then required for such a person's wish to be granted, but does not exclude another from making the attempt.
No, I'm just joking and making shit up. Don't anyone ever do that! As bitchy and pedantic as gcc and clang are about other things, I'm disappointed they don't complain about that.
It was likely someone subverting the process for reasons of urgency, for good intentions or malice. Simple static analysis or unit tests would have caught this.
I'm baffled that this wasn't caught long, long ago. Most of us have worked with internal systems where certs, for whatever reason, don't match the site, and it's surprising many people haven't noted that such doesn't raise any errors on iOS/OSX.
Though thinking back....I actually remember encountering exactly that on the iPad once, surprised that it didn't raise a flag. Like probably most I just shrugged and continued on.
Because it does not work like that. SecureTransport actually verifies the validity of the TLS certificate. If you hand it an invalid certificate in the first place, it will complain.
The issue is one level deeper. When you connect to a server that supports ephemeral key exchange, the parties involved will generate new key pairs on the fly to use for that connection. In order to make sure that the server has the key published in its certificate, the ephemeral public key shown to the client must be signed by the static private key of the certificate and the client MUST verify it. It is the ephemeral key signature that is not validated, not the signature on the certificate itself.
What happens here is that the server gives the client a _valid_ certificate for which it does not own the private key, and in the ephemeral step, it simply generates a key pair without a valid signature. At no point in the handshake the server is asked to verify its ownership of the private key associated to the certificate it is presenting
To summarize, TLS certificate should be valid, but you are never asked to verify you have its private key when the connection used DHE or ECDHE.
Does not NSA have access to multiple of the private keys of the preinstalled root certificates anyways? If so they can MITM any SSL connection where you don't specifically say which CA cert it should use?
Isn't it the case anyways that SSL is not very efficient against state sponsored attacks as almost all of them can generate certificates for any domain?
All this speaks towards it being a unfortunate mistake not something malicious, unless there is something wrong with my understanding how things work :-)
I believe the faulty function -- at line 574 -- is a copy/paste of another function located at line 328 in the same file.
I diff'ed both functions to try to understand the changes which may have been made.
The "goto fail;" is somewhat "drowned" into other changes around, so it stands out less using a diff tool because of these surrounding changes. The surrounding changes in question:
1) Moving a "SSLFreeBuffer(&hashCtx)" instruction at a higher position: Very strange since it accomplish absolutely nothing at its new position, as opposed to it's original position.
2) Renaming of two variables: "hash" became "hashOut" and "exchangeParams" became "signedParams".
3) Indentation of "goto"s
Without the above changes, the "goto fail;" stand out rather well when using a diff tool.
This shows another of the benefits for the community of open sourcing code - because we can see exactly where and what the bug was steps can be taken in other projects to stop it happening there (I think Adam mentioned he was going to check for a test case in Chrome).
If the code was closed all we would have is Apple's release note which just says validation steps were skipped...
Emerging tools such as gofmt and clang-format are able to automatically re-format your code based on the language rules rather than the human behind the monitor.
Using those tools this specific category of issues should at least be visible in the formating diff.
Interesting. Never thought about it that way before.
Those kinds of tools aren't new or emerging. The "indent" utility for C has been around for decades. I first remember using it on 4.3BSD, but it may have been around before that.
NAME
cb - C program beautifier
SYNOPSIS
cb
DESCRIPTION
Cb places a copy of the C program from the standard input on the stan-
dard output with spacing and indentation that displays the structure of
the program.
Worse, lint was part of the original C toolchain, but very few people cared to use it, because they couldn't be bored to tune the tool to their projects.
This is the type of error that any static analysis tool would easily pick.
Trying to get teams to agree on the code style is the hard part when using such tools. `gofmt` somewhat avoids that by defining one true way and only giving minor customization options.
clang-format has the same property. You just have to tell off a bunch of neckbeards and/or fire them if they can't agree to its formatting rules.
Having cast-in-stone formatting rules makes code review easier and shorter and avoids lots of arguments. For ultra-dangerous languages like C++, automatic formatting is indispensable.
Worth noting that static analysis finds bugs like this immediately. TLS code seems like the perfect candidate to run through static analysis on every checkin. There are products such as Coverity and PVS-Studio that would have immediately flagged this and probably some open-source ones built around LLVM as well (unsure about this one, though). I personally use Coverity and have it hooked up in the same way everyone connects Travis CI.
Regarding Adam Langley's comment about -Wall in gcc: -Wextra is what you want to have many more tests analysing your code. I wonder if this would have caught it.
A quick test shows neither -Wall nor -Wextra report anything on either gcc or clang. However, clang's -Weverything does complain (it implies -Wunreachable-code).
Confusingly, gcc accepts -Wunreachable-code as a valid option, but then proceeds not to warn anything. (edit): Which is a known bug apparently,
My personal observation is that while compilers continually get better at identifying straightforward mistakes, they don't have the same capability as a static analysis tool that works across compilation units. That is the real selling point of these tools. Definitely -Weverything/-Wexta plus static analysis for baseline checking.
My other beef is that compilers always add new warnings as options or behind new "catch all" flags like -Weverything that no one knows about. As long as each new warning can be individually disabled, there isn't a huge cost to pay by making much more of them enabled by default. Upgrading to a new compiler version usually requires a tiny bit of work, so adding a few -Wno-* rules for new things you want to disable until the code is clean (or forever) is a small price to pay for all new code getting the checks by default.
Apple introduced -Weverything because suddenly changing the scope of gcc's -Wall (which indeed does not include everything) would break a lot of builds.
Yep. Even ones that may not be fully baked, so some people caution against it.
We have it turned on, and about 10 warnings specifically disabled because they were too noisy or not useful for us. It's always interesting upgrading Xcode and seeing what new warnings we get to fix.
Excessive fear of backwards compatibility: you make -Wall. People use it. You add a new check which will reject some shoddy code. “What if people complain that their project no longer builds with -Wall? Let's add a new setting…”. Repeat…
I've been wondering how hard it would be to get to the point where the defaults are rigorous and developers have to opt out with specific -Wno-… options.
Microsoft VC++ has a -Wall warning level too, and it really does mean all warnings. Each major version of VC++ potentially introduces new warnings to -Wall.
And if you are going to disable a specific warning, it should only be for a limited section of code and include a detailed comment about why the warning is superfluous and why the code is safe.
If the code hasn't been linted, expect lots of false positives. I usually have to disable entire classes of less serious warnings to notice the errors, then slowly work through to the less serious stuff.
I'm calling you on the GOTO is evil thing. It's not. This is a well-structured use of GOTO, and it has a long tradition in C shops. This is a well-understood idiom, and you're seeing a different failure.
Your choices for exception handling systems in the C-like languages are:
- early return (whereupon, resource leaks and bloated cleanup code)
- setjmp/longjmp (whereupon, utter chaos)
- "chaining" success, where you don't use GOTO and have to consistently check your current 'err' state before doing additional "work" (whereupon, lots of bugs when you forget to handle an error case)
- or GOTO to a single piece of error handling code (this is usually coupled with an earlier hunk of code where you initialize variables and set up state; this is essentially the C++ "constructor / destructor" idiom, but done by hand).
The GOTO is the best choice, because everything else is worse.
(If you're using C++ exceptions you're in serious trouble. The effort required to write correct code in the presence of arbitrary exceptions is very high, and you're likely to get things wrong. Scott Meyers wrote like three whole books on the subject, which should be strong evidence that something is Very Wrong, and if you've ever used exceptions in a language that doesn't have garbage collection you'll probably agree (and even GC doesn't save you). Most production C++ code that I've seen use exceptions simply catches them in a bug-handling layer, which responds by scribbling some kind of report and then restarting the app).
Often these GOTOs are wrapped with macros that hide the fact that GOTO is being used. This can hurt code quality, since things are less clear, but in general they work well.
Basically they should have had the dead code warnings enabled, and listened to them, and done better checkin reviews. GOTO is not the enemy here. In paranoid code like this, generally you want to "claim success" as late as possible, so setting 'err' to some failure code would have been a better choice.
If you're using C++ exceptions you're in serious trouble.
Sorry, but I'm going to call you in turn on that one.
Of course there are some unwise things you can do in the presence of the C++ exception mechanism, and 15 or 20 years ago plenty of us did them. I think probably Herb Sutter deserves more credit than anyone else for drawing attention to these things, but the point is valid in any case.
However, we've figured out a lot since then, and for a long time writing exception-safe code from scratch in C++ has been quite straightforward. Just follow the common idioms, which mostly boil down to "use RAII for anything that needs cleaning up".
The biggest difficulties with exceptions in C++, IME, generally come from trying to retrofit them into code that was written without exceptions in mind. If your existing code doesn't necessarily follow even basic good practices for writing exception-safe code, the WarGames rule applies.
- Dijkstra said a lot of stupid things, this is one of them. Parsimonious use of GOTO is fine and sometimes leads to clearer code (exiting deeply nested conditions for example).
- Another stupid thing that Dijkstra said is that anyone who started by learning BASIC has their brain corrupted forever, which is total nonsense (I bet most people reading this started with BASIC and turned out just fine).
There is a context missing. The Dijkstra quote is really old (Wikipedia dates it to 1975) and do not talk about "modern Basic" we know, with a good support for structured programming -- it's about a language where GOTO's were used instead of proper loops. It's certainly still an exageration to speak about "brain corruption", but I think I know why he put such emphasis on the statement.
First, the problem here is totally different than the goto, it's the uncoditional execution of something that was meant to be in a if clause. It could have been anything, even without goto, and have the same problem.
Second, this is not the kind of use of goto that Dijkstra warned about. This is perfectly fine, and widely used from the best programmers.
Please don't fall into the trap of believing that I am terribly dogmatical about [the go to statement]. I have the uncomfortable feeling that others are making a religion out of it, as if the conceptual problems of programming could be solved by a single trick, by a simple form of coding discipline!
E. W. Dijkstra, cited by Donald E. Knuth in Structured Programming with go to Statements, ACM Computing Surveys, 6 (4), 1974
It could actually.
Exceptions and RAII are not resilient against typos and programmer error.
If the exception handler caught the wrong type, for example catching by value instead of reference, instead of having a catch(...) it would have the same problem as the above. It's harder to spot this sort of error than a double goto and incorrect parentheses - because you have to check the code that throws and the code that catches.
Admittedly in the scenario I lay out - what happens is an error is thrown and not caught, rather than a check being skipped, but the net effect is equivalent - and it takes 1 character out of place to invoke.
I don't see the possibility for catching the wrong type (or not catching it) if you get one character wrong. Unless of course you have two valid type names that just differ in one character, but that is a general pitfall and not exception specific.
If you always throw by value (and not by pointer) then it does not matter if you catch by value or by reference, the catch block will be activated either way. The only issue is possible slicing if you throw a derived instance and catch a base class type. You might lose error information that way, but the program will not crash due to an uncaught exception.
Well, since you're throwing an int-pointer then catch(int*) should also catch it quite happily.
I guess you mean there will only be a catch(int) in addition to catch(...) so in that aspect you're right of course.
Still, I would think taking the address of something you throw should ring quite a few warning bells as opposed to merely a missing ref (&) in the catch handler. Similarly to "return &e" which would also be suspicious and require an extra look or two.
Well it was mostly a tongue-in-cheek comment (was thinking of yesterday's C++ exception topic), but essentially true as you would not have used the "if + goto" with C++ error handling. Of course you can write the same bug (and more) in C++ too.
No, you're worse off in C++, where the semantics of exceptions (especially in the area of resource allocation and deletion) are complex and hard to reason about.
I've done a bunch of kernel and other systems work in C and in C++, and my experience is that the C is a lot clearer. This type of code is all about not having magic side effects; everything needs to be in the open and very plain, or bugs start to get pretty subtle and hairy.
I'm not saying "Don't do systems programming in C++", because clearly you can. But it takes discipline to succeed, probably more discipline than you need to apply than if you're writing C.
I don't know about exceptions (they are forbidden where I work) but any amount of C++ would significantly improve this implementation. There is code all over this library that allocates and deletes buffers in structs passed in as function arguments, i.e. other code's structs that nobody should be dicking around with except through constructors, destructors, and accessors.
This code would also be about half as long in straight-forward C++.
Not very well tested, please let me know if it works for you. If you're on OS X Mavericks or on iOS 7 and haven't patched you should get big scary red text.
Not very well tested, please let me know if it works for you. If you're on OS X Mavericks or on iOS 7 and haven't patched you should get big scary red text.
I noticed a while ago that while Safari supports TLS 1.2, but Mail does not. So somewhere in the implementation they are using different code. (Not agreeing or disagreeing, just mentioning an observation.)
Yes, I did it this morning. Look for the update from your phone, so it's a smaller download anyway. It takes more room than they ask for, maybe 750 meg. I just temporarily deleted some podcasts to make room.
There's not a whole lot I can do about that without adding a lot of complexity. You could try downloading https://gotofail.com:1266/test.png I suppose.
I'm going with accident either through a merge or a double paste. I'm guessing that if they were running some kind of static analysis tool that it probably would have flagged this one as a duplicated line and someone would have caught it earlier.
Unfortunately OSX does not appear patched even in the latest developer center 10.9.2 build (13C62). Tested in both Safari and OS-distributed curl. Chrome/Firefox of course is still fine since it uses the NSS stuff, but plenty of OS services use the OS crypto. (I'm violating NDA by commenting on pre-release Apple stuff, of course.)
Windows or ubuntu bootcamp until they fix this, I think.
MITM of any SSL connection in Safari and other system apps, and they couldn't even bother to have an OS X patch ready at the same time as the disclosure for iOS? I think anyone relying on the security of OS X is going to have to seriously rethink their OS choice after this.
Tell that to the people who generated keys on Debian.
Programmers are simply not good enough at writing secure code. Full stop. If you say anything else, you're just flaunting your own unreliability as a source of security advice.
At least that was public and fixed. I think the point is that keys could be completely deterministic on closed source systems and there is no way to know. It's very easy to have deterministic "random" sources that pass statistical tests for randomness.
How is what I said not true? If the same thing as the Deb prng bug happened in a closed source system, it could sit for a couple of years exposing thousands/millions of systems and then be patched quietly to avoid embarrassment, leaving everyone vulnerable.
Static analysis would have caught this immediately. It caused unreachable code that the compiler even recognized and optimized away.
You're saying Google Chrome doesn't test that an ephemeral key is actually signed by the cert's private key? If that's the case, that's completely unacceptable because it's the whole point of the protocol.
I think the various BSD folks, especially those from the OpenBSD team, would like to have a word with you. There are many other unixes out there that put a strong, probably even stronger than linux, emphasis on security.
Um, sure, after it updates. Assuming a similar bug isn't found there and exploited when it connects to the Internet. Since, you know, it can't do anything without a network connection. Just playing devils advocate here, I like ChromeOS too.
This. Canned my MacBook Pro over this. It's been a bumpy relationship anyway. Just on Thursday Apple Mail stopped sending mail to my SMTP server. Final straw for me.
Since the source code is available, might it be possible to produce a hot patch to the binary so that those of us running Mavericks won't have to go through the next few days or weeks with our pants down? It would be a simple matter of finding the JMP instruction generated by the second GOTO and replacing it with no-ops. How hard could it be,at least for someone who actually knows their way around OS X binary file formats?
Likely the compiler will have optimized away the rest of the function as dead-code, so it's not as simple as just getting rid of the unconditional jump.
It appears to be in a cursory glance. Loaded for me in Xcode. That said, signing the security framework would be another issue ;-) I can't imagine apple would let malware replace security binaries that easily.
(Caveat: I haven't tried this myself!)
Here's the analysis and an experimental patch from @i0n1c for the 64bit version of Mavericks' Security.framework (32-bit remains unpatched):
http://sektioneins.de/blog/14-02-22-Apple-SSL-BUG.html
I'll admit I don't think of myself as great at unit testing, but the first thing I do when writing one for a new tool or class is use a code coverage tool to look for uncovered lines and once I have written a few basic behavioral tests I write tests to exercise and validate the output of the uncovered lines.
This ensures that the tests I write to cover the public API don't miss catching internal behaviors that require more subtlety to uncover.
My theory is that Apple is spread too thin. Kayak.com reproducibly crashed MobileSafari the day iOS7 shipped, and brand new iPad minis regularly kernel panic and reboot.
An integration test wouldn't catch this one. You need a specific malicious SSL server (presents a valid certificate, uses ephemeral mode, does not present a valid signature proving that it owns the private key). The code does validate certificates, otherwise this would've been caught ages ago by anyone trying an invalid cert.
Well, as the article says: A test case could have caught this, but it's difficult because it's so deep into the handshake. One needs to write a completely separate TLS stack, with lots of options for sending invalid handshakes.
Something one would have reasonably expected after second biggest company in the world (NASDAQ, Fortune 50), and first in the IT sector.
The entire OS is damn too huge piece of software for Apple not to test intesively to a fanatic stretch. Since all the NSA revelation and the proven extent to which US government lied and continues to lie to its people about all data gathering, I fail to categorize such a simple but yet brilliant overlook as a mistake.
I installed this last night and the update came in at 15MB. Surely a one line bug shouldn't cause a 15MB update? Or is it that some things in iOS might be statically linked and those had to be pushed out as well?
The update probably includes the actual updater executable, not just the diff. This would allow you ship different kind of patches (from simple fixes to entire OS upgrades) using the same mechanism.
The dynamic libraries in iOS aren't shipped as separate dylibs on the device. Rather, they're essentially concatenated together as part of a single file called the dyld shared cache that also lets Apple do some prebinding tricks, interning of objc selector names across all system libraries, etc.
The system-wide iOS build scripts actually randomize the order of the libraries in the shared cache by default (you can check this using dyld_shared_cache_util against the shared cache file, which you can compile from the dyld project on Apple's open source site). Since the ordering of dylibs in this giant shared cache file varies between builds, you could easily end up with a 15 MB diff even though all you've done is deleted a single goto in source code.
The faulty block of code -- line 623 (function SSLVerifySignedServerKeyExchange) -- looks like a cut and paste (or the reverse) of the block of code at line 372 (function SSLSignServerKeyExchange), except this one doesn't have the extra "goto fail;".
I don't think you necessarily need unit tests to catch this; code review would work (even I probably would have caught this if I'd actually read it; "fucking gotos" would have drawn my attention to begin with, and if it was a single line commit, even more so.)
The problem is Apple (intentionally) understaffs and overworks, so I doubt they have the spare people to look at most commits.
I would be really surprised if the rest of the function was still there - this seems like the perfect example for a dead code elimination compiler pass.
The static function that has the bug will most likely be inlined into the other static function which will then be inlined into the outer public symbol that calls it.
I believe you can dump the assembly of the entire function like this:
otool -t -p _SSLProcessServerKeyExchange \
-V /System/Library/Frameworks/Security.framework/Versions/A/Security | less
edit: I believe this could be the offending instruction:
0000000000086df6 jmp 0x86e0d
edit2: That is actually in SSLVerifySignedServerKeyExchangeTls12(). Trying to track down the real one..
edit3: Looks like the compiler did optimize it out if I'm reading it correctly now. The code is around 0x86c97. It does the last if() call and then immediately calls SSLFreeBuffer() and jumps to the end. :(
The code is crap and while I can understand why a single person might have written it this way, I think any organization of full-time professional software developers should be collectively embarrassed to have accepted it. The only reason to use the "goto fail;" idiom is to free two buffers before returning. But the buffers in question are just sslBuffer structs that live on the local stack. Their destructors will be called, in reverse order, regardless of how the function is exited. If this code simply had a fucking destructor for sslBuffer to delete the data pointer, none of the rest of these call sites for SSLFreeBuffer would need to exist at all! And there's 24 such call sites in this file alone. Anyone with any sense would chose the 2-line destructor over dozens of calls to free buffers.
There's even this bullshit:
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
goto fail;
Which is hilarious, because the only way for SSLFreeBuffer to fail is if the buffer doesn't exist, but hashCtx is a local temporary object and it MUST exist. Anyway the only thing we do by jumping to fail is to free the non-existent object once again.
Yes, one makes a conscious choice of implementation language. They aren't dictated by the laws of physics. Choosing your implementation language is a rather important step.
this pattern is structured to avoid the use of an additional stack (local) variable to track a condition, and to skip superfluous execution of code once that condition is detected.
i've used it myself to optimize inner loops of very simple un-accelerated graphics rendering code (which sped it up considerably, since it is potentially skipping many levels of unnecessary execution on the cpu, millions of times), but i agree with some posters here, using it in a security context like this is a bit daring.
still, having said that, this bug really is an indictment of the testing process and not really bad style per se.
The code already has that condition variable ('err'), the goto is to avoid execution of code that should not be executed. It's not about avoiding additional stack -- that was probably the last thing on the programmer's mind. The idiom is about error handling.
292 comments
[ 5.7 ms ] story [ 361 ms ] threadOutside of python, for obvious reasons, it should never be allowed. The slightly nuts conditionals in Erlang are justifiable for avoiding exactly this kind of problem.
While Go has goto for tricky situations like this, because it has defer you don't have to use it often, assuming the free calls were needed (and the vars were not going to be GC'd):
err = SSLHashSHA1.update(&hashCtx, &serverRandom); if (err != nil) { return err; }
Is there objective evidence for this? As a Go programmer a semicolon in an if statement screams to me. I can see it possibly being in issue for new Go programmers- but I don't remember it being one for me.
I rarely use the one-line `if err := ...; err !=nil ` idiom because its quite a mouthful. However when I do, I try to make sure it's not too much to grasp at once. Here the extra `else` goes against that.
Alright, I know this is just a quick snippet on HN and all, I just thought I'd mention it anyways. Maybe next time you actually write that in code you'll think about my point. ;)
This is not spaghetti code or what Dijkstra talked against.
It's one of the two things one hears novices: that gotos are to always be avoided (because they heard that they are bad), and that we should write stuff in assembly (cause they heard that it's fast).
This is a perfect example of just how bad that suggestion is, and just how disastrous it can be. It's totally worth typing a few extra characters here and there to basically avoid these kind of situations completely.
This also applies to the "JS without ;" crowd.
You may think you're too good to know all the rules of ; or you can just don't think about it, and worry about other things instead, like your code.
I certainly see far more bugs caused by an improperly inserted semicolon than an improperly omitted semicolon, but then I'm looking at jshinted code most of the time.
IIFEs are the most common source of semicolon problems:
A less common sort of pattern that I still use pretty often as a DRY measure: This is clean and readable, and without semicolons it's completely wrong.Now sure, you can prepend semicolons to these specific cases, and that will work. Here's why I dislike that:
1. Those leading semicolons look weird. To the uninitiated, it looks like a typo, and that's a significant hazard if anyone else is going to touch your code.
2. While there are only a handful of special rules like this to remember, there's only one rule to remember if you don't rely on ASI.
Your examples will crash immediately and at the right spot though. The problems I see caused by excessive use of semicolons are often far weirder.
That said, inadvertent errors caused by semicolon insertion are still more common and baffling (especially by people addicted to jslint who use a variable declaration idiom particularly easy to screw up with errant semicolons).
A relatively rare scenario, but a brutal one to debug.
As for errors caused by extra semicolons, they can be weird, but I don't think I've ever actually hit one in practice. They'd also be a little easier to spot, since you tend to develop a reasonable instinct for where semicolons belong.
Hence these are the cases where more time and resources will be wasted because of it.
"I certainly see far more bugs caused by an improperly inserted semicolon"
What would be an example of this? Because I've seen exactly zero bugs of this type (not counting typos, of course)
(usually this will be across multiple lines)
...just overwrote c in a different scope. This kind of bug is common, idiomatic, baffling, and actually more likely among coders subscribing to javascript "best practices".
This doesn't justify playing a guessing game and skipping semicolons just because you think you know all the rules about not using them.
if(something) {do_something();}
Many code editors will easily insert curly brace pairs, so it's really just a single extra key stroke you're saving.
That's fine until you write
or worse: I've actually seen something very similar to that one.You haven't really changed the dimensions of the problem by putting it all one one line. Only the whitespace is different. Sure it looks wrong to you; but so does the incorrect code from apple.
I don't understand the need to omit braces; it doesn't make the compiled program any smaller. And denser source code is not always more readable, or we'd prefer "? :" to "if" every time.
if (something); { do_something(); }
So..
The first line of defence is consistent code layout; e.g. always use "{ }" after an if, and only one statement per line. This aims to make simple typos less likely to compile, make the code more readable and so make errors stand out.
Then there is using and paying attention to the linter / compiler warnings / jshint + 'use strict' / static analysis or whatever it’s called in your language of choice.
Then there are unit tests or integration tests.
Any of these might have caught the apple error, but there are no absolute guarantees.
IMHO it's a professionalism issue – gifted amateurs are great, and they can succeed... most of the time. But a professional engineer would put aside ego "I won’t make mistake like that" and laziness "I don’t need to type braces this time" and do the careful thing every time. Because each step raises the bar, and makes it harder to ship code with bugs like this. Because sometimes it matters.
This is what happens if you try the same thing in it http://imgur.com/syt6LuU.
I have emacs setup to auto run clang-format on each save of cc-mode files. I can't NOT notice this stuff now.
Two gotos immediately after one another at the same indentation level is obviously wrong by visual inspection.
Proper indentation might have led the casual viewer to think that the code was actually ok!
http://astyle.sourceforge.net/
They exist because of the long, bloody history of mistakes by very smart people.
As far as the original example goes, if it's an error it's most likely a copy/paste error. Curly braces help there, too. With three times as many lines in the block, the odds of a paste error resulting in code that even compiles is greatly reduced, and a compiler error should call attention to the issue.
Even assuming the bug was malicious, the curly braces would increase the odds of it being caught by another developer. This is particularly the case now that offside rule languages are common. A large chunk of younger devs cut their teeth on languages where
doesn't look odd in the slightest. But I think that the 2nd example above would still look immediately bizarre to nearly everyone.Right, and this demonstrates the major problem with verbosity in languages and APIs and design patterns. When you have to repeat yourself many times, it's very easy to make a mistake in one of the near-copies, and you or a code reviewer can miss it because you'll tend to quickly skim over the boilerplate.
For cases like this, using exceptions rather than manual error value checking would make your code shorter, less redundant, and not susceptible to this particular bug. (Not possible in C, I know).
But I wonder if there's still room to tighten up the code. Perhaps something like
If there were a language feature available to do this more elegantly, whether exceptions or something else such as a Haskell-style monad with fail semantics, I'd almost certainly agree with using it in preference to a series of manual checks, though.
If not, `|| (err = check())` is equivalent to separate checks which also also `goto fail` immediately.
Left-to-right, short-circuit evaluation of the logical operators is so deeply rooted in C idiom that a mainstream compiler that doesn't respect it would be practically unusable. But perhaps it wouldn't work in a hobbyist compiler, or a non-standard dialect of C.
While valid according to the language definition, assignments within conditional expressions do have a reputation for being error-prone and not always the easiest code to read. Sometimes they're still neater than the available alternatives.
However, IMHO in this case, it feels a bit too easy to break the code during later maintenance edits. For example, someone might simplify the code by removing the explicit comparisons with 0 that are unnecessary here, and then later someone else might "fix" an "obvious bug" by replacing the "(err = x) || (err = y)" construction with the common construction "(err == x) || (err == y)" that they assumed was intended, perhaps prompted by a compiler warning. Obviously they shouldn't if they didn't properly understand the code, but when has that ever been a reliable guarantee? :-)
But yes, mandatory braces on the same line is the correct choice.
Lots of points in favor or against:
1) It's a huge compromise, and "open" to anyone to exploit, which would ultimately get caught and fixed faster. But it's also not targeting anything specific, so there's less of a signature of the attacker.
2) Incredibly simple, and thus a plausible mistake.
3) Hidden in plain sight
I'd generally come down on the side of "accident". The better question is if an systematic testing system for exploitable weaknesses (or a lucky chance finding) could find something like this (either a regression, or a badly implemented new feature) and exploit it -- basically assuming there are going to be errors in implementations. That's understood to be a standard technique in the IC, and a whole industry built around it... and then, the scale of compromise.
There are lots of mitigation techniques for vulnerabilities like this (essentially, devices which are too fast-changing and too difficult to fully trust, but which have to touch sensitive data), but it's not as if people can carry around a firewall in their pocket these days, sadly.
I'm certainly re-generating any keys which touched iOS or OSX devices (thankfully very few), and reinstalling OSes, in the interim.
This might explain DROPOUTJEEP particularly considering how much more efficient it is than breaking messages after they are encrypted.
http://mobile.eweek.com/security/nsa-spying-on-apple-iphones...
That and inadequate, bordering on zero, code review. Even a beginner C programmer looking at this code could see how fishy it looks.
No code review while checking in code to libssl. That takes a lot of incompetence.
And that fact that this bug and terrible coding style was in the publicly available source code for so long totally disproves ESR's "many eyes make all bugs shallow" myth. Thanks for the false sense of security, Eric. The chickens have come home to roost again.
If "many eyes make all bugs shallow" were true, somebody would have raised the flag "Hey everybody, these Bozos are actually omitting brackets in their SSL code! Somebody could carelessly insert a statement and accidentally (or maliciously) introduce a hard to see bug some day!"
Hardly anybody actually bothers to read code in the real world. So there aren't "many" eyes, and even if there were, many bugs aren't shallow even to expert eyes, and "all" bugs will never be shallow to most eyes.
That's why it's important to pay competent security professionals to actually take the time and effort to audit source code, which is difficult work that requires much time and effort that takes them away from other valuable, high paying, less tedious and mind numbing work.
"Any sufficiently advanced incompetence is indistinguishable from malice"
https://support.apple.com/library/APPLE/APPLECARE_ALLGEOS/HT...
We begin therefore where they are determined not to end, with the question whether any form of democratic self-government, anywhere, is consistent with the kind of massive, pervasive, surveillance into which the Unites States government has led not only us but the world.
This should not actually be a complicated inquiry.
http://snowdenandthefuture.info/events.html
In many code editors, numerous fat fingered shortcuts could produce such a compilable line duplication/deletion (deletion because maybe that's not a goto line duplicated but a test line deleted).
I'm baffled that neither clang nor gcc spits a warning for the unreachable code.
Not trying to shift blames or anything, but rather than do some finger pointing, I'd rather see this as a warning for all of us to ramp up our game and better our tools so that everyone benefits, reducing the risk of both honest and intentional or covert[1] malfunctions.
[0]: example custom shortcut as ^d to duplicate (could have been cmd(+shift)+D, which sits right along cmd(+shift)+S): http://www.xinsight.ca/blog/xcode-trick-creating-a-shortcut-...
[1]: http://en.wikipedia.org/wiki/Underhanded_C_Contest
I don't believe I've been a victim, and most of the ways you'd exploit this would leave enough traces to be high-risk-of-detection, I think.
But, it's hard to prove a negative, so it's safer to act as if it had been compromised.
(I'm also updating keys anyway, so this is just a matter of waiting a day or two to do so.)
The scary thing is the bar is so low; even I could turn this into a nice weaponized thing to go after the long tail of people who don't upgrade, for the next months. The "diode" etc. infrastructure NSA built isn't that different from botnet C&C or the kind of relays people have when hacking.
I wish more people understood this. Not to let NSA off the hook (what they are doing is awful), but the threat posed by the NSA is a higher-level down the road/slippery slope threat.
There are other more immediate and real dangers out there that are actively trying to steal whatever they can find.
As a matter of fact, by deploying one of those the beginning of the macro, one in the middle, and one at the end, you can set up what's called a "trifor(;;);ce field", an anti-anti-pattern that is documented on the c2 wiki thusly:
The trifor(;;);ce has the power to grant the wish of whomever touches it, and molds the Sacred Realm to reflect that person's heart. The trifor(;;);ce does not discriminate between "good" or "evil". However, if a person without an equal balance of power, wisdom, and courage makes a wish, the trifor(;;);ce will split into its three separate parts: the piece that best personifies the wish-maker will be the only piece to remain in hand, whilst the other two will take residence in whosoever most personifies them. Reassembly is then required for such a person's wish to be granted, but does not exclude another from making the attempt.
No, I'm just joking and making shit up. Don't anyone ever do that! As bitchy and pedantic as gcc and clang are about other things, I'm disappointed they don't complain about that.
I'm baffled that this wasn't caught long, long ago. Most of us have worked with internal systems where certs, for whatever reason, don't match the site, and it's surprising many people haven't noted that such doesn't raise any errors on iOS/OSX.
Though thinking back....I actually remember encountering exactly that on the iPad once, surprised that it didn't raise a flag. Like probably most I just shrugged and continued on.
The issue is one level deeper. When you connect to a server that supports ephemeral key exchange, the parties involved will generate new key pairs on the fly to use for that connection. In order to make sure that the server has the key published in its certificate, the ephemeral public key shown to the client must be signed by the static private key of the certificate and the client MUST verify it. It is the ephemeral key signature that is not validated, not the signature on the certificate itself.
What happens here is that the server gives the client a _valid_ certificate for which it does not own the private key, and in the ephemeral step, it simply generates a key pair without a valid signature. At no point in the handshake the server is asked to verify its ownership of the private key associated to the certificate it is presenting
To summarize, TLS certificate should be valid, but you are never asked to verify you have its private key when the connection used DHE or ECDHE.
Isn't it the case anyways that SSL is not very efficient against state sponsored attacks as almost all of them can generate certificates for any domain?
All this speaks towards it being a unfortunate mistake not something malicious, unless there is something wrong with my understanding how things work :-)
I diff'ed both functions to try to understand the changes which may have been made.
The "goto fail;" is somewhat "drowned" into other changes around, so it stands out less using a diff tool because of these surrounding changes. The surrounding changes in question:
1) Moving a "SSLFreeBuffer(&hashCtx)" instruction at a higher position: Very strange since it accomplish absolutely nothing at its new position, as opposed to it's original position.
2) Renaming of two variables: "hash" became "hashOut" and "exchangeParams" became "signedParams".
3) Indentation of "goto"s
Without the above changes, the "goto fail;" stand out rather well when using a diff tool.
If the code was closed all we would have is Apple's release note which just says validation steps were skipped...
Interesting. Never thought about it that way before.
You want the compilers knowledge of the code for this, e.g. while formating C++ template mess.
There's a tight integration into the actual toolchains, which is a good thing.
This is the type of error that any static analysis tool would easily pick.
Having cast-in-stone formatting rules makes code review easier and shorter and avoids lots of arguments. For ultra-dangerous languages like C++, automatic formatting is indispensable.
Confusingly, gcc accepts -Wunreachable-code as a valid option, but then proceeds not to warn anything. (edit): Which is a known bug apparently,
http://gcc.gnu.org/ml/gcc-help/2011-05/msg00360.html
My other beef is that compilers always add new warnings as options or behind new "catch all" flags like -Weverything that no one knows about. As long as each new warning can be individually disabled, there isn't a huge cost to pay by making much more of them enabled by default. Upgrading to a new compiler version usually requires a tiny bit of work, so adding a few -Wno-* rules for new things you want to disable until the code is clean (or forever) is a small price to pay for all new code getting the checks by default.
We have it turned on, and about 10 warnings specifically disabled because they were too noisy or not useful for us. It's always interesting upgrading Xcode and seeing what new warnings we get to fix.
I've been wondering how hard it would be to get to the point where the defaults are rigorous and developers have to opt out with specific -Wno-… options.
The problem are the many developers that still think they are perfect and know the full C standard, including undefined parts.
Static analysis, regression tests and turning on all the warnings you possibly can should be mandatory, especially for such critical pieces of code.
Many of the C guys I met along my career thought otherwise.
> Static analysis, regression tests and turning on all the warnings you possibly can should be mandatory, especially for such critical pieces of code.
+1
It takes a special kind of ego to write an SSL library with no unit tests, not turn on compiler warnings, and not use static analysis tools.
Error -> Warning 527 Unreachable code at token 'ret' (col 12)
I'd like to point out the case where Debian maintainers "fixed" a "bug" discovered by static analysis; https://blog.isotoma.com/2008/05/debians-openssl-disaster/.
https://github.com/landonf/Testability-CVE-2014-1266/blob/ma...
In all seriousness it's unfortunate the C family of languages even allow this kind of bug - we've all been bitten by it at one time or another.
Your choices for exception handling systems in the C-like languages are:
- early return (whereupon, resource leaks and bloated cleanup code)
- setjmp/longjmp (whereupon, utter chaos)
- "chaining" success, where you don't use GOTO and have to consistently check your current 'err' state before doing additional "work" (whereupon, lots of bugs when you forget to handle an error case)
- or GOTO to a single piece of error handling code (this is usually coupled with an earlier hunk of code where you initialize variables and set up state; this is essentially the C++ "constructor / destructor" idiom, but done by hand).
The GOTO is the best choice, because everything else is worse.
(If you're using C++ exceptions you're in serious trouble. The effort required to write correct code in the presence of arbitrary exceptions is very high, and you're likely to get things wrong. Scott Meyers wrote like three whole books on the subject, which should be strong evidence that something is Very Wrong, and if you've ever used exceptions in a language that doesn't have garbage collection you'll probably agree (and even GC doesn't save you). Most production C++ code that I've seen use exceptions simply catches them in a bug-handling layer, which responds by scribbling some kind of report and then restarting the app).
Often these GOTOs are wrapped with macros that hide the fact that GOTO is being used. This can hurt code quality, since things are less clear, but in general they work well.
Basically they should have had the dead code warnings enabled, and listened to them, and done better checkin reviews. GOTO is not the enemy here. In paranoid code like this, generally you want to "claim success" as late as possible, so setting 'err' to some failure code would have been a better choice.
But using GOTO wasn't a mistake.
Sorry, but I'm going to call you in turn on that one.
Of course there are some unwise things you can do in the presence of the C++ exception mechanism, and 15 or 20 years ago plenty of us did them. I think probably Herb Sutter deserves more credit than anyone else for drawing attention to these things, but the point is valid in any case.
However, we've figured out a lot since then, and for a long time writing exception-safe code from scratch in C++ has been quite straightforward. Just follow the common idioms, which mostly boil down to "use RAII for anything that needs cleaning up".
The biggest difficulties with exceptions in C++, IME, generally come from trying to retrofit them into code that was written without exceptions in mind. If your existing code doesn't necessarily follow even basic good practices for writing exception-safe code, the WarGames rule applies.
- The bug here has nothing to do with GOTO.
- Dijkstra said a lot of stupid things, this is one of them. Parsimonious use of GOTO is fine and sometimes leads to clearer code (exiting deeply nested conditions for example).
- Another stupid thing that Dijkstra said is that anyone who started by learning BASIC has their brain corrupted forever, which is total nonsense (I bet most people reading this started with BASIC and turned out just fine).
First, the problem here is totally different than the goto, it's the uncoditional execution of something that was meant to be in a if clause. It could have been anything, even without goto, and have the same problem.
Second, this is not the kind of use of goto that Dijkstra warned about. This is perfectly fine, and widely used from the best programmers.
The error is in the lack of an if guard.
E. W. Dijkstra, cited by Donald E. Knuth in Structured Programming with go to Statements, ACM Computing Surveys, 6 (4), 1974
Admittedly in the scenario I lay out - what happens is an error is thrown and not caught, rather than a check being skipped, but the net effect is equivalent - and it takes 1 character out of place to invoke.
If you always throw by value (and not by pointer) then it does not matter if you catch by value or by reference, the catch block will be activated either way. The only issue is possible slicing if you throw a derived instance and catch a base class type. You might lose error information that way, but the program will not crash due to an uncaught exception.
int e;
int update() { throw &e; }
the & is the one character - it's a typo - only catch(...) will catch it.
I guess you mean there will only be a catch(int) in addition to catch(...) so in that aspect you're right of course.
Still, I would think taking the address of something you throw should ring quite a few warning bells as opposed to merely a missing ref (&) in the catch handler. Similarly to "return &e" which would also be suspicious and require an extra look or two.
The same applies to any other language that allows for copy-paste of C code.
At least, short of making it mandatory it would at least signal bad style.
I've done a bunch of kernel and other systems work in C and in C++, and my experience is that the C is a lot clearer. This type of code is all about not having magic side effects; everything needs to be in the open and very plain, or bugs start to get pretty subtle and hairy.
I'm not saying "Don't do systems programming in C++", because clearly you can. But it takes discipline to succeed, probably more discipline than you need to apply than if you're writing C.
This code would also be about half as long in straight-forward C++.
https://gotofail.com/
Not very well tested, please let me know if it works for you. If you're on OS X Mavericks or on iOS 7 and haven't patched you should get big scary red text.
https://gotofail.com/
Not very well tested, please let me know if it works for you. If you're on OS X Mavericks or on iOS 7 and haven't patched you should get big scary red text.
Edit: posted here https://news.ycombinator.com/item?id=7282164
Looks like using Chrome instead of Safari may help; I'd say it would be more interesting if standard mail client can be fooled.
how do I patch on OS X Mavericks? Software update shows nothing to update
Install Ubuntu.
I kid, I kid.
Client's that aren't vulnerable should flip out when trying to load that.
Windows or ubuntu bootcamp until they fix this, I think.
Security is not a Boolean. A better question is, which OS is more secure OOTB for a given user.
https://www.gov.uk/government/publications/end-user-devices-...
Tell that to the people who generated keys on Debian.
Programmers are simply not good enough at writing secure code. Full stop. If you say anything else, you're just flaunting your own unreliability as a source of security advice.
(For example, Github lets you push/pull via SSH as git@github.com, and they determine your identity by the SSH key used.)
You're saying Google Chrome doesn't test that an ephemeral key is actually signed by the cert's private key? If that's the case, that's completely unacceptable because it's the whole point of the protocol.
http://research.swtch.com/openssl
You're kidding, right?
http://www.debian.org/security/
Edit:
There were multiple security issues in OpenSSL as recently as January.
http://www.debian.org/security/2014/dsa-2833
http://www.androidcentral.com/samsung-lock-screen-bypassed-e...
You might find this article a good read: http://www.theverge.com/2014/1/21/5307992/inside-the-mind-of...
It's mainly an SSH and RDP terminal anyway.
OSX quality and QA is extremely bad.
Not that we really care. And you could have gotten the beta off of some torrent or whatever, without ever agreeing to the NDA anyway...
I think it'd break one of the binaries, but I could replace that with the real one.
I'll admit I don't think of myself as great at unit testing, but the first thing I do when writing one for a new tool or class is use a code coverage tool to look for uncovered lines and once I have written a few basic behavioral tests I write tests to exercise and validate the output of the uncovered lines.
This ensures that the tests I write to cover the public API don't miss catching internal behaviors that require more subtlety to uncover.
...or an integration test. or a functional test.
When you write an SSL lib, I suspect that at some point you ought to test that it checks f%^&ing certificates :/
My theory is that Apple is spread too thin. Kayak.com reproducibly crashed MobileSafari the day iOS7 shipped, and brand new iPad minis regularly kernel panic and reboot.
It wasn't always like this.
Someone badly needs to sit down and write — preferably a black-box, so it can be used for all — testsuite for SSL/TLS.
Unit tests would've caught this, though.
The entire OS is damn too huge piece of software for Apple not to test intesively to a fanatic stretch. Since all the NSA revelation and the proven extent to which US government lied and continues to lie to its people about all data gathering, I fail to categorize such a simple but yet brilliant overlook as a mistake.
seriously, tho, it is probably the whole binary this was part of.
The system-wide iOS build scripts actually randomize the order of the libraries in the shared cache by default (you can check this using dyld_shared_cache_util against the shared cache file, which you can compile from the dyld project on Apple's open source site). Since the ordering of dylibs in this giant shared cache file varies between builds, you could easily end up with a 15 MB diff even though all you've done is deleted a single goto in source code.
The problem is Apple (intentionally) understaffs and overworks, so I doubt they have the spare people to look at most commits.
You can't really blame them for doing the best they can with limited resources. Startup life is hard.
It should relatively simple to NOP out the 2nd goto, if the rest of the function hasn't been optimised away.
I believe you can dump the assembly of the entire function like this:
edit: I believe this could be the offending instruction: edit2: That is actually in SSLVerifySignedServerKeyExchangeTls12(). Trying to track down the real one..edit3: Looks like the compiler did optimize it out if I'm reading it correctly now. The code is around 0x86c97. It does the last if() call and then immediately calls SSLFreeBuffer() and jumps to the end. :(
There's even this bullshit:
Which is hilarious, because the only way for SSLFreeBuffer to fail is if the buffer doesn't exist, but hashCtx is a local temporary object and it MUST exist. Anyway the only thing we do by jumping to fail is to free the non-existent object once again.In short: wow.
Not in C.
i've used it myself to optimize inner loops of very simple un-accelerated graphics rendering code (which sped it up considerably, since it is potentially skipping many levels of unnecessary execution on the cpu, millions of times), but i agree with some posters here, using it in a security context like this is a bit daring.
still, having said that, this bug really is an indictment of the testing process and not really bad style per se.