5 ms·
> 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 s
by terminus 13y ago
> 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.
- zorked 13y agoNo. 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.
- terminus 13y ago> 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.
- pixl97 13y agoInteresting, 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?
- stormbrew 13y agoIn 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.
- oneeyedpigeon 13y agoDamn 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?
- nickls 13y agoDoesn'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 http://www.diffnow.com/?report=ob51k Diff35
- pmjordan 13y agoWe 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.
- wreegab 13y agoExcept 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 https://news.ycombinator.com/item?id=7286582
- temujin 13y agoI 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!
- rsfinn 13y agoWhich 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.)
- larubbio 13y agoAgreed. That and blank lines between the if blocks to visually separate the code blocks as well.