> 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.
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.
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?
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
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.)
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?
Did you look at the bug? I'll quote it here:
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.