Why was "goto fail;" added without any other change to that part of the code?

https://news.ycombinator.com/item?id=7318652

by 0x006A • 12 years ago

61 85 12 years ago

news.ycombinator.com

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;

How could this ever happen? It does not look like a copy & paste error as suggested in other places, it does not look like refactoring. Was it added intentionally to test something and commited by accident? Is there any possible non malicious explanation someone could come up with?

cjbprime2• 12 years ago

It's not true that there was no other change in the code -- there was a change eight lines earlier. It could be a mismerge of the latest upstream code. There was refactoring nearby, and people can make stupid mistakes while refactoring.

I 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.)

lyngvi2• 12 years ago

++ on mismerge. The change likely won't impact the output binary - compiler should issue a warning, which devs might ignore; then the optimizer would remove it as unreachable.

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.

shawabawa3• 12 years ago

The change likely won't impact the output binary - compiler should issue a warning, which devs might ignore; then the optimizer would remove it as unreachable.

It's not unreachable - it always executes if the if condition is false.

0x006A2• 12 years ago

yes the code above looks like a search and replace of ReadyHash(&SSLHashSHA1, &hashCtx, ctx)) with ReadyHash(&SSLHashSHA1, &hashCtx, ctx)) but while doing that its unlikely you touch the goto line.

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.

judk• 12 years ago

Your search and replace after copy and paste seems to have failed.

0x006A2• 12 years ago

mismerge looks like a possible explanation, do you know what the upstream project/codebase for this is? Would be interesting to look at that.

sentenza• 12 years ago

Think of change, copy, paste, do something else that you suddenly thought of, forget because lunch, commit without looking. (TODO: Write unit test.)

Or a conspiracy. At this poing, I just don't know any more.

danielweber• 12 years ago

Sometimes people make mistakes.

chris_wot• 12 years ago

Don't know why you got voted down. You make a very decent point!

rdtsc• 12 years ago

Common just say, we all know you what you are thinking. Bruce Schneier said it. You should know that we know what you are thinking -- yes there is a very high chance it was put there deliberately.

mistercow2• 12 years ago

