Why was "goto fail;" added without any other change to that part of the code?
Looking at the diff between the two versions of sslKeyExchange.c released by Apple http://opensource.apple.com/source/Security/Security-55471/libsecurity_ssl/lib/sslKeyExchange.c and http://opensource.apple.com/source/Security/Security-55179.13/libsecurity_ssl/lib/sslKeyExchange.c
I was trying to come up with a reasonable explanations of how this could have happened, but failed.
Here the relevant part of the diff:
@@ -627,6 +628,7 @@
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;
+ goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
goto fail;
85 comments
[ 3.5 ms ] story [ 153 ms ] threadI disagree that there's no "reasonable explanation", and so do most other people who've looked at it. That doesn't mean it wasn't malicious, but it means that it has reasonably plausible deniability. (Of course, if you were being malicious, plausible deniability might be important to you.)
Perhaps the developer deliberately allowed that mismerge to give himself an aura of incompetence/sloppiness, enhancing his plausible deniability when it is discovered that one of his other changes led to an insidious vulnerability. But more likely it was just an accident - if he were malicious, the effect of bringing attention to his code would be highly undesirable.
It's not unreachable - it always executes if the if condition is false.
I'm not saying there is no reasonable explanation, just that I am not able to come up with one. Many explanations I have seen so far, require additional code changes in that block and make no sense to me looking at the diff.
Or a conspiracy. At this poing, I just don't know any more.
The lesson is to always make sure to look at your diffs before you commit your changes.
Google / Chrome does this, and so should everyone else, even more so if they're dealing with security.
It also makes me wonder if the diff display shouldn't have a specific notation for duplicated lines, since they are a special case that can easily be mistaken for a trivial change to a line with the way diffs are typically shown.
And sometimes impossible
In Python/Ruby sure, it's easy. In Java, or even C++, it's doable, but not great, and it requires some heavy tricks.
But they surely need tests, but not necessarily TDD.
This is what I expect from an OS developers
Maybe the people from OpenSSL have some test like that already
I've been in that position. Sometimes with embedded, you can't have software fixtures, only hardware ones. (Bit banging)
And you think you know what the device answers and understands, but there's usually some gotchas.
Sure the startup culture and new breed (new bubble?) tech companies mostly emphasise it, but among older companies it's much less common for a few reasons:
1. It just wasn't considered normal practice until fairly recently. 2. They often use 'lower level' languages, and as @raverbashing mentions, they're more difficult to test. 3. The companies are much slower moving, so arguably have less of a risk with not having automated testing. Automated testing has more benefits in agile than it does in the waterfall methodology.
The bug happens very deep inside the handshake protocol and it's extremely hard to test it automatically.
If the code doesn't belong there, get rid of it. If, for whatever reason, you want to show the reader that code used to be there, delete it and leave an explanatory comment. If you're commenting it out because you're not confident in taking it out or you plan to re-enable it in the future, well, that's what you have source control for.
My commit suggestion: - goto fail; + fail;
if i were to take a guess, the programmer first typed out all the if conditional expressions, then went in and pasted 'goto fail' out of a buffer after each one, and accidentally did it twice there.
plese use some imagination before jumping to conclusions. a diff tracks changes at points in time, not an editing session.
he could have easily yanked a goto line 'yy' for use elsewhere or by accident and pasted it by accident with 'p' right after he yanked it.
i see two very similar lines, 49 and 53, which were probably copy/pasted and edited. it is completely reasonable to assume he accidentally did it with another line further down the file.
he could have refactored this code multiple times before re-committing this file, moving blocks around or editing lines. there's plenty of opportunity to introduce errors.
do you understand how editing works?
if the entire diff was just + goto fail; that would be extremely suspicious, but that doesn't seem to be the case.
He removes only one line of two and ships.
And of course i always try to attribute those things to incompetent other than malice, but since in this case there was probably money and bribes involved so i tend to the later.
The cognitive process above is really sad: "I speculate this is what happened as a natural outcome of my other speculation"
This is why you use goto fail and not early returns.
Yes, there should have been code review, static analysis, and testing in place to prevent that. That'll probably start happening, and you can bet that there will be serious discussions internally about what happened.
If this was the action of a malicious government agency, then it was horribly hamfisted execution. It might offer plausible deniability in some sense, but it's hard to imagine a situation where such an action would be worthwhile, given the resources at the disposal of any such actor.
This needs more defense. I find it hard to imagine a situation where such a minimal and deniable action with such a massive effect wouldn't be worthwhile.
Because the goal of any such agency is not "let's render a minority of computing devices insecure for a short period under certain circumstances." That entire argument hinges on the idea that taking this action would have achieved some kind of desirable goal, when it simply would not have.
In fact, the whole thing is a massive distraction. There are myriad easier ways to conduct individual surveillance if that was the goal, and I would be much more concerned about the fact that these same agents already apparently have legal carte blanche to intercept traffic in other ways.
I think that your defense hinges on this being the only thing that they are doing. As one of many things, with no risk and little expended effort, why not?
>There are myriad easier ways to conduct individual surveillance if that was the goal
What's easier than getting a call from somebody placed at a tech company indicating that one of the most security critical pieces of code didn't have test coverage and wasn't statically analyzed before going into the build has a spot where an errant line could be inserted, in a way that could be argued later was accidental.
The total marginal cost to that hypothetical situation would have been to say 'ok.'
>these same agents already apparently have legal carte blanche to intercept traffic in other ways.
intercepting traffic != decoding traffic
I understand that Rails developers believe that all the TLS stacks must have dense rspec test coverage and zero-warnings static analysis passes in their builds, but that isn't the reality for any TLS stack. These aren't Rails apps (and, hate to break it to some of you, but your Rails test coverage isn't doing as much for your security as you'd like to think it is).
This is a nutball conspiracy theory, another example of the tech "community" eating its own rather than focusing on anything that would help mitigate state-sponsored surveillance. And it's happening solely in the service of an "exciting" narrative. The people promoting this bullshit conspiracy theory are, by and large, doing it because they want it to be true.
2. If you're a NSA boss, and some of your experts told you that you can break SSL in Apple products with adding one line that could be almost certainly attributed to inconspicuous human error, would you try to make a deal with Apple?
3. If you're an Apple boss, and NSA offers you cache (or other benefits, like competitor intelligence) for adding plausibly deniable bug in your code, would you turn it down?
There is no direct evidence for this case, sure. There is however ample evidence in the Snowden docs that this scenario happens too often for this to be called bullshit conspiracy theory.
Nobody doubts your ability to spin some coherent-sounding story about the TLS bug. It's not a hard game to play. People have been playing it for centuries. How about you try a more fun topic, like alien landings?
Down this path lies madness. Software has bugs.
Yes, it is quite possible that this is a simple bug. The other option is also quite possible :)
Consider that the NSA has already been known to burn stolen certificates for malware code signing. It's therefore not a stretch to assume they can easily MITM TLS without needing the help from bugs.
If they planted this bug, they would have been effectively democratizing TLS MITM to virtually everyone. This would help their adversaries more than it would help them, so I'm not convinced. It's easier for me to buy that the Chrome Pinkie Pie bugs were planted, due to the hardness of their discovery, than this could ever be.
With other companies, I might find this plausible, but I do not believe Apple is very hard-up for cash or direction on which way the market is going. It seems to me that they have more to lose from a high-profile security breach (say, if this vulnerability had been used in a mass theft) than they do to gain from anything the NSA could offer them.
This needs a defense.
>There is no evidence to back up the idea that the bug was malicious.
This is completely unrelated to what I said needs a defense. It's like you weren't even responding to me.
edit: I don't know Rails.
If this was the action of a malicious government agency, you're the proof it was an awesome move, given your "Stop this nonsense".
Considering the possibility that any system managed from a compromised device could be compromised to any degree, how does the use of static code analysis and testing in the future uncompromise those systems?
Short of finding a unicorn and seeing who can ride it, there's no guarantee that a system isn't pwnd or connected to one that's pwnd. The certificate failure means binaries could have been pushed.
The problem is that you can't trust trust.
http://www.ece.cmu.edu/~ganger/712.fall02/papers/p761-thomps...
It doesn't. But since you were relying on a closed-source system in the first place, you are already screwed - if that's the level of security you require.
And that's the more difficult exploit to engineer. If Danny is using OSX for administration and we treat his installation as compromised we get a much richer stack for exploiting server A.
If you're wondering whether this particular code is a consequence of all that: probably not, but if it were it would prove anything that isn't already known.
And why don't we hear anything the guy who might have done this mistake IF IT WAS AN ACTUAL MISTAKE ?
I don't like conspiracy theories, but I'd be apple, I'd fire the guy for incompetence. I mean of all the code that has to be secure, you have to check this one.
This is why you always, always, ALWAYS put braces around your if()s.
(I'm not religious about what line the open brace goes on, but I will reject any code review that skips the braces entirely.)
I should go consult Code Complete again, actually...
Astyle -A1 -j -c -v asterisk.cpp asterisk.h (Sorry asterisks italicize on HN) http://astyle.sourceforge.net/
Another person would say, they saw a little bigfoot standing on the river bank and the narrator would say, "That's exactly the sort of place we would expect to see a baby bigfoot." [1]
If someone said they saw a bigfoot shopping in Walmart, we could expect to hear, "That's exactly the sort of a place a bigfoot would shop."
The conspiracy theorists and the honest mistake theorists are both finding bigfoot. On the one hand, it's exactly the sort of exploit a state security organ would like to create and on the other its exactly the sort of error a programmer can make in C. And neither side believes in the other's Bigfoot.
[1] We talked about the actual words that were used by bigfoot hunters to advance the story