Hacker News Reader

Comment by pencilo | Hacker News Reader

Take a look at http://opensource.apple.com/source/Security/Security-55471/l...

specifically check the function SSLVerifySignedServerKeyExchange

I leave the joy of spotting it to you. It is obvious and if you know c you'll see it(You don't need any knowledge of crypto).

View on HN

Here's a diff of that file from OS X 10.8.5 (Security-55179.13) to 10.9 (Security-55471): https://gist.github.com/alexyakoubian/9151610/revisions

Check line 631. Appears seemingly out of nowhere.

Good find!

But, are you sure the 10.8.5 (-55179.13) version isn't in some sense a later, patched maintenance branch compared to a 10.9 (-55471) that might have been frozen earlier? The release dates are very close (~2013-10-03 for 10.8.5; ~2013-10-22 for 10.9), and they might have already been separate branches.

(Is there a 10.8.4 version to compare?)

Good point. I just checked that file ever since it was open-sourced in 10.8 and it stays exactly the same throughout all of 10.8.x (10.8 - 10.8.5) and only changes in 10.9. (You can check for yourself here: http://opensource.apple.com/)

It doesn't seem like what you said is the case here but obviously we're still missing changesets that may have been committed between 10.8.5 (Security-55179.13) and 10.9 (Security-55471). It'd be really interesting to do a git-blame on that file.

EDIT: Nevermind, that file wasn't open-sourced in 10.8. It's actually really old. (Look for directories starting with libsecurity_ssl in pre-10.8 OS X versions.) Didn't find anything particularly interesting in the old versions though.

Thanks for clarifying. I know silly errors like this can slip in, but I hope Apple does a deep x-ray on all circumstances surrounding the change.

This bug of an extra duplicate line looks like a merge issue to me.

Goto considered harmful, indeed!

Not much a problem of `goto` per se. Rather a problem with if conditions used without code blocks.

Others might say it's a problem of whitespace insensitive languages ;)

Haha love the whitespace comment. This also makes you think of the static source-code analysis done at Apple. Surely static tools would have picked this up, no...?

Clang currently does not warn about this, but I'd wager that Xcode will boast a feature in the next version that detects dead code due to early returns/gotos.

Clang may not warn about white space issues, but it certainly does warn about unreachable code if -Wunreachable is on. This code would not compile in my project because the unconditional goto leaves the following code unreachable.

Even if there were braces, the bug would still exist if the extra goto was outside the braces.

It might be more noticeable, but then, the original bug existed because no one noticed.

From a different point of view, part of the problem is the undefined state of the code at the "fail" label.

Execution will arrive there somehow, but the 'how' is unclear. The word "fail" implies you should reach that point only if there was an error, but that is a bad assumption in this case.

If the real answer to 'how did we get here?' was checked, then the bug could not hide in the undefined behavior. This would not allow a dangling goto to result in a false positive. A false negative will get someone's attention when their web page doesn't load.

Something like this could remove the undefined state:

      goto pass;
  fail:
    if ( err == 0 ) {
    	assert( err != 0 ); // BUG! variable must contain an error number
    	err = kSomeAppropriateErrorNumber;
    }
  pass:
    SSLFreeBuffer(&signedHashes);
    SSLFreeBuffer(&hashCtx);
    return err;

    return ERRCODE;

Would have produced the exact same bug.

So, what does that mean, the goto fail portion of the code. It seems like it will go to that no matter what. What is the end outcome?

Thanks in advanced

At that point the variable 'err' is still zero (noErr). So the function returns noErr, the caller thinks everything is good, and the communication is allowed.

People bitch about indentation in Python. But in this case it would have prevented the bug!

Of course Python doesn't even have a goto, so the code would have needed to be structured differently to start with.

If I were a betting man I'd wager that this bug wasn't really an accident. It would be really interesting to check commit logs to see the history.

> People bitch about indentation in Python

Really? I did something in Python for the first time a while ago and the indentation as code block is something I find very elegant. I can't fathom why would people find something wrong with this.

it's relatively harder to refactor code - you can't just select between { and }. otherwise, no idea - python is my weapon of choice, so i may be biased.

Can't really write onliners. And use of otherwise great interactive shell is not as good (not that bracketed blocks would help).

Style guideline of using curly braces with ALL if statements, even one liners, would have gone a long way to prevent this.

Code reviews and comprehensive test suites for critical code would have prevented this and many other kinds of mistakes besides.

Here's a little bit of a conspiracy theory for you. There's an old saying: "there are no coincidences on Wall Street". Not with the amounts of money that are involved.

I think something similar should apply to critical security code. This reminds me of when someone tried to add the following to the Linux kernel:

if ((options == (__WCLONE|__WALL)) && (current->uid = 0)) retval = -EINVAL;

Oops, is that "uid == 0", or is that "uid = 0"? Yet another "typo" that couldn't happen in Python.

How could this not happen in Python?

Because assigning in if would be a syntax error in Python.

Python does not allow an assignment to occur within an expression. They deliberately chose that restriction, to avoid that hard-to-see bug.

I have always wondered why all languages don't enforce this?

Because it is often really convenient, particularly with a simple macro preprocessor like cpp but in other circumstances too. I like gcc's approach of forcing you (if you have the right warning enabled) to put an extra pair of unnecessary parentheses around assignments in expressions.

because, for example in C, such a incompatible change would alienate most developers used to the very ideomatic

        if((val = function(args)) != expected)
            return val;

True, but in languages with operator overloading you (may) end up with other issues. Unless you look at the source you have no idea what the '==' operator will do.

A better solution is gcc's which forces you (if you enable the restriction) to put an extra pair of parens around inline assignments.

The parenthesis in this attack are sufficient to prevent that warning.

Good question, even though Python doesn't allow assignment in an expression, but for this reason I put the constant on the left side of the comparison no matter what language I use.

Don't C compilers warn about code that is never executed?

I'm actually pretty impressed how this bug was not caught before. The compiler warning is just one thing that came to my mind but test coverage should have been the strongest hint. A linter could have also make the thing easier for a code review. Probably these parts of the code should have stricter coding guideline.

You are 100% right. LLVM should fire a dead code warning here. However later code is used as a jump target for goto so it's quite hard for the compiler to infer this (or is it because its a different scope?).

Either way, definitely avoidable.

Looks typical of a merge cock up to me as the indentation is preserved suggesting it's a duplicate line or there was an if statement on the previous line that was removed.

The basic blocks from just after the unconditional goto up to the "fail:" label are all unreachable, so a dead code warning could be issued.

> However later code is used as a jump target for goto so it's quite hard for the compiler to infer this

Nope, because a jump target always starts a new basic block.

Looks to me like Apple aren't running Code-Coverage tests on their OS. That's kind of very scary.

> Don't C compilers warn about code that is never executed?

Not by default. On both GCC and Clang this kind of error isn't caught using -Wall or even -Wextra - you have to explicitly opt-in using -Wunreachable-code. It'd really be nice if that changed and Clang basically had close to -Weverything on by default and required you to opt out, preferably with something like `#pragma ALLOW_SHODDY_<category>` per file to put some pressure on C programmers not to just continue ignoring errors rather than fixing them.

In the latest GCC releases, -Wunreachable-code has been removed (the option still exists but is silently ignored for compat with existing Makefiles), as its output varied so much between releases (depending on what got optimized away).

I'd hope after this someone brings it back in some form – it's an incredibly useful tool and a diversity of implementations would be great for security-critical code.

I agree that they should. This is one of the types of errors that lint was traditionally used to find.

(Spoiler) This is another reason why you should never use if statements without curly braces.

Thanks for the hint. It was amusing to spot the actual bug.

I partially agree. Curly braces won't fix a typo or a rushed code review.

Does anyone else look at this file and suddenly have a greater appreciation that their company uses a cumbersome, time-consuming code review process which enforces an annoying, rigid code style?

We've gone to doing pull requests from branches with our code within our own repo because many of us work remotely and we want the eyeballs. Your teammates have to approve the code and merge it and close your branch, you can't do it yourself.

I thought I would hate this model but I absolutely love it. It's the next best thing to pair programming and somewhat less exhausting.

That's hilarious. Took me a while as I was looking for something more subtle. Thanks for the clues.

Another clue: indentation fail.

Conspiracy theories aside, every programmer has probably made that mistake. The better ones learn to just avoid if statements without blocks ;)

