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.
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.
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.
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;
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.
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.
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:
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.
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.
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.
> 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.
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.
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.
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.
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.
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.
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:
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.
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.
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).