I installed this last night and the update came in at 15MB. Surely a one line bug shouldn't cause a 15MB update? Or is it that some things in iOS might be statically linked and those had to be pushed out as well?
Apple's SSL/TLS bug
61–70 of 295 posts
Re: Apple's SSL/TLS bug
#62Worth noting that static analysis finds bugs like this immediately. TLS code seems like the perfect candidate to run through static analysis on every checkin. There are products such as Coverity and PVS-Studio that would have immediately flagged this and probably some open-source ones built around LLVM as well (unsure about this one, though). I personally use Coverity and have it hooked up in the same way everyone co…
Regarding Adam Langley's comment about -Wall in gcc: -Wextra is what you want to have many more tests analysing your code. I wonder if this would have caught it.
A quick test shows neither -Wall nor -Wextra report anything on either gcc or clang. However, clang's -Weverything does complain (it implies -Wunreachable-code).
Confusingly, gcc accepts -Wunreachable-code as a valid option, but then proceeds not to warn anything. (edit): Which is a known bug apparently,
Re: Apple's SSL/TLS bug
#63Dijkstra always contended GOTOs were harmful! In all seriousness it's unfortunate the C family of languages even allow this kind of bug - we've all been bitten by it at one time or another.
I'm calling you on the GOTO is evil thing. It's not. This is a well-structured use of GOTO, and it has a long tradition in C shops. This is a well-understood idiom, and you're seeing a different failure.
Your choices for exception handling systems in the C-like languages are:
- early return (whereupon, resource leaks and bloated cleanup code)
- setjmp/longjmp (whereupon, utter chaos)
- "chaining" success, where you don't use GOTO and have to consistently check your current 'err' state before doing additional "work" (whereupon, lots of bugs when you forget to handle an error case)
- or GOTO to a single piece of error handling code (this is usually coupled with an earlier hunk of code where you initialize variables and set up state; this is essentially the C++ "constructor / destructor" idiom, but done by hand).
The GOTO is the best choice, because everything else is worse.
(If you're using C++ exceptions you're in serious trouble. The effort required to write correct code in the presence of arbitrary exceptions is very high, and you're likely to get things wrong. Scott Meyers wrote like three whole books on the subject, which should be strong evidence that something is Very Wrong, and if you've ever used exceptions in a language that doesn't have garbage collection you'll probably agree (and even GC doesn't save you). Most production C++ code that I've seen use exceptions simply catches them in a bug-handling layer, which responds by scribbling some kind of report and then restarting the app).
Often these GOTOs are wrapped with macros that hide the fact that GOTO is being used. This can hurt code quality, since things are less clear, but in general they work well.
Basically they should have had the dead code warnings enabled, and listened to them, and done better checkin reviews. GOTO is not the enemy here. In paranoid code like this, generally you want to "claim success" as late as possible, so setting 'err' to some failure code would have been a better choice.
But using GOTO wasn't a mistake.
Re: Apple's SSL/TLS bug
#64The faulty block of code -- line 623 (function SSLVerifySignedServerKeyExchange) -- looks like a cut and paste (or the reverse) of the block of code at line 372 (function SSLSignServerKeyExchange), except this one doesn't have the extra "goto fail;".
Re: Apple's SSL/TLS bug
#65Earlier quoted context omitted.
Safari/iOS 4.3.3: not vulnerable Safari/i0S 7.0.4: VULNERABLE Chrome/iOS 7.0.4: not vulnerable Looks like using Chrome instead of Safari may help; I'd say it would be more interesting if standard mail client can be fooled.
I would be shocked if this doesn't apply to the email client as well.
I noticed a while ago that while Safari supports TLS 1.2, but Mail does not. So somewhere in the implementation they are using different code. (Not agreeing or disagreeing, just mentioning an observation.)
Re: Apple's SSL/TLS bug
#66I just made this - it'll tell you if you're vulnerable. https://gotofail.com/ Not very well tested, please let me know if it works for you. If you're on OS X Mavericks or on iOS 7 and haven't patched you should get big scary red text. Edit: posted here https://news.ycombinator.com/item?id=7282164
Your checker doesn't work well with curl, btw -- you end up seeing both the not vulnerable AND the vulnerable (alt text) messages.
There's not a whole lot I can do about that without adding a lot of complexity. You could try downloading https://gotofail.com:1266/test.png I suppose.
Re: Apple's SSL/TLS bug
#67Re: Apple's SSL/TLS bug
#68I think what this really shows is that Apple has no unit tests to cover the security of their products. And that is pretty scary.
Re: Apple's SSL/TLS bug
#69Earlier quoted context omitted.
and goto statements everywhere :S
the gotos actually make sense in this case. unless you'd prefer some insane tree of if/else?
Couldn't you just set a 'failed' boolean and wrap the fail: statements in a conditional check on it? Not that I'm saying all uses of goto, particularly this one, are necessarily absolute evil.
Re: Apple's SSL/TLS bug
#70Earlier quoted context omitted.
Regarding Adam Langley's comment about -Wall in gcc: -Wextra is what you want to have many more tests analysing your code. I wonder if this would have caught it.
A quick test shows neither -Wall nor -Wextra report anything on either gcc or clang. However, clang's -Weverything does complain (it implies -Wunreachable-code). Confusingly, gcc accepts -Wunreachable-code as a valid option, but then proceeds not to warn anything. (edit): Which is a known bug apparently, http://gcc.gnu.org/ml/gcc-help/2011-05/msg00360.html
My personal observation is that while compilers continually get better at identifying straightforward mistakes, they don't have the same capability as a static analysis tool that works across compilation units. That is the real selling point of these tools. Definitely -Weverything/-Wexta plus static analysis for baseline checking.
My other beef is that compilers always add new warnings as options or behind new "catch all" flags like -Weverything that no one knows about. As long as each new warning can be individually disabled, there isn't a huge cost to pay by making much more of them enabled by default. Upgrading to a new compiler version usually requires a tiny bit of work, so adding a few -Wno-* rules for new things you want to disable until the code is clean (or forever) is a small price to pay for all new code getting the checks by default.