Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Apple's SSL/TLS bug

imperialviolet.org

Post reply on HN
Print view View original

191–200 of 295 posts

Re: Apple's SSL/TLS bug

#191

Post by pjmlp »

eliteraspberrie wrote: post #175
pjmlp wrote: post #45

Earlier quoted context omitted.

There is no excuse to bypass static analysis nowadays. At very least should be part of the continuous integration build. The problem are the many developers that still think they are perfect and know the full C standard, including undefined parts.

Unfortunately the decision to use static analysis tools would have to come from developers who are comfortable admitting they make mistakes sometimes. 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.

Fully agree, I met quite a few of those egos already.

Re: Apple's SSL/TLS bug

#192

Post by Silhouette »

kabdib wrote: post #63
taylodl wrote: post #26

Dijkstra 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 d…

If you're using C++ exceptions you're in serious trouble.

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.

Re: Apple's SSL/TLS bug

#193

Post by beachstartup »

kabdib wrote: post #174
thrownaway2424 wrote: post #151

The code is crap and while I can understand why a single person might have written it this way, I think any organization of full-time professional software developers should be collectively embarrassed to have accepted it. The only reason to use the "goto fail;" idiom is to free two buffers before returning. But the buffers in question are just sslBuffer structs that live on the local stack. Their destructors will be…

Nope, it's actually pretty decent C. With a bug.

this pattern is structured to avoid the use of an additional stack (local) variable to track a condition, and to skip superfluous execution of code once that condition is detected.

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.

Re: Apple's SSL/TLS bug

#194

Post by TwoBit »

eliteraspberrie wrote: post #175
pjmlp wrote: post #45

Earlier quoted context omitted.

There is no excuse to bypass static analysis nowadays. At very least should be part of the continuous integration build. The problem are the many developers that still think they are perfect and know the full C standard, including undefined parts.

Unfortunately the decision to use static analysis tools would have to come from developers who are comfortable admitting they make mistakes sometimes. 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.

OpenSSL was written by monkeys: http://www.peereboom.us/assl/assl/html/openssl.html

Re: Apple's SSL/TLS bug

#195

Post by TwoBit »

oneeyedpigeon wrote: post #72
throwaway_yy2Di wrote: post #62

Earlier quoted context omitted.

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

The real WTF is those Warn flags, then. In what sane world does "extra" mean more than "all", and "all" and "everything" mean different things?

Microsoft VC++ has a -Wall warning level too, and it really does mean all warnings. Each major version of VC++ potentially introduces new warnings to -Wall.

Re: Apple's SSL/TLS bug

#196

Post by Silhouette »

bunderbunder wrote: post #185
orangecat wrote: post #138

Earlier quoted context omitted.

As far as the original example goes, if it's an error it's most likely a copy/paste error. 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, u…

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 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.

Re: Apple's SSL/TLS bug

#197

Post by pjmlp »

TwoBit wrote: post #194
eliteraspberrie wrote: post #175

Earlier quoted context omitted.

Unfortunately the decision to use static analysis tools would have to come from developers who are comfortable admitting they make mistakes sometimes. 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.

OpenSSL was written by monkeys: http://www.peereboom.us/assl/assl/html/openssl.html

Very enlightening.

Re: Apple's SSL/TLS bug

#198

Post by rythie »

How do we even know we are downloading the real iOS 7.0.6? - surely it downloads the new version using this library?

Edit: it's also digitally signed

Re: Apple's SSL/TLS bug

#199

Post by Tloewald »

raverbashing wrote: post #189
Tloewald wrote: post #184

Earlier quoted context omitted.

var a = 17, b = 13; c = 5; (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".

use strict? jshint / jslint your code? This doesn't justify playing a guessing game and skipping semicolons just because you think you know all the rules about not using them.

jslint won't stop it if there's a variable (c in this case) in the outer scope -- it's perfectly fine code as far as jslint is concerned. And jslint encourages that declaration style (actually encourages is too weak a word).

Re: Apple's SSL/TLS bug

#200

Post by Tloewald »

mistercow wrote: post #190
Tloewald wrote: post #183

Earlier quoted context omitted.

Good examples actually. (Makes me feel better about my ingrained semicolon habit.) 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…

The first example won't crash if the rvalue preceding the IIFE is a higher-order function (specifically, one that outputs a function). 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 be…

I'd say it's a lot easier to be suspicious of lines of code that start with '(' or '[' than find semicolons at the end of the wrong line. It's incredibly easy to put a semicolon where a comma is expected and vice versa. I do it all the time (usually causing an immediate error so it's not a huge deal).
Post reply on HN