Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/tls: no support for SSLv2 handshake #3930

Closed
gopherbot opened this issue Aug 9, 2012 · 42 comments
Closed

crypto/tls: no support for SSLv2 handshake #3930

gopherbot opened this issue Aug 9, 2012 · 42 comments
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 9, 2012

by ian.ragsdale:

To reproduce, run the attached TLS echo server and try to connect using the openssl
s_client utility:

> openssl s_client -quiet -connect localhost:50000                                   
                                                      
95052:error:140773F2:SSL routines:SSL23_GET_SERVER_HELLO:sslv3 alert unexpected
message:/SourceCache/OpenSSL098/OpenSSL098-47/src/ssl/s23_clnt.c:602:
> openssl s_client -quiet -connect localhost:50000 -ssl3                             
                                                      
depth=0
/CN=relay.cloudsmtp.com/O=Datran/OU=CapThought/ST=Texas/C=US/L=Austin/emailAddress=ian.ragsdale@gmail.com
verify error:num=20:unable to get local issuer certificate
verify return:1
depth=0
/CN=relay.cloudsmtp.com/O=Datran/OU=CapThought/ST=Texas/C=US/L=Austin/emailAddress=ian.ragsdale@gmail.com
verify error:num=21:unable to verify the first certificate
verify return:1
hi 
>> hi

Note that the s_client is unable to connect with its default settings, but can connect
by specifying a version.

This is because TLS-capable clients that also support connecting to SSLv2 servers start
off by sending a SSLv2 formatted CLIENT-HELLO packet instead of a TLS ClientHello.  The
crypto/tls package does not recognize these packets and so terminates the handshake,
making it impossible to use these clients. However, these ARE compliant TLS clients, and
they set the TLS version correctly in the CLIENT-HELLO packet in order to signify that
they support TLS.  This makes it possible for the crypto/tls code to read these packets,
recognize TLS compliance, and send back a TLS-compliant ServerHello packet, at which
point the entire rest of the conversation is exactly like any other TLS session.

So, in order to support these clients, the only thing that needs to be done is to
recognize a SSLv2 compliant packet correctly and begin a standard TLS conversation. 
Since it seemed fairly simple to do so, I've attached a fairly simple patch to allow
communicating with these clients.  I'm sure it's unacceptable in its current form, but
it functions for me and illustrates the changes that are needed to support this
functionality.

I completely understand (and agree with) the aversion to supporting older versions of
SSL, but this is actually TLS behavior documented in at least the 1.0 and 1.1 RFCs, and
is a fairly minor change, which should greatly improve interop.  Lots of common clients
(recent versions of Java and Ruby at least) do this by default. I can easily configure
my own clients to not do this, but it isn't easy for a lot of people.

Attachments:

  1. tls_echo.go (1002 bytes)
  2. 0001-Allow-SSLv2-compatible-client-hello-so-SSLv2-compati.patch (5479 bytes)
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Aug 9, 2012

Comment 1:

-> agl for triage.
If agl thinks it is worth doing please send us the patches using the code review process
described at http://golang.org/doc/contribute.html.
Thanks.

Owner changed to @agl.

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Aug 9, 2012

Comment 2 by ian.ragsdale:

Will do as soon as I get the nod from agl
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Aug 9, 2012

Comment 3:

This has previously been a good method for educating people not to use crypto/tls for
serious things. Since the whole crypto stack, from ModExp upwards, is basically
unreviewed, I want to encourage people to put Go behind OpenSSL or something similar
once they get to the point of having client diversity like this.
So I would actually like to not support SSLv2 compat handshakes; not just because it
highlights poor clients (OpenSSL fixed this behaviour quite a while ago), but because of
that more subtle reason.

Labels changed: added priority-later, removed priority-triage.

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Aug 9, 2012

Comment 4 by ian.ragsdale:

I see your point, but I can't easily put it behind OpenSSL - I'm working on a SMTP
server that should support STARTTLS, and this makes most of the clients that I've tried
with not work out of the box.  I'd like this service to be usable by as many as possible
without complicated SSL configuration.
I'm not aware of any existing SMTP proxy that I can use like Nginx or HAProxy to be a
SMTP front-end, only store & forward type SMTP servers, which I'd rather not use.
I understand that the TLS implementation isn't as well-reviewed as OpenSSL, but what
incentive do people have to do that as long as the answer is always to put it behind
OpenSSL?
Anyway, I understand if the answer is no, it's a limited use case that will eventually
be a non-issue, it just seemed worthwhile to improve interop in the short term.  I can
just keep on using my patched version. :)
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Aug 9, 2012

