
Why was "goto fail;" added without any other change to that part of the code? - 0x006A
Looking at the diff between the two versions of sslKeyExchange.c released by Apple http:&#x2F;&#x2F;opensource.apple.com&#x2F;source&#x2F;Security&#x2F;Security-55471&#x2F;libsecurity_ssl&#x2F;lib&#x2F;sslKeyExchange.c and http:&#x2F;&#x2F;opensource.apple.com&#x2F;source&#x2F;Security&#x2F;Security-55179.13&#x2F;libsecurity_ssl&#x2F;lib&#x2F;sslKeyExchange.c
I was trying to come up with a reasonable explanations of how this could have happened, but failed.<p>Here the relevant part of the diff:<p><pre><code>  @@ -627,6 +628,7 @@
           goto fail;
       if ((err = SSLHashSHA1.update(&amp;hashCtx, &amp;signedParams)) != 0)
           goto fail;
  +        goto fail;
       if ((err = SSLHashSHA1.final(&amp;hashCtx, &amp;hashOut)) != 0)
           goto fail;
</code></pre>
How could this ever happen? It does not look like a copy &amp; 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?
======
matthewmacleod
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.

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

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

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

~~~
patio11
Remember how Diaspora was supposed to be the private peer-to-peer encrypted
Facebook but peers private keys could be read or overwritten by anyone on the
Internet? Doesn't your argument suggest "Well we can't rule out the NSA having
a man on the inside trying to undermine their encryption"? Heck, why not go
all the way: the NSA funded Diaspora to bring Facebook to the negotiating
table?

Down this path lies madness. Software has bugs.

~~~
miopa
Yes, we can't rule that. But we could clearly see that the Diaspora codebase
screamed incompetence. Apple, on the other hand, has some very high quality
products and decades of experience.

Yes, it is quite possible that this is a simple bug. The other option is also
quite possible :)

~~~
tptacek
It is equally possible that Apple is a giant conspiracy dedicated to
concealing the grey aliens who actually control human civilization and are
harvesting our brainwaves, which actually function as the raw compute for a
giant intergalactic payments call center application, via Flappy Bird. Think
about it.

~~~
luke-stanley
That's not equally possible, due to Occam's razor:
[http://en.wikipedia.org/wiki/Occam's_razor](http://en.wikipedia.org/wiki/Occam's_razor)
Plus, the security services have a proven ability and intent. Be serious now.

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

~~~
lyngvi
++ 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
> 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.

~~~
lyngvi
D'oh. That's what I get for not paying attention and looking at the
indentation instead of for braces. Still, my bet's on mismerge first - though
it'd be worth reviewing that committer's other changes.

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

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

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

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

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

------
sslfailio
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;

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

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

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

~~~
ryandrake
If the code is supposed to be dead, DELETE it. Do not comment it out.

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.

~~~
dllthomas
I don't agree. Or I do to a point, but it's a relatively weak point. I agree
code should be yanked if there's no reason to be keeping it around. I don't
think there's never reason to be keeping it around. Sending people to figure
out which version of the repository had the relevant piece of code (which is
now correspondingly less likely to have been updated at all when surrounding
code changed) doesn't make sense if you genuinely expect it will be relevant
again soon.

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

------
DanHulton
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
Also, you almost never need to use goto.

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

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

~~~
DanHulton
Hm, and I'm of the opposite opinion. I like methods that fail early. Gigantic
if-chains are hella-hard to parse.

I should go consult Code Complete again, actually...

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

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

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

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

~~~
wreegab
> 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"

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

------
cgore
I appreciate the coding standard at a previous employer
([http://www.astronautics.com/](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.

------
beachstartup
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
That didn't happen - look at the diff in the OP.

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

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

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

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

------
danielweber
Sometimes people make mistakes.

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

