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: tls connection broken in 1.5 #12957

Closed
leo-stone opened this issue Oct 16, 2015 · 18 comments
Closed

crypto/tls: tls connection broken in 1.5 #12957

leo-stone opened this issue Oct 16, 2015 · 18 comments
Milestone

Comments

@leo-stone
Copy link

@leo-stone leo-stone commented Oct 16, 2015

Hello,
it seems "crypto/tls" is broken since GO 1.5. (Windows)
Here you find the code to reproduce the problem.

I could not test on any OS other than Windows.
Confirmed on Windows, Linux, OS X (tx @bradfitz )

@leo-stone leo-stone changed the title TLS broken in 1.5 crypto/tls: tls connection broken in 1.5 Oct 16, 2015
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 16, 2015

Can you write a test case that does not require the heroku package and does not require contacting other servers? There isn't enough information here to know where the problem is.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Oct 16, 2015
@leo-stone

This comment has been minimized.

Copy link
Author

@leo-stone leo-stone commented Oct 16, 2015

I did what I could, to provide you with a setup to investigate the issue.
Unfortunatly i have no more resources than that.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 16, 2015

@ianlancetaylor, the referenced program at least contains the necessary credentials to run it easily.

I can confirm the issue on OS X too, so it's not Windows specific.

With Go 1.4:

$ go run x.go
2015/10/16 07:37:26 rendezvous.runtime.heroku.com:5000
2015/10/16 07:37:26 /81ba7f46d09156cc43d642e03f249157f4ad1e488583903b130ab5b1e97cb464
2015/10/16 07:37:27 Sending secret: 81ba7f46d09156cc43d642e03f249157f4ad1e488583903b130ab5b1e97cb464
2015/10/16 07:37:27 Bytes sent:  64
2015/10/16 07:37:27 12 <nil>
2015/10/16 07:37:27 rendezvous

2015/10/16 07:37:30 7 <nil>
2015/10/16 07:37:30 hello

2015/10/16 07:37:30 0 EOF
2015/10/16 07:37:30 

With Go 1.5:

$ go run x.go
2015/10/16 07:38:07 rendezvous.runtime.heroku.com:5000
2015/10/16 07:38:07 /e6ac3371bd9e97be4ef5db06bd4b82712a89264b7198ee0619eca06b306fb18e
2015/10/16 07:38:07 Sending secret: e6ac3371bd9e97be4ef5db06bd4b82712a89264b7198ee0619eca06b306fb18e
2015/10/16 07:38:07 Bytes sent:  64
(hang)

Ignore the difference in the hex string. That changes randomly per run regardless of Go version.

I haven't looked into this more than just reproducing it.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 16, 2015

Perhaps @bgentry can help debug, as the author of that Heroku package.

@bgentry

This comment has been minimized.

Copy link
Contributor

@bgentry bgentry commented Oct 16, 2015

@bradfitz I suspect this could have something to do with the specific version of OpenSSL running on Heroku's rendezvous component. I no longer work there, but maybe @fabiokung, @dpiddy, or @freeformz would be able to debug further.

The heroku-go package is just using http.Client under the hood to make an API call that spawns a "dyno" (container) on Heroku. That part is really irrelevant, except that it returns a URL that needs to be used in the connection to rendezvous.

