8 ms·
Take a look at http://opensource.apple.com/source/Security/Security-55471/libsecurity_ssl/lib/sslKeyExchange.c?txt http://opensource.apple.com/source/Security/S
by pencilo 13y ago
Take a look at http://opensource.apple.com/source/Security/Security-55471/libsecurity_ssl/lib/sslKeyExchange.c?txt 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).
- Someone 13y agoFor 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.
- pencilo 13y agoYou don't need fancy software, compilers warn about this kind of thing.
- d0 13y agoJust 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.
- Someone 13y agoOops. 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.
- mau 13y agoDon'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.
- d0 13y agoYou 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.
- jey 13y agoThe 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.
- fit2rule 13y agoLooks to me like Apple aren't running Code-Coverage tests on their OS. That's kind of very scary.
- acdha 13y ago> 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.
- guycook 13y agoWhoops. goto fail indeed
- d0 13y agoThat's hilarious. Took me a while as I was looking for something more subtle. Thanks for the clues. Another clue: indentation fail.
- PhantomGremlin 13y agoPeople 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.
- wreegab 13y ago> 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.
- baq 13y agoit'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.
- rplnt 13y agoCan't really write onliners. And use of otherwise great interactive shell is not as good (not that bracketed blocks would help).
- schnable 13y agoStyle guideline of using curly braces with ALL if statements, even one liners, would have gone a long way to prevent this.
- lzhz 13y agoCode reviews and comprehensive test suites for critical code would have prevented this and many other kinds of mistakes besides.
- PhantomGremlin 13y agoHere'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.
- hahainternet 13y agoHow could this not happen in Python?
- manojlds 13y agoBecause assigning in if would be a syntax error in Python.
- PhantomGremlin 13y agoPython does not allow an assignment to occur within an expression. They deliberately chose that restriction, to avoid that hard-to-see bug.
- staticfish 13y agoI have always wondered why all languages don't enforce this?
- joveian 13y agoBecause 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.
- cnvogel 13y agobecause, for example in C, such a incompatible change would alienate most developers used to the very ideomatic if((val = function(args)) != expected) return val;
- deleted 13y ago[deleted]
- alex1 13y agoHere'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 https://gist.github.com/alexyakoubian/9151610/revisions Check line 631. Appears seemingly out of nowhere.
- gojomo 13y agoGood 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?)
- alex1 13y agoGood 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/ 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.
- chaitanya 13y ago(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.
- spoint 13y agoI partially agree. Curly braces won't fix a typo or a rushed code review.
- saalweachter 13y agoDoes 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?
- nickbauman 13y agoWe'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.
- jamiesonbecker 13y agoCloudFlare 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-source-implementation-of-the-two-man-rule http://blog.cloudflare.com/red-october-cloudflares-open-sour...
- deleted 13y ago[deleted]
- nikster 13y agoConspiracy 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.
- chengg11 13y agoOf 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.
- juliok 13y agoMy 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!
- hawleyal 13y agoIt'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.
- nymble 13y agoThis does not look like a merge error. The added line stands alone and it's very well located for an exploit.
- aes 13y agoJust 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(...)".
- java123 13y agoI 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.
- rennison 13y agoDamn 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.
- WaltSellers 13y agoWhat 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.)
- WaltSellers 13y agoWas 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".