Hacker News Reader

Comment by blazespin | Hacker News Reader

Tptacek, it is scary because of exactly that -- it is so simple. How could such a secure platform built by such a massively large cap tech company do something so incredibly stupid in its most critical security stack.

If this trivial bug can happen and pushed into production .. Good lord, anything can happen.

The foundation of our mobile economy has just been proven to be on very very shaky legs.

View on HN

It would be nice if massive meant "more secure," but the real relationship is not between an organization's size and its security, rather it's about how much that organization tries to minimize, or displace, risk. Very large organizations tend to not specifically care that something is provably secure so much as they want contractor X to be liable if something bad happens because of it.

Some organizations--and even though it's easy to mock Apple here, I do believe they're one of them--sufficiently appreciate that improving security minimizes risk, and so spend a decent amount of money on it. You could argue they failed here, but even Adam Langley who essentially "is SSL" at Google admits he's not sure they have a test that simulates an attack on a possible similar implementation error in their own code (though he does argue that such an error would have been caught in Google's code review process.)

In any case, an attention to security is certainly not something that is borne out of an organization's revenue or number of employees.

I'm under the impression that Apple prides itself on having a smaller number of very dedicated employees that are perfectionists.

I can totally see this being "innocent 2AM mis-judgement call by a single employee", judging from what my peers tell me of Apple's corporate culture. I do think that Adam Langley's suggestion that code review would help is plausible, but it merely just means that more than one person has to make the same mistake in a judgement call. (It reduces the probability of such an error happening; it doesn't theoretically eliminate it.)

I can totally see this being "innocent 2AM mis-judgement call by a single employee", judging from what my peers tell me of Apple's corporate culture.

https://support.apple.com/library/APPLE/APPLECARE_ALLGEOS/HT...

We begin therefore where they are determined not to end, with the question whether any form of democratic self-government, anywhere, is consistent with the kind of massive, pervasive, surveillance into which the Unites Sta tes government has led not only us but the world.

This should not actually be a complicated inquiry.

http://snowdenandthefuture.info/events.html

FIPS 140-2 certification isn't remotely an indication of correctness of code, for better or worse.

Take, for example, the implementation Dual EC DRBG in the FIPS 140-2 certified OpenSSL module -- it was fatally flawed, and has never worked in practice. (It will be removed from the next version of the module in light of developments in the past year.)

https://lwn.net/Articles/578375/

> The foundation of our economy has just been proven to be on very very shaky legs.

Exactly. That sums up my feelings about it better than I could. Something so critical, something trivially easy to catch in a code review was not caught. And, the only scenario in which a code review might not have caught it: no code review. That's no code review for libssl.

If this is incompetence and not malice, it's incompetence of monumental proportions.

No. No. No.

Code reviews only mean your code is going through two sieves instead of one.

Of course it helps. But there is no guarantee of anything unless the reviewer is incapable of making mistakes, in which case you could just ask him to write the code in the first place.

> But there is no guarantee of anything unless the reviewer is incapable of making mistakes, in which case you could just ask him to write the code in the first place.

Did you look at the bug? I'll quote it here:

    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)  
        goto fail;
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)    
        goto fail;  
        goto fail;  
    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)  
        goto fail;

Even somebody with basic programming skills can see that's wrong. And, remember this is libssl. Any checkins to that warrant thoroughness if not paranoia.

In the CR tool I assume it may have looked more like this:

        if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)  
            goto fail;
        if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)    
            goto fail;
    -   if ((err = SSLHashSHA1.update(&hashCtx, &somethingElse)) != 0)
            goto fail;  
        if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)  
            goto fail;

Which is a little harder to see. Obviously you should always look at it in a side-by-side view (god I wish github would implement this) or at the resulting code, but people are imperfect.

Damn fine point. I really don't think we can conclude this is malice; do all of our own bugs always turn out to be really tricky to spot? Do we never make utterly ridiculous mistakes along these lines?

Doesn't look like any (major) modification to surrounding lines.

    if ((err = ReadyHash(&SSLHashSHA1, &hashCtx, ctx)) != 0)
        goto fail;

changes to:

    if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
        goto fail;

See: http://www.diffnow.com/?report=ob51k

Diff35

We only see the diff between released versions, not intermediate commits. For all we know, Apple developers use a single-pane diff tool, where such bugs are easy to miss.

Except that I don't think this was the original code considering it appears to be cut and paste of same code in same file. I commented on this: https://news.ycombinator.com/item?id=7286582

I seriously don't understand why so many people don't habitually use braces there...

  if (...) {
    goto fail;
  }
  if (...) {
    goto fail;
    goto fail;
  }
  if (...) {
    goto fail;
  }

tada, not actually a bug!

Which is fine, but in my experience flaws like this are often introduced via automated merges that could as easily have resulted in:

  if (...) {
    goto fail;
  }
  if (...) {
    goto fail;
  }
    goto fail;
  if (...) {
    goto fail;
  }

which produces the same bug. (Everyone's been shouting about "braces in single statement if clauses" as though they're an absolute fix; they're not, although they're a good idea in general. And yes, code review, better merge tools, yada yada.)

Agreed. That and blank lines between the if blocks to visually separate the code blocks as well.

Interesting, does this mean if you were using an updated SHA256 hash (as will be required soon for all EV certs) that the exploit would not have occurred?

