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

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

#51

Post by bakhy » Fri, Feb 28, 2014, 4:36 PM UTC

mistercow wrote: post #6

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.

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.

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

#52

Post by beachstartup » Fri, Feb 28, 2014, 4:37 PM UTC

stcredzero wrote: post #48

beachstartup wrote: post #36

Earlier quoted context omitted.

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…

It's just that the code had been around for years by that point, and given that, that's not the story the diff tells.

that's true but there are other, material changes in the file including conditional logic very close to the bug - to me that says there was at least some additional editing going on, giving opportunity to introduce an error.

if the entire diff was just + goto fail; that would be extremely suspicious, but that doesn't seem to be the case.

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

#53

Post by bakhy » Fri, Feb 28, 2014, 4:40 PM UTC

chrismcb wrote: post #49

wmgries wrote: post #45

Earlier quoted context omitted.

Also, you almost never need to use goto.

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.

All three of those patterns are something which would lead me to consider is there a more functional-style option available to rewrite the method in question. I'll take a gigantic nested if over a split-end method (multiple exit points) any day of the week ;) But of course, such things cannot be avoided completely, nor will avoiding them magically eliminate all bugs in the universe..

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

#54

Post by brudgers » Fri, Feb 28, 2014, 4:41 PM UTC

matthewmacleod wrote: post #24

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 i…

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

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

#55

Post by shawabawa3 » Fri, Feb 28, 2014, 4:41 PM UTC

lyngvi wrote: post #16

cjbprime wrote: post #2

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 plausi…

++ 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 lik…

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

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

#56

Post by matthewmacleod » Fri, Feb 28, 2014, 4:57 PM UTC

brudgers wrote: post #54

matthewmacleod wrote: post #24

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 i…

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 o…

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 system

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.

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

#57

Post by brudgers » Fri, Feb 28, 2014, 4:58 PM UTC

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

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

#58

Post by tptacek » Fri, Feb 28, 2014, 4:58 PM UTC

matthewmacleod wrote: post #56

brudgers wrote: post #54

Earlier quoted context omitted.

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 o…

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

It wasn't a closed-source system. This bug was in public source code, sitting there for anyone to see it. Nobody did.

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

#59

Post by matthewmacleod » Fri, Feb 28, 2014, 5:03 PM UTC

tptacek wrote: post #58

matthewmacleod wrote: post #56

Earlier quoted context omitted.

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

It wasn't a closed-source system. This bug was in public source code, sitting there for anyone to see it. Nobody did.

I'm aware that this particular code was - but presumably whichever hypothetical person has arbitrary binaries pushed to them in the grandparent's scenario was using a Mac or iOS.

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

#60

Post by miopa » Fri, Feb 28, 2014, 5:09 PM UTC

tptacek wrote: post #41

pessimizer wrote: post #30

Earlier quoted context omitted.

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

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 a…

1. It is certain that NSA has some very smart people playing with the sources of the Apple crypto-modules.

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.