Apple's SSL/TLS bug
imperialviolet.org
imperialviolet.org
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 generally come down on the side of "accident". The better question is if an systematic testing system for exploitable weaknesses (or a lucky chance finding) could find something like this (either a regression, or a badly implemented new feature) and exploit it -- basically assuming there are going to be errors in implementations. That's understood to be a standard technique in the IC, and a whole industry built around it... and then, the scale of compromise.
There are lots of mitigation techniques for vulnerabilities like this (essentially, devices which are too fast-changing and too difficult to fully trust, but which have to touch sensitive data), but it's not as if people can carry around a firewall in their pocket these days, sadly.
I'm certainly re-generating any keys which touched iOS or OSX devices (thankfully very few), and reinstalling OSes, in the interim.
This might explain DROPOUTJEEP particularly considering how much more efficient it is than breaking messages after they are encrypted.
http://mobile.eweek.com/security/nsa-spying-on-apple-iphones...
That and inadequate, bordering on zero, code review. Even a beginner C programmer looking at this code could see how fishy it looks.
No code review while checking in code to libssl. That takes a lot of incompetence.
"Any sufficiently advanced incompetence is indistinguishable from malice"
https://support.apple.com/library/APPLE/APPLECARE_ALLGEOS/HT...
We begin therefore where they are determined not to end, with the question whether any form of democratic self-government, anywhere, is consistent with the kind of massive, pervasive, surveillance into which the Unites States government has led not only us but the world.
This should not actually be a complicated inquiry.
And that fact that this bug and terrible coding style was in the publicly available source code for so long totally disproves ESR's "many eyes make all bugs shallow" myth. Thanks for the false sense of security, Eric. The chickens have come home to roost again.
If "many eyes make all bugs shallow" were true, somebody would have raised the flag "Hey everybody, these Bozos are actually omitting brackets in their SSL code! Somebody could carelessly insert a statement and accidentally (or maliciously) introduce a hard to see bug some day!"
Hardly anybody actually bothers to read code in the real world. So there aren't "many" eyes, and even if there were, many bugs aren't shallow even to expert eyes, and "all" bugs will never be shallow to most eyes.
That's why it's important to pay competent security professionals to actually take the time and effort to audit source code, which is difficult work that requires much time and effort that takes them away from other valuable, high paying, less tedious and mind numbing work.
But it's the size of the target base and its quality which makes it worth trying considering the way in which iPhones may be present in even a security savvy an individual's social context.
In many code editors, numerous fat fingered shortcuts could produce such a compilable line duplication/deletion (deletion because maybe that's not a goto line duplicated but a test line deleted).
I'm baffled that neither clang nor gcc spits a warning for the unreachable code.
Not trying to shift blames or anything, but rather than do some finger pointing, I'd rather see this as a warning for all of us to ramp up our game and better our tools so that everyone benefits, reducing the risk of both honest and intentional or covert[1] malfunctions.
[0]: example custom shortcut as ^d to duplicate (could have been cmd(+shift)+D, which sits right along cmd(+shift)+S): http://www.xinsight.ca/blog/xcode-trick-creating-a-shortcut-...
Isn't it the case anyways that SSL is not very efficient against state sponsored attacks as almost all of them can generate certificates for any domain?
All this speaks towards it being a unfortunate mistake not something malicious, unless there is something wrong with my understanding how things work :-)
The scary thing is the bar is so low; even I could turn this into a nice weaponized thing to go after the long tail of people who don't upgrade, for the next months. The "diode" etc. infrastructure NSA built isn't that different from botnet C&C or the kind of relays people have when hacking.
I wish more people understood this. Not to let NSA off the hook (what they are doing is awful), but the threat posed by the NSA is a higher-level down the road/slippery slope threat.
There are other more immediate and real dangers out there that are actively trying to steal whatever they can find.
I diff'ed both functions to try to understand the changes which may have been made.
The "goto fail;" is somewhat "drowned" into other changes around, so it stands out less using a diff tool because of these surrounding changes. The surrounding changes in question:
1) Moving a "SSLFreeBuffer(&hashCtx)" instruction at a higher position: Very strange since it accomplish absolutely nothing at its new position, as opposed to it's original position.
2) Renaming of two variables: "hash" became "hashOut" and "exchangeParams" became "signedParams".
3) Indentation of "goto"s
Without the above changes, the "goto fail;" stand out rather well when using a diff tool.
As a matter of fact, by deploying one of those the beginning of the macro, one in the middle, and one at the end, you can set up what's called a "trifor(;;);ce field", an anti-anti-pattern that is documented on the c2 wiki thusly:
The trifor(;;);ce has the power to grant the wish of whomever touches it, and molds the Sacred Realm to reflect that person's heart. The trifor(;;);ce does not discriminate between "good" or "evil". However, if a person without an equal balance of power, wisdom, and courage makes a wish, the trifor(;;);ce will split into its three separate parts: the piece that best personifies the wish-maker will be the only piece to remain in hand, whilst the other two will take residence in whosoever most personifies them. Reassembly is then required for such a person's wish to be granted, but does not exclude another from making the attempt.
No, I'm just joking and making shit up. Don't anyone ever do that! As bitchy and pedantic as gcc and clang are about other things, I'm disappointed they don't complain about that.
I don't believe I've been a victim, and most of the ways you'd exploit this would leave enough traces to be high-risk-of-detection, I think.
But, it's hard to prove a negative, so it's safer to act as if it had been compromised.
(I'm also updating keys anyway, so this is just a matter of waiting a day or two to do so.)
I'm baffled that this wasn't caught long, long ago. Most of us have worked with internal systems where certs, for whatever reason, don't match the site, and it's surprising many people haven't noted that such doesn't raise any errors on iOS/OSX.
Though thinking back....I actually remember encountering exactly that on the iPad once, surprised that it didn't raise a flag. Like probably most I just shrugged and continued on.
The issue is one level deeper. When you connect to a server that supports ephemeral key exchange, the parties involved will generate new key pairs on the fly to use for that connection. In order to make sure that the server has the key published in its certificate, the ephemeral public key shown to the client must be signed by the static private key of the certificate and the client MUST verify it. It is the ephemeral key signature that is not validated, not the signature on the certificate itself.
What happens here is that the server gives the client a _valid_ certificate for which it does not own the private key, and in the ephemeral step, it simply generates a key pair without a valid signature. At no point in the handshake the server is asked to verify its ownership of the private key associated to the certificate it is presenting
To summarize, TLS certificate should be valid, but you are never asked to verify you have its private key when the connection used DHE or ECDHE.
I'd like to point out the case where Debian maintainers "fixed" a "bug" discovered by static analysis; https://blog.isotoma.com/2008/05/debians-openssl-disaster/.
The problem are the many developers that still think they are perfect and know the full C standard, including undefined parts.
It takes a special kind of ego to write an SSL library with no unit tests, not turn on compiler warnings, and not use static analysis tools.
Static analysis, regression tests and turning on all the warnings you possibly can should be mandatory, especially for such critical pieces of code.
Many of the C guys I met along my career thought otherwise.
> Static analysis, regression tests and turning on all the warnings you possibly can should be mandatory, especially for such critical pieces of code.
+1
https://github.com/landonf/Testability-CVE-2014-1266/blob/ma...
Error -> Warning 527 Unreachable code at token 'ret' (col 12)
Confusingly, gcc accepts -Wunreachable-code as a valid option, but then proceeds not to warn anything. (edit): Which is a known bug apparently,
I've been wondering how hard it would be to get to the point where the defaults are rigorous and developers have to opt out with specific -Wno-… options.
We have it turned on, and about 10 warnings specifically disabled because they were too noisy or not useful for us. It's always interesting upgrading Xcode and seeing what new warnings we get to fix.
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.
Windows or ubuntu bootcamp until they fix this, I think.
> I think anyone relying on the security of OS X is going to have to seriously rethink their OS choice after this.
http://www.androidcentral.com/samsung-lock-screen-bypassed-e...
You might find this article a good read: http://www.theverge.com/2014/1/21/5307992/inside-the-mind-of...
It's mainly an SSH and RDP terminal anyway.
OSX quality and QA is extremely bad.
Tell that to the people who generated keys on Debian.
Programmers are simply not good enough at writing secure code. Full stop. If you say anything else, you're just flaunting your own unreliability as a source of security advice.
(For example, Github lets you push/pull via SSH as git@github.com, and they determine your identity by the SSH key used.)
You're saying Google Chrome doesn't test that an ephemeral key is actually signed by the cert's private key? If that's the case, that's completely unacceptable because it's the whole point of the protocol.
You're kidding, right?
http://www.debian.org/security/
Edit:
There were multiple security issues in OpenSSL as recently as January.
Security is not a Boolean. A better question is, which OS is more secure OOTB for a given user.
https://www.gov.uk/government/publications/end-user-devices-...
I think it'd break one of the binaries, but I could replace that with the real one.
Not that we really care. And you could have gotten the beta off of some torrent or whatever, without ever agreeing to the NDA anyway...
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
how do I patch on OS X Mavericks? Software update shows nothing to update
Install Ubuntu.
I kid, I kid.
Client's that aren't vulnerable should flip out when trying to load that.
Looks like using Chrome instead of Safari may help; I'd say it would be more interesting if standard mail client can be fooled.
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++;
}
if (sslVersionIsLikeTls12(ctx))
{
err = SSLVerifySignedServerKeyExchangeTls12(ctx, sigAlg, signedParams,
signature, signatureLen);
} else {
err = SSLVerifySignedServerKeyExchange(ctx, isRsa, signedParams,
signature, signatureLen);
}
I checked inside the SSLVerifySignedServerKeyExchange disassembly and the compiler expectedly omitted the remainder of the function, so it isn't as simple as sticking a few nops in.
I wonder if something simple (and stupid) like an LD_PRELOAD with an alternative fixed SSLVerifySignedServerKeyExchange would work. Won't work if the code ends up getting inlined.
Guess not, looks like SSLVerifySignedServerKeyExchange is static.
I'll admit I don't think of myself as great at unit testing, but the first thing I do when writing one for a new tool or class is use a code coverage tool to look for uncovered lines and once I have written a few basic behavioral tests I write tests to exercise and validate the output of the uncovered lines.
This ensures that the tests I write to cover the public API don't miss catching internal behaviors that require more subtlety to uncover.
...or an integration test. or a functional test.
When you write an SSL lib, I suspect that at some point you ought to test that it checks f%^&ing certificates :/
My theory is that Apple is spread too thin. Kayak.com reproducibly crashed MobileSafari the day iOS7 shipped, and brand new iPad minis regularly kernel panic and reboot.
It wasn't always like this.
Someone badly needs to sit down and write — preferably a black-box, so it can be used for all — testsuite for SSL/TLS.
Unit tests would've caught this, though.
The entire OS is damn too huge piece of software for Apple not to test intesively to a fanatic stretch. Since all the NSA revelation and the proven extent to which US government lied and continues to lie to its people about all data gathering, I fail to categorize such a simple but yet brilliant overlook as a mistake.
By the way the test coverage in OpenSSL is piss-poor as well.
This is a perfect example of just how bad that suggestion is, and just how disastrous it can be. It's totally worth typing a few extra characters here and there to basically avoid these kind of situations completely.
This also applies to the "JS without ;" crowd.
You may think you're too good to know all the rules of ; or you can just don't think about it, and worry about other things instead, like your code.
I certainly see far more bugs caused by an improperly inserted semicolon than an improperly omitted semicolon, but then I'm looking at jshinted code most of the time.
IIFEs are the most common source of semicolon problems:
for(i = 0; i < 10; i++){
x = arr[i]
(function(x) {
setTimeout(function(){console.log(x)})
})()
}
init()
[x, y, z].forEach(function(a){
if(a.length > 5) {
process(a)
}
})
Now sure, you can prepend semicolons to these specific cases, and that will work. Here's why I dislike that:
1. Those leading semicolons look weird. To the uninitiated, it looks like a typo, and that's a significant hazard if anyone else is going to touch your code.
2. While there are only a handful of special rules like this to remember, there's only one rule to remember if you don't rely on ASI.
Your examples will crash immediately and at the right spot though. The problems I see caused by excessive use of semicolons are often far weirder.
That said, inadvertent errors caused by semicolon insertion are still more common and baffling (especially by people addicted to jslint who use a variable declaration idiom particularly easy to screw up with errant semicolons).
A relatively rare scenario, but a brutal one to debug.
As for errors caused by extra semicolons, they can be weird, but I don't think I've ever actually hit one in practice. They'd also be a little easier to spot, since you tend to develop a reasonable instinct for where semicolons belong.
Hence these are the cases where more time and resources will be wasted because of it.
"I certainly see far more bugs caused by an improperly inserted semicolon"
What would be an example of this? Because I've seen exactly zero bugs of this type (not counting typos, of course)
(usually this will be across multiple lines)
...just overwrote c in a different scope. This kind of bug is common, idiomatic, baffling, and actually more likely among coders subscribing to javascript "best practices".
This doesn't justify playing a guessing game and skipping semicolons just because you think you know all the rules about not using them.
if (something) do_something();
if (something)
do_something();
That's fine until you write
if (something) do_something(); do_something();
if (something); do_something();
You haven't really changed the dimensions of the problem by putting it all one one line. Only the whitespace is different. Sure it looks wrong to you; but so does the incorrect code from apple.
I don't understand the need to omit braces; it doesn't make the compiled program any smaller. And denser source code is not always more readable, or we'd prefer "? :" to "if" every time.
if (something); { do_something(); }
The first line of defence is consistent code layout; e.g. always use "{ }" after an if, and only one statement per line. This aims to make simple typos less likely to compile, make the code more readable and so make errors stand out.
Then there is using and paying attention to the linter / compiler warnings / jshint + 'use strict' / static analysis or whatever it’s called in your language of choice.
Then there are unit tests or integration tests.
Any of these might have caught the apple error, but there are no absolute guarantees.
IMHO it's a professionalism issue – gifted amateurs are great, and they can succeed... most of the time. But a professional engineer would put aside ego "I won’t make mistake like that" and laziness "I don’t need to type braces this time" and do the careful thing every time. Because each step raises the bar, and makes it harder to ship code with bugs like this. Because sometimes it matters.
if (something); { do_something(); }
So..
if(something) {do_something();}
Many code editors will easily insert curly brace pairs, so it's really just a single extra key stroke you're saving.
Two gotos immediately after one another at the same indentation level is obviously wrong by visual inspection.
Proper indentation might have led the casual viewer to think that the code was actually ok!
This is what happens if you try the same thing in it http://imgur.com/syt6LuU.
I have emacs setup to auto run clang-format on each save of cc-mode files. I can't NOT notice this stuff now.
Outside of python, for obvious reasons, it should never be allowed. The slightly nuts conditionals in Erlang are justifiable for avoiding exactly this kind of problem.
They exist because of the long, bloody history of mistakes by very smart people.
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
{
goto fail;
}
{
goto fail;
} if (condition)
{
doSomething();
}
if (condition)
{
doSomething();
}
{
doAnotherThing();
}
if (condition)
{
doSomething();
doAnotherThing();
}
As far as the original example goes, if it's an error it's most likely a copy/paste error. Curly braces help there, too. With three times as many lines in the block, the odds of a paste error resulting in code that even compiles is greatly reduced, and a compiler error should call attention to the issue.
Even assuming the bug was malicious, the curly braces would increase the odds of it being caught by another developer. This is particularly the case now that offside rule languages are common. A large chunk of younger devs cut their teeth on languages where
if (condition)
doSomething();
doAnotherThing();
Right, and this demonstrates the major problem with verbosity in languages and APIs and design patterns. When you have to repeat yourself many times, it's very easy to make a mistake in one of the near-copies, and you or a code reviewer can miss it because you'll tend to quickly skim over the boilerplate.
For cases like this, using exceptions rather than manual error value checking would make your code shorter, less redundant, and not susceptible to this particular bug. (Not possible in C, I know).
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;
}If there were a language feature available to do this more elegantly, whether exceptions or something else such as a Haskell-style monad with fail semantics, I'd almost certainly agree with using it in preference to a series of manual checks, though.
If not, `|| (err = check())` is equivalent to separate checks which also also `goto fail` immediately.
Left-to-right, short-circuit evaluation of the logical operators is so deeply rooted in C idiom that a mainstream compiler that doesn't respect it would be practically unusable. But perhaps it wouldn't work in a hobbyist compiler, or a non-standard dialect of C.
While valid according to the language definition, assignments within conditional expressions do have a reputation for being error-prone and not always the easiest code to read. Sometimes they're still neater than the available alternatives.
However, IMHO in this case, it feels a bit too easy to break the code during later maintenance edits. For example, someone might simplify the code by removing the explicit comparisons with 0 that are unnecessary here, and then later someone else might "fix" an "obvious bug" by replacing the "(err = x) || (err = y)" construction with the common construction "(err == x) || (err == y)" that they assumed was intended, perhaps prompted by a compiler warning. Obviously they shouldn't if they didn't properly understand the code, but when has that ever been a reliable guarantee? :-)
But yes, mandatory braces on the same line is the correct choice.
if ((err = DSomething()) != 0)
goto fail;
While Go has goto for tricky situations like this, because it has defer you don't have to use it often, assuming the free calls were needed (and the vars were not going to be GC'd):
defer SSLFreeBuffer(&hashCtx)
defer SSLFreeBuffer(&signedHashes)
if err = SSLHashSHA1.update(&hashCtx, &serverRandom); err != nil {
return err;
} else if err = SSLHashSHA1.update(&hashCtx, &signedParams); err != nil {
return err;
} else if err = SSLHashSHA1.final(&hashCtx, &hashOut); err != nil {
return err;
}
return nil;
}err = SSLHashSHA1.update(&hashCtx, &serverRandom); if (err != nil) { return err; }
Is there objective evidence for this? As a Go programmer a semicolon in an if statement screams to me. I can see it possibly being in issue for new Go programmers- but I don't remember it being one for me.
I rarely use the one-line `if err := ...; err !=nil ` idiom because its quite a mouthful. However when I do, I try to make sure it's not too much to grasp at once. Here the extra `else` goes against that.
Alright, I know this is just a quick snippet on HN and all, I just thought I'd mention it anyways. Maybe next time you actually write that in code you'll think about my point. ;)
This is not spaghetti code or what Dijkstra talked against.
It's one of the two things one hears novices: that gotos are to always be avoided (because they heard that they are bad), and that we should write stuff in assembly (cause they heard that it's fast).
The problem is Apple (intentionally) understaffs and overworks, so I doubt they have the spare people to look at most commits.
You can't really blame them for doing the best they can with limited resources. Startup life is hard.
Can someone explain to me what this code is? It appears to be implementing standard crypto procedures -- are there not suitably licensed public implementations that apple can use? If they are writing it themselves, why are they open sourcing it? (I don't have a reason why they shouldn't it's just that I've never associated apple with much interest in open sourcing things).
Using public implementations for crypto code has been difficult until pretty recently. There didn't exist many high-quality, trustworthy, public implementations of the types of crypto that real-world systems have wanted to do.
Even now, the situation is pretty grim for anyone who wants to integrate crypto code into an existing large codebase. The canonical answer is "just use NaCl." However, I've had developers outright refuse to use NaCl because "it's so hard to compile." They were saying NaCl has some issues compiling on certain types of architectures, and issues compiling as a dynamic library. When libsodium was suggested in response, they retorted that libsodium was "NaCl but with modifications by people other than the ones you'd want making modifications to crypto code." In other words, libsodium purports to be "NaCl, but easy to integrate into your project," yet the reason it's easy to integrate is because someone other than recognized cryptography experts fiddled around with NaCl until it was easy to compile. So apparently it's an open question whether libsodium is as trustworthy as NaCl, and NaCl is a pain to integrate.
All of this implies that the current situation is still not very good as of 2014 for people who want to just do crypto safely and in some standard way. And rather than writing that code in 2014, Apple had to write it years ago, when the situation was far more painful than the present day's shortcomings.
To answer your question, yes, there are public implementations which could theoretically be used. But actually using them is... difficult. Not nearly as difficult or as error-prone as rolling your own, but perhaps difficult enough where someone who isn't an expert in cryptography might make the dire mistake of believing that rolling their own is less difficult.
I wish Matasano would publish an open-source crypto library. An "NaCl that's easy to use and that people actually trust." They have the resources and the credit to pull it off.
Alternatively, I wish we could gather funds to have them audit libsodium.
Also, all of this is based on the assumption that NaCl or libsodium actually meet the needs of what Apple was trying to do with this crypto code. It probably doesn't, because NaCl / libsodium provides a complete replacement to the entire crypto stack of an application. To take a wild guess, I'm thinking that Apple needs to implement / interact with some other kinds of crypto than what NaCl/libsodium provide. For example, certificate validation. So then you'd suggest "use cryptlib!" but this is hampered by the fact that in addition to all the previous considerations, cryptlib also isn't free.
Interesting. Never thought about it that way before.
indent - indent and format C program source
HISTORY
The indent command appeared in 4.2BSD.
NAME
cb - C program beautifier
SYNOPSIS
cb
DESCRIPTION
Cb places a copy of the C program from the standard input on the stan-
dard output with spacing and indentation that displays the structure of
the program.This is the type of error that any static analysis tool would easily pick.
You want the compilers knowledge of the code for this, e.g. while formating C++ template mess.
There's a tight integration into the actual toolchains, which is a good thing.
Having cast-in-stone formatting rules makes code review easier and shorter and avoids lots of arguments. For ultra-dangerous languages like C++, automatic formatting is indispensable.
It should relatively simple to NOP out the 2nd goto, if the rest of the function hasn't been optimised away.
I believe you can dump the assembly of the entire function like this:
otool -t -p _SSLProcessServerKeyExchange \
-V /System/Library/Frameworks/Security.framework/Versions/A/Security | less
0000000000086df6 jmp 0x86e0d
edit3: Looks like the compiler did optimize it out if I'm reading it correctly now. The code is around 0x86c97. It does the last if() call and then immediately calls SSLFreeBuffer() and jumps to the end. :(
0000000000086c80 callq *%r14 ; SSLHashSHA1.update(&hashCtx, &signedParams))
0000000000086c83 movl %eax, %ebx
0000000000086c85 testl %ebx, %ebx
0000000000086c87 jne 0x86c9c
0000000000086c89 leaq 0xffffffffffffff38(%rbp), %rdi
0000000000086c90 leaq 0xffffffffffffff58(%rbp), %rsi
0000000000086c97 callq *%r14
0000000000086c9a movl %eax, %ebx
0000000000086c9c leaq 0xffffffffffffff08(%rbp), %rdi
0000000000086ca3 callq _SSLFreeBufferWhoever wrote that code even before it had the bug was an incompetent cowboy hotshot who wanted to show off how pedantically he knew and could surf the nuances of syntax and optimize the code to have the smallest source file size by penny pinching brackets.
"ALWAYS USE BRACKETS" is one of the oldest and wisest rules in the book, and this shows why. Anyone who actually writes security related code like that, blatantly ignoring important coding practices in the name of "aesthetic beauty" or however they rationalize omitting brackets, should be fired.
I bet they're the same cowboys who fanatically prosthelytize JavaScript automatic semicolon insertion too, because that's another outlet for their obsession with showing off how clever they are to write code that requires them to deeply understand and consider every nuance of the syntax at every keystroke, instead of facing the fact that they or somebody who comes later might actually make a mistake, and coding defensively.
Maybe they really are fucking geniuses who never make mistakes, but that's not true about everyone else who has to read and modify the code.
Apple should go through all their code and fix it, and dock the salary of anyone caught not using brackets.
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.
If the code was closed all we would have is Apple's release note which just says validation steps were skipped...
Deleted comment
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);
Not to be a conspiracy theorist, but history tells that computer technology was released to the public, so to me it still looks like there is no absolute objective, safe way to use a computer for a layman. It still looks more like a tool of surveillance than a tool of communication.
You'd want my opinion, but I'd be apple (or Linux for the days that happened for a certain commit with a =!0 thing), I'd try to sue the guy who was responsible for this commit.
You could argue in court that it's impossible to prove if it's voluntary, but I guess we must set an example and start to hold anyone responsible for this. It's time to set the courts aware of computer security standards...
Of course you can.
> It is the company's responsibility ...
Not if the programmer was hired for his independent expertise rather than to be told what to do, and how to do it, by higher-ups. That's a a common arrangement in small companies.
> There are so many bugs in software, that this would be a terrible standard to set, and no developer would be safe.
All true, but surely you realize that lawyers and judges don't necessarily ask themselves, "Is this fair?"
Remember that a group of scientists were successfully convicted of manslaughter in Italy for not predicting an earthquake. It was a different country and legal system, but not that different.
http://abcnews.go.com/International/scientists-convicted-man...
The handwriting is on the wall.
1. That does not excuse software from a proper code review process, in fact any reasonable software company would probably see this as even more of a reason to code review the code. If I knew that a software company doesn't properly review critical code, I would definitely think twice about using that company's software.
2. Apple is not a small company so that doesn't excuse them in this case.
>All true, but surely you realize that lawyers and judges don't necessarily ask themselves, "Is this fair?" Remember that a group of scientists were successfully convicted of manslaughter in Italy for not predicting an earthquake. It was a different country and legal system, but not that different. http://abcnews.go.com/International/scientists-convicted-man....
Come on this is the Italian legal system. No offense to Italy, I believe it to be a beautiful country, but they have had a shocking record in their legal system as of late. And I highly doubt that this would have stood up in many other western legal systems.
But even if I concede to all your points, someone on HN should be smart enough to realise that suing someone for a software bug would definitely not be a smart move. And we definitely shouldn't be recommending such a thing. Just like we shouldn't be recommending scientists to be convicted of manslaughter for not predicting an earthquake.
> 1. That does not excuse software from a proper code review process, in fact any reasonable software company ...
You changed the subject. The original subject was whether a programmer could be held accountable for a bug in a computer program. The answer is yes, under civil law, that can happen.
There are companies whose computer science departments consist of one person, which was my example, and in such a case, there is no company-wide software review process, because -- as I already said -- the computer science department consists of one person working alone.
Feel free to change the subject if you want.
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.
seriously, tho, it is probably the whole binary this was part of.
The answer here, folks, is don't update until all the bugs have been found and fixed. Apple's golden days of OS updates was back in 10.6, when they spent most of the time just fixing bugs rather than adding new features. Every OS update since has had as many bugs as it fixed, though overall things were getting better.
It's the old Wait-until-SP1 advice. For security, it's often true. The exception being if the patch cycle is fast enough...I'm not going to say for sure whether one is better than another.
How can you ever be sure of this? And also I can almost guarantee that there is no non-trivial software that is bug free. As a programmer the only way you can guarantee bug-free code is to not program at all.
So if you follow this advice, you will never update at all, which in my opinion is bad because the benefits of an update outway the negatives.
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.
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.
Sorry, but I'm going to call you in turn on that one.
Of course there are some unwise things you can do in the presence of the C++ exception mechanism, and 15 or 20 years ago plenty of us did them. I think probably Herb Sutter deserves more credit than anyone else for drawing attention to these things, but the point is valid in any case.
However, we've figured out a lot since then, and for a long time writing exception-safe code from scratch in C++ has been quite straightforward. Just follow the common idioms, which mostly boil down to "use RAII for anything that needs cleaning up".
The biggest difficulties with exceptions in C++, IME, generally come from trying to retrofit them into code that was written without exceptions in mind. If your existing code doesn't necessarily follow even basic good practices for writing exception-safe code, the WarGames rule applies.
- The bug here has nothing to do with GOTO.
- Dijkstra said a lot of stupid things, this is one of them. Parsimonious use of GOTO is fine and sometimes leads to clearer code (exiting deeply nested conditions for example).
- Another stupid thing that Dijkstra said is that anyone who started by learning BASIC has their brain corrupted forever, which is total nonsense (I bet most people reading this started with BASIC and turned out just fine).
First, the problem here is totally different than the goto, it's the uncoditional execution of something that was meant to be in a if clause. It could have been anything, even without goto, and have the same problem.
Second, this is not the kind of use of goto that Dijkstra warned about. This is perfectly fine, and widely used from the best programmers.
The error is in the lack of an if guard.
E. W. Dijkstra, cited by Donald E. Knuth in Structured Programming with go to Statements, ACM Computing Surveys, 6 (4), 1974
Edit: it's also digitally signed
Code Reviews? Static Analysis?
/sigh
Admittedly in the scenario I lay out - what happens is an error is thrown and not caught, rather than a check being skipped, but the net effect is equivalent - and it takes 1 character out of place to invoke.
If you always throw by value (and not by pointer) then it does not matter if you catch by value or by reference, the catch block will be activated either way. The only issue is possible slicing if you throw a derived instance and catch a base class type. You might lose error information that way, but the program will not crash due to an uncaught exception.
int e;
int update() { throw &e; }
the & is the one character - it's a typo - only catch(...) will catch it.
I guess you mean there will only be a catch(int) in addition to catch(...) so in that aspect you're right of course.
Still, I would think taking the address of something you throw should ring quite a few warning bells as opposed to merely a missing ref (&) in the catch handler. Similarly to "return &e" which would also be suspicious and require an extra look or two.
I've done a bunch of kernel and other systems work in C and in C++, and my experience is that the C is a lot clearer. This type of code is all about not having magic side effects; everything needs to be in the open and very plain, or bugs start to get pretty subtle and hairy.
I'm not saying "Don't do systems programming in C++", because clearly you can. But it takes discipline to succeed, probably more discipline than you need to apply than if you're writing C.
At least, short of making it mandatory it would at least signal bad style.
This code would also be about half as long in straight-forward C++.
The same applies to any other language that allows for copy-paste of C code.
There's even this bullshit:
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
goto fail;
In short: wow.
Not in C.
i've used it myself to optimize inner loops of very simple un-accelerated graphics rendering code (which sped it up considerably, since it is potentially skipping many levels of unnecessary execution on the cpu, millions of times), but i agree with some posters here, using it in a security context like this is a bit daring.
still, having said that, this bug really is an indictment of the testing process and not really bad style per se.