Comment 5:

You make a fair point about SMTP: you don't have a lot of options there and
SMTP/STARTTLS clients are a real mess because nobody cares much about them.
I am on the fence about this and could be persuaded if it was causing a bunch of people
pain. But it sounds like it's not a big problem for you, and having less code is nice.
So far this has caused pleasingly little trouble for people, at least judging by the
reports that I've seen, so I'm tempted to keep things as they are. But we can leave this
bug open in case there are others who want this and it turns out that I'm misjudging the
cost/benefit.
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Aug 9, 2012

Comment 6 by ian.ragsdale:

Makes sense to me.  I just want to add that the error returned from the TLS package is
"local error: unexpected message", and the OpenSSL error happens in
SSL23_GET_SERVER_HELLO, so that if someone else runs into this error they might
hopefully find this page and the patch.
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Aug 21, 2012

Comment 7 by vadim.spivak:

I just hit the same issue after debugging it for quiet some time..
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Aug 23, 2012

Comment 8:

vadim: whatever the merits of the bug, having people waste time debugging isn't good.
I've sent out a change for review to return a more specific error in this case.
http://golang.org/cl/6474055
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Aug 23, 2012

Comment 9:

This issue was updated by revision 0a115d7.

Return a better error message in this situation.
R=golang-dev, r
CC=golang-dev
http://golang.org/cl/6474055
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Sep 6, 2012

Comment 10 by vspivak@rbcon.com:

I tried implementing an OpenSSL wrapper for GO - https://github.com/vadims/sslconn
However, it's not very pretty due to the non blocking IO needed to multiplex OpenSSL
connections and Go's blocking Readers/Writers. 
Is there any way you could reconsider merging Ian's patch?
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Sep 6, 2012

Comment 11 by ian.ragsdale:

The reason given for not merging my patch is that they don't consider the existing TLS
code production ready, not because of any known defects, but because it hasn't received
the kind of code review warranted for a cryptographic library.
A more interesting question (and perhaps a bug for this one to depend on) is what level
of review WOULD be sufficient for this code to be considered production ready. 
Identifying that as a need could be the start of a good project for someone, perhaps a
summer of code project, or something that could be sponsored by one of the companies out
there using Go in production.
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Sep 6, 2012

Comment 12 by vspivak@rbcon.com:

The problem is that developers who use crypto/tls don't know that it's not production
ready, specifically on the client side where compatibility is less important. 
It's only apparent when you run into these corner cases.
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Sep 12, 2012

Comment 13:

Labels changed: added priority-someday, removed priority-later.

Status changed to LongTerm.

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Dec 10, 2012

Comment 14 by justinjosephsmith:

Any update here? It would be great to see some sort of list that indicates what has gone
through a security review. After reading this thread I'm not sure how much of "crypto"
has been reviewed for use "in a real app". In lieu of more information, the only viable
option appears to be relying on OpenSSL for all crypto.
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Dec 10, 2012

Comment 15:

justinjosephsmith: the quality of the crypto code is on a par with the rest of the Go
standard library. The issue with crypto is that there's more scope for subtle mistakes
to be a real problem.
I've received one (non-security) bug from someone competent who reviewed the code, but
it certainly hasn't undergone the kind of review that OpenSSL has. (That makes it the
same as the vast majority of crypto code: GnuTLS, GnuPG etc.)
On the other hand, the security advisories for OpenSSL are fairly numerous
http://openssl.org/news/ and there are more in the pipeline.
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Dec 10, 2012

Comment 16 by ian.ragsdale:

It seems like we have a bit of a chicken/egg problem.  If we discourage the use of the
crypto code there isn't much incentive to review it. 
Perhaps it makes sense to allow the functionality but have a big warning about using it?
It could require a specific configuration option, like the one for disabling certificate
checks in the client.
tls.Config{InsecureAllowV2Clients: true}
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Dec 11, 2012

Comment 17 by justinjosephsmith:

This makes sense. I misunderstood an earlier statement. Thanks for the response.
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Oct 10, 2013

Comment 18 by tomfite:

Thanks for posting the patch, I found this thread after trying to track down SSLv2
errors with a Go server application that I'm working on.
I've attempted to merge the patch into Go v1.1.1 and I'm not having much luck as the
crypto/tls library has changed quite a bit since this patch was written. By chance has
anyone attempted to make this patch work with newer versions of the library?
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Oct 28, 2013

Comment 19 by erwan.legrand:

Here is a new version of the patch for the Go 1.1 branch.
Before you use this, please read Adam Langley's comments above: the Go TLS stack is not
ready for production. For all we know, it might leak your private key. You have been
warned.

