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: client not always able to connect when using NPN #4088

Closed
gopherbot opened this issue Sep 16, 2012 · 3 comments
Closed

crypto/tls: client not always able to connect when using NPN #4088

gopherbot opened this issue Sep 16, 2012 · 3 comments
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 16, 2012

by tfh@skip.org:

The crypto/tls client is unable to make connections when:

- NPN is being used, and
- NPN is not the last extension received in the ServerHello message.

During umarshalling of ServerHello messages (in  crypto/tls/handshake_messages.go), too
many bytes are read while unmarshalling NPN extensions. It reads upto the end of the
ServerHello, instead of the end of the extension; so any extensions after NPN are
accidentally interpreted as part of the NPN extension instead.

What steps will reproduce the problem?
1. Run the attached test.go. It will attempt two connections to https://skip.org:4443 ,
which has reordered ServerHello extensions to show the problem (NPN is not the last
extension, ServerName and others come after NPN).

What is the expected output?
The two connections should succeed.

What do you see instead?
The connection using NPN fails.

I have attached a patch which fixes the problem.


Tom

Attachments:

  1. test.go (736 bytes)
  2. npn-unmarsal.patch (410 bytes)
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Sep 17, 2012

Comment 1:

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

Status changed to Accepted.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 6, 2012

Comment 2:

Labels changed: added go1.1.

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Oct 9, 2012

Comment 3:

This issue was closed by revision 7e90f7b.

Status changed to Fixed.

@gopherbot gopherbot added fixed labels Oct 9, 2012
@rsc rsc added this to the Go1.1 milestone Apr 14, 2015
@rsc rsc removed the go1.1 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
I typoed the code and tried to parse all the way to the end of the
message. Therefore it fails when NPN is not the last extension in the
ServerHello.

Fixes golang#4088.

R=golang-dev, rsc
CC=golang-dev
https://golang.org/cl/6637052
This issue was closed.
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
3 participants
You can’t perform that action at this time.