One of the most valuable companies in the world and they can't hire a junior level engineer to write 100% code coverage unit test? Unbelievable.
Apple's SSL/TLS bug
201–210 of 295 posts
Re: Apple's SSL/TLS bug
#202Earlier quoted context omitted.
Not possible in C, and I'm not even certain that high-level exceptions are desirable in a language like C. But I wonder if there's still room to tighten up the code. Perhaps something like if ( (err = SSLHashSHA1.update(&hashCtx, &serverRandom) != 0) || (err = SSLHashSHA1.update(&hashCtx, &signedParams) != 0) || (err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)) { goto fail; }
I understand that there are sequence points at those || divisions, and I appreciate that you've been careful with your layout. Even so, my spider sense is tingling horribly at having not just one assign-and-test in the condition for an if statement but a whole series of them that each reassign to the same variable. If there were a language feature available to do this more elegantly, whether exceptions or something e…
Should the variable reuse matter? Is there a case where the compiler won't short-circuit the || statement on the first failure?
If not, `|| (err = check())` is equivalent to separate checks which also also `goto fail` immediately.
Re: Apple's SSL/TLS bug
#203I 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?
The dynamic libraries in iOS aren't shipped as separate dylibs on the device. Rather, they're essentially concatenated together as part of a single file called the dyld shared cache that also lets Apple do some prebinding tricks, interning of objc selector names across all system libraries, etc.
The system-wide iOS build scripts actually randomize the order of the libraries in the shared cache by default (you can check this using dyld_shared_cache_util against the shared cache file, which you can compile from the dyld project on Apple's open source site). Since the ordering of dylibs in this giant shared cache file varies between builds, you could easily end up with a 15 MB diff even though all you've done is deleted a single goto in source code.
Re: Apple's SSL/TLS bug
#204One of the most valuable companies in the world and they can't hire a junior level engineer to write 100% code coverage unit test? Unbelievable.
Or an intern to rewrite the entire thing from scratch. The code quality of the entire library is awful. No comments. Impossible to determine ownership of pointers. MIXED TABS AND SPACES?? WTF?
People are downvoting my comments all over this thread but really, if this is not literally the worst code in iOS then I'm throwing away my iPad immediately. The priesthood of cryptography is always warning the lay programmer to avoid re-implementing crypto, and instead to use libraries written by experts. But the crypto experts are apparently good at math and terrible at programming computers. This Apple library certainly isn't the only evidence of that. OpenSSL is largely uncommented and untested as well.
Re: Apple's SSL/TLS bug
#205It's interesting watching all the speculation about "was it a backdoor, or just a bug?" Lots of points in favor or against: 1) It's a huge compromise, and "open" to anyone to exploit, which would ultimately get caught and fixed faster. But it's also not targeting anything specific, so there's less of a signature of the attacker. 2) Incredibly simple, and thus a plausible mistake. 3) Hidden in plain sight I'd generall…
It was likely someone subverting the process for reasons of urgency, for good intentions or malice. Simple static analysis or unit tests would have caught this.
Re: Apple's SSL/TLS bug
#206This could not have happened with C++ exceptions and RAII instead of manual error checking and goto for cleanup ;)
I don't know about exceptions (they are forbidden where I work) but any amount of C++ would significantly improve this implementation. There is code all over this library that allocates and deletes buffers in structs passed in as function arguments, i.e. other code's structs that nobody should be dicking around with except through constructors, destructors, and accessors.
This code would also be about half as long in straight-forward C++.
Re: Apple's SSL/TLS bug
#207Something that concerned me was things like software update daemons connecting with SSL. Can they be compromised? Or is this something that requires connecting to a black-hat server?
Re: Apple's SSL/TLS bug
#208Earlier quoted context omitted.
Likely the compiler will have optimized away the rest of the function as dead-code, so it's not as simple as just getting rid of the unconditional jump.
Ah, right. Good point.
If the code to Security.framework is complete and builds, we might not need to patch the binary directly.
Re: Apple's SSL/TLS bug
#209From looking at the code it seems like there should be a way to change the instructions to "fix" the issue, but in a round-about way. This code: if (sslVersionIsLikeTls12(ctx)) { /* Parse the algorithm field added in TLS1.2 */ if((charPtr + 2) > endCp) { sslErrorLog("signedServerKeyExchange: msg len error 499\n"); return errSSLProtocol; } sigAlg.hash = *charPtr++; sigAlg.signature = *charPtr++; } is only executed in…
If Security.framework is fully buildable from their source, perhaps the optimized out code isn't as much of a problem.
Re: Apple's SSL/TLS bug
#210I loaded up the Xcode project for Security.framework and it complained about disabled compiler warnings.
I also noticed their open-source download page [1] doesn't force you to https by default, which probably matters to anyone looking at Security.framework to fix SSL MITM problems :)
Doesn't look like they make their frameworks easy to compile for outsiders. The only corecrypto headers I can find are in XNU, and they appear to be outdated.
I'm hand-constructing what I can from context. Here are the defines from ccasn1.h: https://bochs.info/p/6jquf
You need these too:
typedef int ccoid_t;
extern size_t ccder_sizeof_raw_octet_string(signed long length);