Live data from Hacker News

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

news.ycombinator.com

21–30 of 86 posts

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

#21
post #7

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

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.

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

#22

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.

Do you understand diff format?

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

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

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

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

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

#25
post #18

Earlier quoted context omitted.

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.

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

Yes, something that would test all the steps of the SSL authentication and possible failures

Maybe the people from OpenSSL have some test like that already

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

#26
post #21
post #7

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

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.

How about verifying a bad cert?

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

#27
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…

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.

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

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

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

#29

Earlier quoted context omitted.

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.

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

Tell me about it

And you think you know what the device answers and understands, but there's usually some gotchas.

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

#30

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…

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

Post reply on HN