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

271–280 of 295 posts

Re: Apple's SSL/TLS bug

#271

Post by jjgreen »

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.

Please don't fall into the trap of believing that I am terribly dogmatical about [the go to statement]. I have the uncomfortable feeling that others are making a religion out of it, as if the conceptual problems of programming could be solved by a single trick, by a simple form of coding discipline!

E. W. Dijkstra, cited by Donald E. Knuth in Structured Programming with go to Statements, ACM Computing Surveys, 6 (4), 1974

Re: Apple's SSL/TLS bug

#272

Post by marcan42 »

llimllib wrote: post #57
arielweisberg wrote: post #56

How is this code not covered by a unit test? 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…

> How is this code not covered by a unit test? ...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 :/

An integration test wouldn't catch this one. You need a specific malicious SSL server (presents a valid certificate, uses ephemeral mode, does not present a valid signature proving that it owns the private key). The code does validate certificates, otherwise this would've been caught ages ago by anyone trying an invalid cert.

Unit tests would've caught this, though.

Re: Apple's SSL/TLS bug

#273

Post by SimHacker »

I'd love to see the git blame logs for that one.

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

Re: Apple's SSL/TLS bug

#274

Post by SimHacker »

terminus wrote: post #236
pjene wrote: post #100

Earlier quoted context omitted.

Sure, this aligns with interests, but the bug's existence is predicated on the entire code base being written with substandard style rules and no static analysis or tests , which suggests to me that incompetence got here first.

> entire code base being written with substandard style rules and no static analysis or tests, which suggests to me that incompetence got here first. 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.

Whether it was incompetence or malice, whoever was responsible for the extra goto, as well as whoever was responsible for ANY of that code not following common practices including ALWAYS using brackets, should be fired.

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.

Re: Apple's SSL/TLS bug

#275

Post by SimHacker »

valleyer wrote: post #218
huhtenberg wrote: post #171

Earlier quoted context omitted.

Yeah, that'd trigger a warning in most compilers - "Empty statement found. Was it an intent?" or something similar.

Not true, try for (;;) ; in gcc / clang

That's because "for (;;) ;" is and idiom that's commonly used in a programming pattern called the "for(;;);ce field", to work around other C and C preprocessor quirks, by syntactically insulating and protecting statements in C preprocessor macros from outside interference.

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.

Re: Apple's SSL/TLS bug

#276

Post by steeve »

apaprocki wrote: post #120
steeve wrote: post #92

Does anybody know the .dylib or binary file where this function resides on OSX? I can't find libsecurity_ssl on my system. It should relatively simple to NOP out the 2nd goto, if the rest of the function hasn't been optimised away.

The static function that has the bug will most likely be inlined into the other static function which will then be inlined into the outer public symbol that calls it. 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 edit: I believe this could be the offending instruction: 0000000…

Confirmed here too :(

    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   _SSLFreeBuffer

Re: Apple's SSL/TLS bug

#277

Post by ZoFreX »

ZoFreX wrote: post #166
jdsnape wrote: post #15

This shows another of the benefits for the community of open sourcing code - because we can see exactly where and what the bug was steps can be taken in other projects to stop it happening there (I think Adam mentioned he was going to check for a test case in Chrome). If the code was closed all we would have is Apple's release note which just says validation steps were skipped...

The code in question is open source.

Wish I could delete that comment - reading comprehension failure!

Re: Apple's SSL/TLS bug

#279

Post by SideburnsOfDoom »

gsg wrote: post #259
SideburnsOfDoom wrote: post #216

Earlier quoted context omitted.

> I do write single statement conditionals without braces, but I write them on one line That's fine until you write if (something) do_something(); do_something(); or worse: if (something); do_something(); I've actually seen something very similar to that one. 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 do…

Note that braces won't save you from if (something); { do_something(); } (Personally, I configure my editor to highlight if (...); in bright red.)

This is true; there is no silver bullet. So we rely on defence in depth.

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.

Re: Apple's SSL/TLS bug

#280

Post by lloeki »

brudgers wrote: post #19
rdl wrote: post #8

It'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…

My gut says the probability of a random typo executing without raising an error or exception is rather low. The probability of it doing so in a way that well aligns with the interests of nation states, large corporations, and/or criminal enterprise is even lower. This might explain DROPOUTJEEP particularly considering how much more efficient it is than breaking messages after they are encrypted. http://mobile.eweek.c…

> My gut says the probability of a random typo executing without raising an error or exception is rather low

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

[1]: http://en.wikipedia.org/wiki/Underhanded_C_Contest

Post reply on HN