crypto/tls: fix renegotiation extension.

There are two methods by which TLS clients signal the renegotiation
extension: either a special cipher suite value or a TLS extension.

It appears that I left debugging code in when I landed support for the
extension because there's a "+ 1" in the switch statement that shouldn't
be there.

The effect of this is very small, but it will break Firefox if
security.ssl.require_safe_negotiation is enabled in about:config.
(Although almost nobody does this.)

This change fixes the original bug and adds a test. Sadly the test is a
little complex because there's no OpenSSL s_client option that mirrors
that behaviour of require_safe_negotiation.

Change-Id: Ia6925c7d9bbc0713e7104228a57d2d61d537c07a
Reviewed-on: https://go-review.googlesource.com/1900
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/crypto/tls/handshake_messages.go b/src/crypto/tls/handshake_messages.go
index 5d14871a348e6ece28a1bea5296e6766d3474527..95325088cf5cbcdd1c9038757242df4056c77007 100644 (file)
--- a/src/crypto/tls/handshake_messages.go
+++ b/src/crypto/tls/handshake_messages.go
@@ -430,7 +430,7 @@ func (m *clientHelloMsg) unmarshal(data []byte) bool {
                                m.signatureAndHashes[i].signature = d[1]
                                d = d[2:]
                        }
-               case extensionRenegotiationInfo + 1:
+               case extensionRenegotiationInfo:
                        if length != 1 || data[0] != 0 {
                                return false
                        }

diff --git a/src/crypto/tls/handshake_server_test.go b/src/crypto/tls/handshake_server_test.go
index f9545461a4e669befb71ded83cc396dc9693e274..62051160120e161788b4a59c2564133921f94891 100644 (file)
--- a/src/crypto/tls/handshake_server_test.go
+++ b/src/crypto/tls/handshake_server_test.go
@@ -103,6 +103,54 @@ func TestNoCompressionOverlap(t *testing.T) {
        testClientHelloFailure(t, clientHello, "client does not support uncompressed connections")
 }
 
+func TestRenegotiationExtension(t *testing.T) {
+       clientHello := &clientHelloMsg{
+               vers:                VersionTLS12,
+               compressionMethods:  []uint8{compressionNone},
+               random:              make([]byte, 32),
+               secureRenegotiation: true,
+               cipherSuites:        []uint16{TLS_RSA_WITH_RC4_128_SHA},
+       }
+
+       var buf []byte
+       c, s := net.Pipe()
+
+       go func() {
+               cli := Client(c, testConfig)
+               cli.vers = clientHello.vers
+               cli.writeRecord(recordTypeHandshake, clientHello.marshal())
+
+               buf = make([]byte, 1024)
+               n, err := c.Read(buf)
+               if err != nil {
+                       t.Fatalf("Server read returned error: %s", err)
+               }
+               buf = buf[:n]
+               c.Close()
+       }()
+
+       Server(s, testConfig).Handshake()
+
+       if len(buf) < 5+4 {
+               t.Fatalf("Server returned short message of length %d", len(buf))
+       }
+       // buf contains a TLS record, with a 5 byte record header and a 4 byte
+       // handshake header. The length of the ServerHello is taken from the
+       // handshake header.
+       serverHelloLen := int(buf[6])<<16 | int(buf[7])<<8 | int(buf[8])
+
+       var serverHello serverHelloMsg
+       // unmarshal expects to be given the handshake header, but
+       // serverHelloLen doesn't include it.
+       if !serverHello.unmarshal(buf[5 : 9+serverHelloLen]) {
+               t.Fatalf("Failed to parse ServerHello")
+       }
+
+       if !serverHello.secureRenegotiation {
+               t.Errorf("Secure renegotiation extension was not echoed.")
+       }
+}
+
 func TestTLS12OnlyCipherSuites(t *testing.T) {
        // Test that a Server doesn't select a TLS 1.2-only cipher suite when
        // the client negotiates TLS 1.1.