But one thing that pisses me off is that they go and implement SSL and don't have any automated tests for it!!

For a company of Apple's size, that can only be called grossly negligent. There is no excuse. And they probably don't have any automated testing for most of their other code either.

What I would expect is that SSL code is tested with all the different of ways of spoofing a certificate. And that in an automated manner, on every build.

Of all the comments, I think yours is most to the point: we cannot rely on developers not making mistakes, but some kind of processes (tools, QA testing) should have caught an error like this one.

My sentiments exactly. For them to have a piece of code that returns OK/notOK, in SSL of all things, and not have tests for both branches is inexcusable. Not only is this code untested, obviously, but the calling code (which should have two branches based on the result of this call) must be untested too. Bleagh!

It's not a mistake, though. The second goto line was added separately.

I could see if they were copying & pasting goto statements all over the place, and happened to paste it twice by accident. But that's how it occurred. The second one was added in a separate commit.

For the likely error I spot, you don't even need To know C. Anything remotely algol-like Will do.

I wonder whether the software from the guys at viva64.com would spot that.

You don't need fancy software, compilers warn about this kind of thing.

Just eyeballs and a clue stick!

However I can't see a valid case for this pattern so static analyser rule to knock it on the head would be a good idea.

Oops. I was doing this from an iPad, and iOS Safari is extremely bad at rendering source code (it's plain text, so it wraps it. It also chooses an incredibly large font size, so lines wrap at 50 characters or so in portrait mode. That made me bail out early, after reading this and not spotting the SRVR = SERVER abbreviation:

    clientRandom.data = ctx->clientRandom;
    clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
    serverRandom.data = ctx->serverRandom;
    serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;

That is something that I think static analysis tools could signal. It would be a red herring, though.

Was there a more fundamental problem in the assumptions of the code?

The state of the operation is "success" (err==zero) until some step changes the state to "failure" (non-zero).

The state should be assumed to be "failure" until the last step changes it to "success".

Whoops. goto fail indeed

Just from looking the code it seems pretty sloppy, not what I would expect from security sensitive code.

Mixed tabs and spaces, inconsistent indentation, two empty lines in a row, sometimes "if (...)" and sometimes "if(...)".

I wonder about the code style, too, but on a different level. You don't need curly braces (surely you should always use them) or whitespace sensitivity or source code indentation beautification if you use the appropriate programming technique for this situation. Such endless error checking followed by releasing of resources at the end is a case for try / catch / finally. I wonder when people start using decent languages for important coding.

CloudFlare just opensourced "Red October", which implements the two-man rule for any cryptographic checkout, but it could be really good for code checkins to catch things like this.

http://blog.cloudflare.com/red-october-cloudflares-open-sour...

Damn what kind of half-assed developers do they have at Apple these days? They broke 2 of my (admittedly many) cardinal rules of C development: 1 - There is never ever a valid reason for using goto. It should not be part of the language. 2 - Always enclose script blocks in {} even if it's only a single line.

What about these?

  - don't leave local variables uninitialized.
  - don't try to hand-optimize lines unnecessarily. 
  - don't mix assignments into boolean expressions. (Which is really an unneeded optimization.)

This does not look like a merge error. The added line stands alone and it's very well located for an exploit.

Reply on news.ycombinator.com