A lot of code editors (Xcode doesn't by default, but you can easily configure it to) have a keyboard shortcut for duplicating the current line. I have plenty of times accidentally made changes at the wrong place in a file because I was mistaken about where the cursor was.

The lesson is to always make sure to look at your diffs before you commit your changes.

Cthulhu_• 12 years ago

And to have someone else look at your diffs and sign off on them before it's merged into master.

Google / Chrome does this, and so should everyone else, even more so if they're dealing with security.

mistercow2• 12 years ago

That's a good measure too, although I think with a mistake like this, the person writing the code might be more likely to spot the mistake than someone else.

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.

bakhy2• 12 years ago

Strongly agree with the lesson. I got burned once, produced a security hole by merging a messy config file. The problem was quickly detected, but I have not checked in a single commit after that without first going through all the diffs. Plus, I sometimes notice possible improvements during such a review. It's very helpful.

homakov2• 12 years ago

More importantly, why Apple doesn't run test suites, basic checks? Test driven development must be everywhere

raverbashing3• 12 years ago

Because TDD in low level languages is a pain in the behind.

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.

nailer• 12 years ago

Ack re: TDD, but integration tests with a bad certificate would have picked this up.

homakov2• 12 years ago

Not testing everywhere, but "smoking test" that your SSL implementation protects from MITM, at all, and none of your programmers screwed it (with gotofail or anything else).

This is what I expect from an OS developers

stcredzero4• 12 years ago

And sometimes impossible

I've been in that position. Sometimes with embedded, you can't have software fixtures, only hardware ones. (Bit banging)

danpalmer• 12 years ago

Test driven development is actually quite uncommon in many parts of the industry.

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.

laureny• 12 years ago

TDD might be useful for unit tests (even that I'm not convinced of) but not for integration tests.

The bug happens very deep inside the handshake protocol and it's extremely hard to test it automatically.

stcredzero4• 12 years ago

How about verifying a bad cert?

landypro• 12 years ago

That's hardly an excuse given the significance of this code.

dllthomas2• 12 years ago

Right, this is a much stronger case for static analysis and warning-free builds. This change caused dead code. If that code was supposed to be dead, comment it out.

sslfailio• 12 years ago

Yes people make mistakes.... that is why code in a file called sslKeyExchange.c ffs can be expected to be reviewed by another person at apple BEFORE the commit enters a release candidate. A double goto fail; is something that sticks out to anyone looking at that commit line by line. Seems like apple handles application security like any 6 dude valley startup.

My commit suggestion: - goto fail; + fail;

jfontan• 12 years ago

Maybe another check was done between those two goto fail but later was deleted because didn't make sense. The extra goto fail was left behind.

beachstartup4• 12 years ago

i do this all the time in vim. all it takes is one extra keystroke 'p' to do this.

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.

jeremyjh• 12 years ago

That didn't happen - look at the diff in the OP.

beachstartup4• 12 years ago

that's bullshit. you don't know that it didn't happen - there's no telling what he did with the code in the time between commits. he could moved things around 20 times, finally ending up in the same place, with the exception of the accidentally pasted a line.

plese use some imagination before jumping to conclusions. a diff tracks changes at points in time, not an editing session.

stcredzero4• 12 years ago

Do you understand diff format?

beachstartup4• 12 years ago

do YOU understand how diff works? it tracks changes between two points in time, it doesn't track how you got there or what you did in between.

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?

gcb02• 12 years ago

I'm 100% convinced it's malicious... But otherwise, it could be explained by the coder adding another check

   If(err==my_dev_string)
     goto fail;

Then before committing, sending the diff to a colleague who emails back "you forgot a debug there, besides that ship it"

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.

wreegab2• 12 years ago

but since in this case there was probably money and bribes involved

The cognitive process above is really sad: "I speculate this is what happened as a natural outcome of my other speculation"

gcb02• 12 years ago

my two speculations are really opposite (and most believable one to defend the coders innocence, if that was the case). i presented both and pointed out the one i have a gut feeling towards.

axylone• 12 years ago

Looks like they also introduced a reference leak on allocation failure:

    @@ -198,10 +198,8 @@
            sslDebugLog("SSLEncodeRSAKeyParams: modulus len=%ld, exponent len=%ld\n",
                    modulusLength, exponentLength);
         OSStatus err;
    -    if ((err = SSLAllocBuffer(keyParams, 
    -                       modulusLength + exponentLength + 4)) != 0) {
    -        CFReleaseSafe(exponent);
    -        CFReleaseSafe(modulus);
    +    if ((err = SSLAllocBuffer(keyParams,
    +                       modulusLength + exponentLength + 4, ctx)) != 0) {
             return err;
            }
         uint8_t *charPtr = keyParams->data;

Note the removed CFReleaseSafe(exponent) and modulus. All other return paths in SSLEncodeRSAKeyParams() call CFRelease(exponent) and modulus.

This is why you use goto fail and not early returns.

matthewmacleod4• 12 years ago

Stop this nonsense. There are plainly dozens of reasons that this could have happened accidentally. Literally every single developer has made similar stupid mistake, and literally every organisation has released stupidly broken code.

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.

pessimizer4• 12 years ago

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.

matthewmacleod4• 12 years ago

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.

tptacek5• 12 years ago

No, it doesn't need a defense. There is no evidence to back up the idea that the bug was malicious. People that have worked on large C codebases have seen crazier things than this. Routinely.

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.

wreegab2• 12 years ago

If this was the action of a malicious government agency, then it was horribly hamfisted execution

If this was the action of a malicious government agency, you're the proof it was an awesome move, given your "Stop this nonsense".

brudgers4• 12 years ago

How should a malicious actor have better crafted an exploit of equivalent scope and potency?

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...

matthewmacleod4• 12 years ago

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.

marvy• 12 years ago

Whenever someone brings up the "trusting trust" thingy, I feel compelled to point out the antidote: http://www.dwheeler.com/trusting-trust/

luke-stanley2• 12 years ago

That's what they expect you to think ;)

gatehouse• 12 years ago

If you are wondering whether there is a massive conspiracy to make software weaker and to hijack the work of everyone writing networked software and construct a massive surveillance apparatus and use it against enemies of Freedom™ and randomly chosen accidental targets: the answer is yes. It has been demonstrated beyond doubt.

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.

nitrogen• 12 years ago

This kind of mistake is why I always use curly braces with if() statements when I write C, and double-check the results through Vim's auto-indentation.

cgore• 12 years ago

I appreciate the coding standard at a previous employer (http://www.astronautics.com/) a little bit more now, that INSISTED that braces always happen, even for one line conditionals like these. It was kind of irritating at the time though.

donatj• 12 years ago

I had an accidental line duplication in git the otherday trying to do a by-line commit with a visual tool.

jokoon• 12 years ago

that's like telling programmers that Apple doesn't use a repo system, so there could not be any trace of this.

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.

DanHulton2• 12 years ago

Braces, y'all!

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.)

wmgries• 12 years ago

Also, you almost never need to use goto.

chrismcb• 12 years ago

This isn't goto's fault. A return, or a "finished = true;" or a ton of other code could have had the same result. Using a goto means it is easier to find it than some other similar mistakes.

Dewie• 12 years ago

Does no one use a pretty printer before committing code? Pretty printing would eliminate any doubt as to what code belongs to what if-statement and the like.

micro-ram• 12 years ago

I use Astyle to enforce both braces and indentation automatically. In VC Pre-Build Event add:

Astyle -A1 -j -c -v asterisk.cpp asterisk.h (Sorry asterisks italicize on HN) http://astyle.sourceforge.net/

brudgers4• 12 years ago

A few years ago, my son would watch "Finding Bigfoot.". Someone said they saw a bigfoot running off into the woods the narrator would say, "The bigfoot must have been frightened, so it's exactly what I would expect a bigfoot to do."

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

jheriko• 12 years ago

this looks exactly like a copy paste error - there are also plenty of shortcut keys for duplicating lines or transposing them...

Why was "goto fail;" added without any other change to that part of the code?

Related Stories