Permalink
29 comments
on commit
sign in to comment.
Browse files
PR: 2658
Submitted by: Robin Seggelmann <seggelmann@fh-muenster.de> Reviewed by: steve Support for TLS/DTLS heartbeats.
- Loading branch information...
Showing
with
561 additions
and 4 deletions.
- +3 −0 CHANGES
- +20 −0 apps/s_cb.c
- +8 −0 apps/s_client.c
- +10 −0 apps/s_server.c
- +151 −1 ssl/d1_both.c
- +13 −0 ssl/d1_clnt.c
- +8 −0 ssl/d1_lib.c
- +13 −0 ssl/d1_pkt.c
- +13 −0 ssl/d1_srvr.c
- +1 −1 ssl/dtls1.h
- +12 −0 ssl/s3_clnt.c
- +21 −0 ssl/s3_lib.c
- +13 −0 ssl/s3_pkt.c
- +12 −0 ssl/s3_srvr.c
- +24 −2 ssl/ssl.h
- +4 −0 ssl/ssl3.h
- +4 −0 ssl/ssl_err.c
- +7 −0 ssl/ssl_locl.h
- +211 −0 ssl/t1_lib.c
- +13 −0 ssl/tls1.h
20
apps/s_cb.c
152
ssl/d1_both.c
13
ssl/d1_pkt.c
21
ssl/s3_lib.c
13
ssl/s3_pkt.c
Oops, something went wrong.
bd6941cDoh!
bd6941cThe ramifications are of truly epic proportions.
bd6941cI don't see you guys implementing this.
perhaps you should criticize less, and maybe peruse the code looking for other bugs.
bd6941c@Dijit That's a little uncalled for. I don't think anyone here is directly criticising this commit or its authors (everybody writes bugs!) however the code here is currently causing massive ramifications for a large chunk of the internet and it's therefore worthy of note in at least some way.
bd6941cOh, shit!
bd6941cI too have had some bad commits... or even good commits with horrible unforeseen ramifications.
I'll totally buy you a drink @snhenson -- I bet you could use one.
Best of luck all.
bd6941cSorry, but whoever wrote this rubbish shouldn't be allowed near C compiler, much less critical libraries like this one. Also, it is blatantly obvious that this code was either not reviewed, or reviewer was at least as incompetent as person who wrote this.
Take a look:
@tls1_heartbeat(SSL *s)
and
@tls1_process_heartbeat(SSL *s)
one does not simply check if malloc failed? Mind you, that all other calls to OPENSSL_malloc in relevant file do check if null was returned, so it wasn't designed in infinite memory / abort on failure environment. How had such code even passed the review?
Also, I love how it's "1 + 2 + payload + padding" in one place and "3 + payload + padding" in other, in case someone was attempting to understand this.
bd6941c@Amelek I agree, but it's not only this guy. The OpenSSL source is plagued with antiquated and terrible programming practices - more specifically the terrible naming conventions (or lack thereof) that are completely non-descriptive, ambiguous, and quite frankly just plain stupid. Look at char * buffer, char * p, int r, n2s and s2n.
It's impossible to know what these are without reading the code around it for context. Why not just put the extra effort and name it response_buffer and response_buffer_ptr and save time for everyone else who has to inspect that code.
These bugs happen because the people who write them can neither read nor review it themselves, and obviously neither can anybody else without spending a disproportionate amount of time figuring out what the hell you were trying to do.
Also if you use _bp++ =, shave your unix beard and remember you live in 2014. Writing code for a large scale project doesn't need any of your 1337 bullsh_t.
bd6941cClassic Robin and steve.
bd6941cFucking steve.
bd6941c@AdditionalPylons
Are you serious? Who doesn't use this?
bd6941cPeople should write and review the code (preferably while still in dev) instead of ranting about it...
bd6941c@kivikakk
Probably everybody uses this. There is however major difference between everybody and person writing user-facing code of one of the most critical libraries used in Linux environment. There is no place for 1337 code in OpenSSL nor any user exposed code. And even if we stomach single letter local variables, look at this:
&s->s3->rrec.data[0]"s" is SSL* context, then "s3" is ssl3_state_st*, "rrec" is actually decrypted user content. How is this 21 century coding practice? And &x[0] seem to be an idiom for people who never attempted to understand C.
Tbh, I think this whole patch should be reverted and this feature removed. There is no need to have keepalive protocol on the encryption layer, and even if there exists some bizarre environment where this is required, keepalive protocol should not allow user defined payload. 64kb of user payload is criminal.
bd6941c@Amelek The thread needed a "this programmer shouldn't be allowed near a computer" Dunning-Kruger representative. So thanks for stepping up.
The effective problem with this code, as I'm sure you can figure out by reading your nearest tech journal, has nothing to do with anything you've pointed out. Yes, the style of OpenSSL is arcane, but it's consistently so throughout the library. That's not the problem. This particular change set is neither worse nor better than the core of the library.
If you want to understand what's wrong with the OpenSSL code, compare it against the Linux kernel code. The kernel is the largest collaborative software project in the world and is easily one of the most readable, but by your standards, none of the developers should be allowed near a compiler. Just look at those variable names! And the pointer arithmetic!
No, If you want to understand what's really wrong with OpenSSL, look through the code to see how a function like
EVP_CIPHER_CTX_new()is implemented. Along the way you'll discover a very key difference between the two codebases. That is the real reason why you shouldn't trust OpenSSL. Not this code here. This was just a logic failure which would have been just as easy to make if your own coding standards had been followed.bd6941c@tylerl Damn it. You had to give just enough tantalizing hints without any explanation, so that now I have to spend the next day or two crawling through code bases to satisfy my curiosity. Haha
bd6941c@Amelek
FWIW, I've worked with a compiler where &x[0] was necessary because char[] would not implicitly decay to char*. I believe even the C89 standard requires this decay, but you've got to work with the toolchain you're given.
bd6941cEpic
bd6941cDope!
bd6941cNightmare!
bd6941cSo it is generally the case for OpenSSL that major additions of features have no tests for them whatsoever? That makes me sad. Shouldn't I be able to have my browser/client/server refuse SSL connections with implementations that I don't trust or are know to have flaws? Sure an endpoint could lie about what code it was running, but it would at least be effective against non-malicious flaws.
bd6941cI just wanted to ask the same as @O1O1O1O. Even small projects doesn't accept contributions without the corresponding test suite... But hey, it's written in C, what do you expect... Not that I know of any seriously practical OS or core library which was written in a test driven way, using some other language than C, BUT maybe it's time to raise the bar and start leaving this computing dark medieval age behind us...
We saw better security architecture before (centralized into the OS, seamlessly exposed to applications):
http://plan9.bell-labs.com/sys/doc/auth.html
Integrated systems has been written in a different language from C before (not suffering obvious plagues like, null pointer dereference or buffer overflow):
http://en.wikipedia.org/wiki/Lisp_machine
Multi-core friendly, high performance code can be generated from a domain friendly custom language:
http://nv.github.io/nile/demo_shape.html
And let's not forget the newly budding, full-stack language, which can bring these concepts under one, common hood:
http://www.red-lang.org/
(which is kind of following the principles of low fat computing: http://www.ultratechnology.com/lowfat.htm)
I'm aware of http://www.jwz.org/doc/worse-is-better.html but does it really have to be that way?
bd6941c@Amelek
I would just like to point at that you have no public repositories so I cant review your code at all. Perhaps you could submit a pull request renaming some variables so its more to your liking.
bd6941c@andrewchambers
Doing full pull request seem like a waste of time to me and I don't really feel like contributing to that project now.
Regardless, there is refactored version of tls1_process_heartbeat
https://github.com/Amelek/RandomUploads/blob/master/openSSL/openssl_refactored.patch
Is it better? No - it's not reviewed and thus can't be used anywhere. There may be new bugs introduced in it.
Observation while doing that patch: OpenSSL's fixed code:
bd6941c@Amelek I like the renamed variables. Truly while "hbtype" makes sense to you while you are writing the code, "heartbeat_type" is just so much more user friendly, and makes it for a much easier time when you or someone looks at the code later. And so I approve this message.
bd6941cFirst person to find another critical bug with OpenSSL wins!
bd6941cCan someone point out exactly where the actual bug is? I'm curious.
bd6941cIt was simultaneously discovered after 2 long years by two independent groups? Really? That's so improbable. Anyone that's been in US government has heard the phrase, "Cover your ass". Sounds like one group knew for a while and only admitted it after an independent group reported it. Or were they using the same test suite? Seems like with that much money and safety at stake it shouldn't be criminals that discover and exploit these things first as has been confirmed in at least two places. They need to cut back on the coffee & cigarettes.
I'm not going to pretend I know whether or not this code is good or not - I just know that I avoided pointers when possible; something it seems would be near impossible to do in kernel code but very desirable when writing something like openSSL.
bd6941c@BarneysBombers I believe it was far more likely that someone, somewhere was either suspicious or actually new there was an issue with OpenSSL can somehow various teams were pointed in the right direction looking for holes. My guess is that there were investigations around the various Bitcoin exchanges getting plundered even when people said they had extremely secure passwords (but no 2FA). This is separate from any other Bitcoin related issues that resulted in losses although one can never rule out exchanges that had admin accounts publicly accessible and compromised due to the SSL issue. Of course some other high value targets like government or banking entities could also have gathered evidence that SSL was being compromised in a way they could not identify. If you had gathered enough reports you could correlate OpenSSL version to those reports and narrow you focus...
bd6941cWell, security isn't something that should be strictly outsourced. There needs to be competitive and competent security teams investigating these things internal to the government and big businesses as well. It's not like these organizations can't afford it. And it's not like if they couldn't afford it the government wouldn't print the money so that they could if they so desired, a bit like they all do with their defense budgets.
I find it a bit unreasonable to believe that the Russian, Chinese, Ukrainian, American, and various other security agencies didn't know about this flaw, almost from the get go. Indeed with the news about the NSA and the behavior of these governments it wouldn't be unreasonable to think this at all.
We do not need a situation in security such as what caused financial markets to collapse all over the world a few years back; where the 'independent regulatory and ratings outsiders, the experts' were almost as irresponsible as the financial firms and traders. Well, it's already been exposed it wasn't so much irresponsibility as corruption, regardless that I spend more time in jail as a 19 year old forgetting I had a traffic ticket than any of the perpetrators of those abuses.