You can see a complete example of how this is done in the hk client, which uses heroku-go. So anybody with a Heroku account should be able to reproduce this issue using hk run -a myappname bash on Windows (which I haven't tried).

Separately, @leo-stone you are going to want to rotate your API key as the one you included in your repo is now compromised, and anybody can use it to modify your Heroku apps and/or rack up charges on your account.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 16, 2015

@bgentry, if the official hk client uses Go, I would suspect people at Heroku have already identified this problem? I'm a little surprised we haven't already heard about it. Can you recommend any contacts there to talk about this?

@freeformz

This comment has been minimized.

Copy link
Contributor

@freeformz freeformz commented Oct 16, 2015

@dickeyxxx owns the heroku CLI tools, which include hk AFAIK.

@bgentry

This comment has been minimized.

Copy link
Contributor

@bgentry bgentry commented Oct 16, 2015

hk is more or less unmaintained at this point, so wouldn't
necessarily count on this having been encountered before.

On Friday, October 16, 2015, Brad Fitzpatrick notifications@github.com
wrote:

@bgentry https://github.com/bgentry, if the official hk client uses Go,
I would suspect people at Heroku have already identified this problem? I'm
a little surprised we haven't already heard about it. Can you recommend any
contacts there to talk about this?


Reply to this email directly or view it on GitHub
#12957 (comment).

@jdxcode

This comment has been minimized.

Copy link

@jdxcode jdxcode commented Oct 16, 2015

To be clear, it is not maintained. We need to update the project to reflect this.

@bgentry

This comment has been minimized.

Copy link
Contributor

@bgentry bgentry commented Oct 16, 2015

regardless of hk's status, this is probably bad interaction between Go's crypto/tls on Windows and whatever Heroku's rendezvous component is using for TLS. hk run should at least be a reliable way to reproduce the issue.

@dickeyxxx if you could raise this w/ folks on Heroku's runtime team to help track down the issue that would be 👍

@jdxcode

This comment has been minimized.

Copy link

@jdxcode jdxcode commented Oct 16, 2015

will do! thanks @bgentry!

@leo-stone

This comment has been minimized.

Copy link
Author

@leo-stone leo-stone commented Oct 18, 2015

I got around to confirm that this issue also exists on Linux-64bit.

@leo-stone

This comment has been minimized.

Copy link
Author

@leo-stone leo-stone commented Feb 20, 2016

Issue still existent in go 1.6.0 :(

@danp

This comment has been minimized.

Copy link
Contributor

@danp danp commented Feb 21, 2016

Ah! I had forgotten about this. We have an internal rendezvous client written in Go and it works, but it took a tweak to get going.

Here's a standalone client that works:

https://gist.github.com/dpiddy/5062801f553c7a4d53a0

I think the necessary trick is here to ensure the token goes in one write. Credit to @fabiokung for this.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 21, 2016

@dpiddy, can you explain that code at all? The bug only affects < TLS1.0? What is a version of that code which fails? It looks like the token was already in one write before anyway. Is that the issue, or is the prepended null byte the issue? The comment in the code doesn't really help.

/cc @agl in case it's obvious to him

@danp

This comment has been minimized.

Copy link
Contributor

@danp danp commented Feb 21, 2016

I think it's ultimately the rendezvous service's fault, it doesn't buffer to read the secret. So for that to work it has to come in one read (TLS record?). It also uses TLSv1.0 currently which led me to this which is what I would guess is the issue.

This standalone program shows what rendezvous sees:

https://gist.github.com/dpiddy/cb646c925ff5c645803f

The output is:

% go run main.go
2016/02/21 10:11:12 write n=6
2016/02/21 10:11:12 read n=1 b="h"

If I remove or up MaxVersion it behaves differently:

# comment out MaxVersion setting
% go run main.go
2016/02/21 10:12:34 write n=6
2016/02/21 10:12:34 read n=6 b="hello\n"

But, again, it seems like all this is expected for a TLSv1.0 server and we should get rendezvous up to TLSv1.2 and/or buffer to read the secret.

@fabiokung

This comment has been minimized.

Copy link

@fabiokung fabiokung commented Feb 22, 2016

+1 to what @dpiddy said. This doesn't seem to be a problem on crypto/tls. Rather, it is Heroku's rendezvous service making bad (wrong) assumptions about data boundaries. It expects the 64 bytes token to come in a single TLS record, which is a bug.

Go's crypto/tls breaks up the first write for SSLv3/TLSv1 in two records, as a BEAST mitigation. Heroku's rendezvous service shouldn't care about how many records data has been split into and should properly detect boundaries of the 64 bytes token.

I think it is safe to close this.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 22, 2016

Thanks for researching. Will close.

@bradfitz bradfitz closed this Feb 22, 2016
@golang golang locked and limited conversation to collaborators Jun 1, 2017
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
9 participants
You can’t perform that action at this time.