Item — HN Reader

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.

1 comments

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

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.

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

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?