From ea64e5785d37487f40083a04d36f3d20c95b3f74 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Fri, 19 Dec 2014 15:14:03 -0800 Subject: [PATCH] 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 Reviewed-by: Brad Fitzpatrick --- src/crypto/tls/handshake_messages.go | 2 +- src/crypto/tls/handshake_server_test.go | 48 +++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/crypto/tls/handshake_messages.go b/src/crypto/tls/handshake_messages.go index 5d14871a34..95325088cf 100644 --- 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 f9545461a4..6205116012 100644 --- 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. -- 2.48.1