Attachments:

  1. Allow-SSLv2-ClientHello-go1.1.patch (5309 bytes)
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 20:

Labels changed: added repo-main.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Mar 3, 2014

Comment 21:

Adding Release=None to all Priority=Someday bugs.

Labels changed: added release-none.

@ian-kent

This comment has been minimized.

Copy link
Contributor

@ian-kent ian-kent commented Dec 27, 2014

I have the same problem, also SMTP related - I'd rather avoid manually patching Go source (since it would mean other developers doing the same)

If the concern is discouraging the use of crypto/tls, could the patch be modified so it has a feature flag (disabled by default), forcing developers to read the documentation (or code) which could explain the risks. I think that would be safer than expecting developers to patch the Go source, which may be a necessity for some.

If crypto/tls really isn't "production ready", the documentation should make it much clearer - I only found this bug after trying to connect to the Go server using openssl s_client!

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 29, 2014

Considering that SSLv3 is broken (https://disablessl3.com/, http://googleonlinesecurity.blogspot.com/2014/10/this-poodle-bites-exploiting-ssl-30.html) nobody should be using SSLv3 these days, much less SSLv2.

I'm tempted to close this bug without action. @agl?

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Dec 29, 2014

bradfitz: this is about ancient clients that wrap an SSLv3/TLS handshake in SSLv2 form in order to be compatible with SSLv2 servers. IE on XP was a big source of this and we've made that mostly moot by disabling SSLv3 by default on trunk. You can also get the same thing with s_client if you don't update OpenSSL, which is similarly a bad idea.

Since this deprecation is so old (1996) that it can vote now, I think it's ok that it's a little bit of work to support it.

@elimisteve

This comment has been minimized.

Copy link
Contributor

@elimisteve elimisteve commented Jan 14, 2015

I've tweaked erwan.legrand's Go 1.1 patch to support Go 1.4. I applied it to my std lib, and the dreaded failed requests and tls: unsupported SSLv2 handshake received error messages appear to be gone! (See below.)

Having a 95% solution in Go was painful, and after trying many other options (e.g., re-implementing much of my proxy app in another language with a more mature TLS library, using a not-programmable-enough Python proxy, and almost starting from scratch and using Nginx + Lua), this finally appears to be a fully working solution -- and 100% in Go!

To reiterate previous security warnings:

Before you use this, please read Adam Langley's comments above: the Go TLS stack is not ready for production. For all we know, it might leak your private key. You have been
warned.

To download and use this patch, then recompile the Go standard library on Linux, run something like...

cd ~
wget https://gist.githubusercontent.com/elimisteve/e3fea232c11fa08146d6/raw/e2457232b63a8f0433326874621565475cdb2524/Allow-SSLv2-ClientHello-go1.4.patch
cd go/src  # cd to wherever your Go source is located
patch -p0 < ~/Allow-SSLv2-ClientHello-go1.4.patch
./all.bash  # recompile

Thank you, all!

@elimisteve

This comment has been minimized.

Copy link
Contributor

@elimisteve elimisteve commented Jan 14, 2015

For some reason I kept accidently using an old Go runtime after recompiling. If you've used the above patch and recompiled, and you want to make sure your go binary is patched, you can run

grep 'tls: unsupported SSLv2 handshake received' `which go`

to see if this SSLv2 error from the standard library, which should NOT be in your patched go, is present nonetheless. And if you run go build myprogram.go, afterward you can run grep 'tls: unsupported SSLv2 handshake received' myprogram to make sure that the Go runtime that has been linked into your myprogram binary is the patched one.

@athornton

This comment has been minimized.

Copy link

@athornton athornton commented Feb 2, 2015

I just got bitten by this one.

The problem (at least for me) is less ancient clients that actually support SSLv2, and more: the Java 6 default behavior, when making an SSL connection, is to send SSLv2 Hello (it doesn't actually support SSLv2, but does send the Hello--see http://www.oracle.com/technetwork/java/javase/documentation/cve-2014-3566-2342133.html ).

Now, sure, Java 6 is also pretty old, but it's in really wide use in the wild, especially in the context of big horrible Enterprisey apps (which is where this got me). And in many cases it's not going to be feasible to modify the Java properties of the connecting applications to only do TLS.

Six months ago I would have had more sympathy for "Go's SSL implementation is not production-ready." These days I'm pretty sure OpenSSL's SSL implementation isn't production-ready either.

@bradfitz bradfitz changed the title crypto/tls: no support for SSLv2 crypto/tls: no support for SSLv2 handshake Feb 3, 2015
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
agl added a commit that referenced this issue May 11, 2015
…ase of an SSLv2 handshake.

««« backport 8048fe8f6f4b
crypto/tls: return better error message in the case of an SSLv2 handshake.

Update #3930
Return a better error message in this situation.

R=golang-dev, r
CC=golang-dev
https://golang.org/cl/6474055

»»»
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Dec 8, 2015

I rebased on 1.5.2/fixed/polished/optimized that patch a bit and successfully ran it at pretty high load. It's here: crypto/tls: support SSLv2 compatibility handshakes. I wouldn't use it if I were you, but if you're desperate, it might not blow up.

@nathany

This comment has been minimized.

Copy link
Contributor

@nathany nathany commented Mar 1, 2016

I'm tempted to close this bug without action. @agl?

That sounds like a good idea.
https://drownattack.com/

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 1, 2016

Hah, indeed. I've wanted to close this bug for a long time, but that's a great excuse.

@bradfitz bradfitz closed this Mar 1, 2016
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Mar 1, 2016

I'm not against closing this, but DROWN is not relevant.

This ticket is about supporting an unfortunate compatibility trick, not the entire protocol, so it would not introduce the vulnerability.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 1, 2016

My thought process was that if people finally kill off SSLv2, this compatibility feature is no longer relevant.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Mar 1, 2016

@cespare

This comment has been minimized.

Copy link
Contributor

@cespare cespare commented Mar 1, 2016

If more anecdata helps, I ran into this within the last couple of months with a third-party service doing HTTPS requests to our servers from Java 6 code (as mentioned elsewhere in this thread, the Java 6 SSL stack will do an SSLv2 Client Hello by default, although it is possible to work around explicitly).

@elimisteve

This comment has been minimized.

Copy link
Contributor

@elimisteve elimisteve commented Mar 1, 2016

I, too, ran into this SSLv2 issue from a Java client, which was trying to connecting to an HTTPS proxy I wrote in Go.

@athornton

This comment has been minimized.

Copy link

@athornton athornton commented Mar 1, 2016

Yeah. As far as I know, the primary place this shows up is The Very Important Service That Only Runs On Java 6 And No One Understands It Enough To Rewrite It And There Is No Way We Are Going To Crack It Open To Change The SSL Settings Are You Kidding? Which, on my more cynical days, I think describes about 90% of Enterprise Software.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 1, 2016

Okay, I can reopen this. I was being too hopeful.

@FiloSottile, the tree is open. Can you formally send https://gist.github.com/FiloSottile/8e0fe1a3523126e8c581? It doesn't look too invasive. We can guard it behind a flag (make it opt-in) for more paranoia.

@bradfitz bradfitz reopened this Mar 1, 2016
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Mar 2, 2016

Done https://go-review.googlesource.com/20094

Considering that ian.ragsdale, erwan.legrand and @elimisteve contributed to earlier versions of the patch, should we chase their CLAs?

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Mar 2, 2016

CL https://golang.org/cl/20094 mentions this issue.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Mar 2, 2016

I looked at the sequence of patches. The one in the Gist has only trivial changes from the original by Ian Ragsdale, so I think the only CLA we absolutely need is his.

@iragsdale, could you complete a contributor CLA (https://golang.org/doc/contribute#copyright) so that we can put the SSLv2 compatibility code into Go's standard crypto/tls?

If @ErwanLegrand and @elimisteve want to complete CLAs as well I'm happy to credit them too, but the followup changes appear to have been quite minor.

Thanks everyone.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 2, 2016

@agl replied on https://golang.org/cl/20094 :

I don't believe that we should land this. It goes against the general trend of dropping support for old SSL versions which keep causing problems.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Mar 2, 2016

I am glad that we have established that Java 6 is the holdout. I looked around on the internet and found what appears to be Oracle's official advice on SSL settings as of January 2015.

Under the text "For the following use cases Oracle recommends", it says that client-side uses should set the protocols to exclude the SSLv2 hello, for example:

sslSocket.setEnabledProtocols(new String[] {"TLSv1"});
sslEngine.setEnabledProtocols(new String[] {"TLSv1"});

So the Java clients that are still sending SSLv2Hello are not following Oracle's advice.

Based on @agl's Go server-side advice and Oracle's Java client-side advice, I think it is reasonable for Go not to support SSLv2 compatibility handshakes for Java clients.

@iragsdale, @ErwanLegrand, @elimisteve, sorry for the false alarm; no CLA needed after all.

Thanks everyone.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants
You can’t perform that action at this time.