> do something so incredibly stupid in its most critical security stack.

I disagree. We are human. We are not divine. We do stupid things and we can write stupid infinite loop or off-by-1 and the bug will live for a decade. I am sure you have written a program which can be easily exploited and you know that it is such a trivial bug.

Yes, the programmer is only human. But the vp of engineering should have had a budget and process that would not let this slip through.

Are you saying they should have extra people doing code review - requires two instead of 1 (I don't know the number, but assume that's the norm). Or more rigorous testing technique? I am all for it. Is this something they can easily discover through automated testing?

having extra people doing code review risks something that happens with physical products.

You take the experienced people off the shop floor, and call them inspectors. So now the quality of product coming off the shop floor is worse.

Ann is the first inspector in the chain. When she's busy (and remember, she will be because quality has dropped) she might be tempted to let a few things go because Bob, the second inspector in the chain, is bound to catch them. That's what he's there for.

Bob is the second inspector in the chain. Sometimes Ann really churns the product through. Luckily during those busy times he knows she's already inspected the stuff, so he only needs to give a 10% inspection.

So you have worse product with more errors and leaky inspection.

Ideally you'd have a system with skilled workers and self inspection. That's okay for aerospace (which pays well) but not so great for lower cost product.

Not quite sure how this transfers to coding.

Okay. Good point but I hope if people take the job seriously they will have to be very careful. Though that's ideal..

How about this:

owner/peer of the module has to sign off, and randomly select two more. One must be QA and one is another programmer who works on the module. We can also pick a "junior" level, but then that's probably not going to work since Apple employes programmers who have some years of experience already. Or we can pick someone who isn't directly working on that module, but have some qualification to do review. Mostly just asking "why are you having two goto, why return -100 here)

But I see counterargument: they will just listen to the author if he's senior or the owner who is also a senior. Their words carry weight.

That's assuming they are full time at inspection, another scenario is where one person does it in addition to their current responsibilities. Therefore the overall quantity of what's produced is reduced but the inspection levels are higher.

The vp of engineering should have invested in a good static analysis tool which would have spotted this section in about five seconds. Failing that turning on a sensible set of defaults in the compiler (warning about unreachable code) would have also drawn attention to it.

There are many many ways this should have been caught before it even left the building by numerous automated tools. Heck most modern IDEs will flag unreachable code in the editor so there is no real excuse for this. (Adding -Wunreachable-code to a sample project in Xcode immediately flags the next line).

The programmer is human which is why automated verification tools exist.

(AppCode's default inspections will also flag the issue providing you run them)

I'd be really really surprised they didn't run this through static analysis. I will, to be honest, then maybe people are right about "it's stupid they make such stupid mistake."

I am not familiar with running static analysis and looking output, only did very minimal undefined behavior santisizer detection.

I haven't been keeping up, but a year ago Google wasn't using any static analyzer at all on Chrome. They didn't use a real memory access checker like valgrind even to load a blank page, although they came up with a weaker memcheck that mostly works.

Lax development is par for the course at these mega corps.

I cannot comment on Chrome itself, but internal C++ development at Google has a pretty comprehensive array of checks, both static and dynamic.

We've been using valgrind bots for a long time on the Chromium project - certainly for much longer than a year.

For example, according to the Wayback Machine, our sites page had an entry about it since 2009:

http://web.archive.org/web/20091123132855/http://www.chromiu...

I was thinking of this:

http://www.reddit.com/r/programming/comments/mw2sn/googles_a...

This is a comment that seems to suggest that it's abnormal for dumb bugs to cause huge security problems. It is not. If the foundation of our economy is C software security, well, hate to break it to you, but...

What strikes me is that the bug looks like a typo but it implements a logic error and that logic error pretty much negates everything the library represents itself as doing and what the library represents itself as doing is providing a foundation upon which signals security relies.

Suppose a contractor was hired by the NSA to write an exploit with equivalent function. How could it be crafted more cleverly?

It is absolutely abnormal for companies like Apple to release core security bugs this shallow that could have been easily discovered by straightforward unit tests and static analysis tools.

This is why it's a big deal.

The other reason why this is no big deal (anymore) is that the Snowden leaks have shown that the NSA has total control over all iPhones.

Why should we even bother talking about bugs like this anymore? Pure distraction.

Which is worse: the NSA having total control over all iPhones (citation? I obviously haven't been paying enough attention), or the NSA and all the (other) bad guys in the world having total control ...? Sure, they're both terrible, but I'd take the former over the latter.

> the Snowden leaks have shown that the NSA has total control over all iPhones

You mean: had total control over all the original iPhones that they could get physical access to. (back when jailbreaking was extremely simple and common)

Where does the "physical access" part come from? And if that was the case, why would it be impossible now?

One of the sources: http://www.forbes.com/sites/erikkain/2013/12/30/the-nsa-repo...

From the slide on your very link: "The initial release of DROPOUTJEEP will focus on installing the implant via close access methods. A remote installation capability will be pursued in a future release."

Basically how it worked was they jailbroke your iPhone and installed spyware on it. Is it quite likely that today, 7 years later, they have a remote 0day to do the same? Absolutely. But there's no proof that "the NSA has total control over all iPhones".

Reply on news.ycombinator.com