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: server should send empty server_name extension when using SNI #16072

Closed
magiconair opened this issue Jun 15, 2016 · 4 comments
Closed
Assignees
Milestone

Comments

@magiconair
Copy link
Contributor

@magiconair magiconair commented Jun 15, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
go version go1.6.2 darwin/amd64
  1. What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/frschroeder/gopath"
GORACE=""
GOROOT="/Users/frschroeder/go1.6.2"
GOTOOLDIR="/Users/frschroeder/go1.6.2/pkg/tool/darwin_amd64"
GO15VENDOREXPERIMENT="1"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fno-common"
CXX="clang++"
CGO_ENABLED="1"
  1. What did you do?

Sample app: https://play.golang.org/p/Tk9CR4BUyU

a) Save certPEM from the sample app into a file, e.g. cert.pem
b) Start WireShark and capture traffic on lo or lo0
c) Set a filter tcp.port == 4433
d) Goto Preferences -> Protocols -> SSL and add the cert.pem to the RSA key list for 127.0.0.1:4433 protocol tcp

Run the sample program from as follows:

1st terminal: go run main.go -server
2nd terminal: go run main.go

This will start a server in the first terminal and a client in the second terminal which executes a SNI handshake.

WireShark will capture and decrypt the traffic

  1. What did you expect to see?

A Server Hello with an empty server_name extension, e.g.

Secure Sockets Layer
    TLSv1.2 Record Layer: Handshake Protocol: Server Hello
        Content Type: Handshake (22)
        Version: TLS 1.2 (0x0303)
        Length: 53
        Handshake Protocol: Server Hello
            Handshake Type: Server Hello (2)
            Length: 49
            Version: TLS 1.2 (0x0303)
            Random
            Session ID Length: 0
            Cipher Suite: TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f)
            Compression Method: null (0)
            Extensions Length: 9
            Extension: server_name
                Type: server_name (0x0000)
                Length: 0
            Extension: renegotiation_info
                Type: renegotiation_info (0xff01)
                Length: 1
                Renegotiation Info extension
                    Renegotiation info extension length: 0
  1. What did you see instead?
Secure Sockets Layer
    TLSv1.2 Record Layer: Handshake Protocol: Server Hello
        Content Type: Handshake (22)
        Version: TLS 1.2 (0x0303)
        Length: 49
        Handshake Protocol: Server Hello
            Handshake Type: Server Hello (2)
            Length: 45
            Version: TLS 1.2 (0x0303)
            Random
            Session ID Length: 0
            Cipher Suite: TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f)
            Compression Method: null (0)
            Extensions Length: 5
            Extension: renegotiation_info
                Type: renegotiation_info (0xff01)
                Length: 1
                Renegotiation Info extension
                    Renegotiation info extension length: 0

The extension section of the ServerHello does not contain a server_name extension.

When you make a request with an SNI header the client sends a client hello with a server name extension. RFC 6606 Section 3 states:

   A server that receives a client hello containing the "server_name"
   extension MAY use the information contained in the extension to guide
   its selection of an appropriate certificate to return to the client,
   and/or other aspects of security policy.  In this event, the server
   SHALL include an extension of type "server_name" in the (extended)
   server hello.  The "extension_data" field of this extension SHALL be
   empty.

We found this when debugging a TLS connection from the AWS API GW to a go server instance.

I am not sure whether this is a real issue or something that can be ignored. In any case it seems to be mentioned in the RFC and I couldn't find any mention in the issues. So even if it doesn't get addressed people can still find out why.

I realize that you prefer discussion first over code but the following patch added the server_name extension:

diff --git a/src/crypto/tls/handshake_messages.go b/src/crypto/tls/handshake_messages.go
index 111ce53..34f8eaa 100644
--- a/src/crypto/tls/handshake_messages.go
+++ b/src/crypto/tls/handshake_messages.go
@@ -490,6 +490,7 @@ type serverHelloMsg struct {
    cipherSuite         uint16
    compressionMethod   uint8
    nextProtoNeg        bool
+   serverName          bool
    nextProtos          []string
    ocspStapling        bool
    scts                [][]byte
@@ -545,6 +546,9 @@ func (m *serverHelloMsg) marshal() []byte {
        nextProtoLen += len(m.nextProtos)
        extensionsLength += nextProtoLen
    }
+   if m.serverName {
+       numExtensions++
+   }
    if m.ocspStapling {
        numExtensions++
    }
@@ -614,6 +618,11 @@ func (m *serverHelloMsg) marshal() []byte {
            z = z[1+l:]
        }
    }
+   if m.serverName {
+       z[0] = byte(extensionServerName >> 8)
+       z[1] = byte(extensionServerName & 0xff)
+       z = z[4:]
+   }
    if m.ocspStapling {
        z[0] = byte(extensionStatusRequest >> 8)
        z[1] = byte(extensionStatusRequest)
diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go
index e16cddc..66a9c02 100644
--- a/src/crypto/tls/handshake_server.go
+++ b/src/crypto/tls/handshake_server.go
@@ -122,6 +122,7 @@ func (hs *serverHandshakeState) readClientHello() (isResume bool, err error) {
    c.haveVers = true

    hs.hello = new(serverHelloMsg)
+   hs.hello.serverName = len(hs.clientHello.serverName) > 0

    supportedCurve := false
    preferredCurves := config.curvePreferences()
@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone Jun 15, 2016
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 15, 2016

CC @agl

@agl agl self-assigned this Jun 15, 2016
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Jun 15, 2016

I've never understood why the RFC says that and crypto/tls may not always know whether a callback used the SNI name or not. So the only reasonable answers are to never echo the extension or to always echo it. We currently do the former. Your change would lead to the latter. I welcome any suggestions about advantages!

@magiconair

This comment has been minimized.

Copy link
Contributor Author

@magiconair magiconair commented Jun 16, 2016

Do other implementations depend on that behavior because it is in the RFC? I realize that the RFC says SHALL and not MUST.

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Aug 17, 2016

There's no reported problems arising from this I believe. OpenSSL, for what it's worth, allows a callback to decide whether to echo this. Since we don't have that and there's no concrete motivation, I don't see impetus to change our behaviour here.

@agl agl closed this Aug 17, 2016
@golang golang locked and limited conversation to collaborators Aug 17, 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
4 participants
You can’t perform that action at this time.