From f77df846bfc28789571eca457ee6cae28fcb141e Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Thu, 10 Feb 2022 09:47:49 -0800 Subject: [PATCH 1/2] crypto/tls: reject duplicate extensions Does what it says on the tin. Fixes #51088 Change-Id: I12c0fa6bba1c1ce96c1ad31ba387c77a93f801c9 Reviewed-on: https://go-review.googlesource.com/c/go/+/384894 Reviewed-by: Roland Shoemaker Run-TryBot: Roland Shoemaker TryBot-Result: Gopher Robot Reviewed-by: Damien Neil --- handshake_messages.go | 12 ++++++++++++ handshake_messages_test.go | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/handshake_messages.go b/handshake_messages.go index 17cf859..7ab0f10 100644 --- a/handshake_messages.go +++ b/handshake_messages.go @@ -384,6 +384,7 @@ func (m *clientHelloMsg) unmarshal(data []byte) bool { return false } + seenExts := make(map[uint16]bool) for !extensions.Empty() { var extension uint16 var extData cryptobyte.String @@ -392,6 +393,11 @@ func (m *clientHelloMsg) unmarshal(data []byte) bool { return false } + if seenExts[extension] { + return false + } + seenExts[extension] = true + switch extension { case extensionServerName: // RFC 6066, Section 3 @@ -750,6 +756,7 @@ func (m *serverHelloMsg) unmarshal(data []byte) bool { return false } + seenExts := make(map[uint16]bool) for !extensions.Empty() { var extension uint16 var extData cryptobyte.String @@ -758,6 +765,11 @@ func (m *serverHelloMsg) unmarshal(data []byte) bool { return false } + if seenExts[extension] { + return false + } + seenExts[extension] = true + switch extension { case extensionStatusRequest: m.ocspStapling = true diff --git a/handshake_messages_test.go b/handshake_messages_test.go index cc427bf..49452da 100644 --- a/handshake_messages_test.go +++ b/handshake_messages_test.go @@ -6,6 +6,7 @@ package tls import ( "bytes" + "encoding/hex" "math/rand" "reflect" "strings" @@ -463,3 +464,23 @@ func TestRejectEmptySCT(t *testing.T) { t.Fatal("Unmarshaled ServerHello with zero-length SCT") } } + +func TestRejectDuplicateExtensions(t *testing.T) { + clientHelloBytes, err := hex.DecodeString("010000440303000000000000000000000000000000000000000000000000000000000000000000000000001c0000000a000800000568656c6c6f0000000a000800000568656c6c6f") + if err != nil { + t.Fatalf("failed to decode test ClientHello: %s", err) + } + var clientHelloCopy clientHelloMsg + if clientHelloCopy.unmarshal(clientHelloBytes) { + t.Error("Unmarshaled ClientHello with duplicate extensions") + } + + serverHelloBytes, err := hex.DecodeString("02000030030300000000000000000000000000000000000000000000000000000000000000000000000000080005000000050000") + if err != nil { + t.Fatalf("failed to decode test ServerHello: %s", err) + } + var serverHelloCopy serverHelloMsg + if serverHelloCopy.unmarshal(serverHelloBytes) { + t.Fatal("Unmarshaled ServerHello with duplicate extensions") + } +} From 5eed9ff4b3864ef588ad30f19e144e16e673048d Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Tue, 19 Apr 2022 06:26:53 -0400 Subject: [PATCH 2/2] crypto/tls: remove tls10default GODEBUG flag Updates #45428 Change-Id: Ic2ff459e6a3f1e8ded2a770c11d34067c0b39a8a Reviewed-on: https://go-review.googlesource.com/c/go/+/400974 Reviewed-by: Filippo Valsorda Auto-Submit: Filippo Valsorda TryBot-Result: Gopher Robot Run-TryBot: Filippo Valsorda Reviewed-by: Roland Shoemaker --- common.go | 6 +----- handshake_server_test.go | 10 ---------- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/common.go b/common.go index e6e7598..59b41ef 100644 --- a/common.go +++ b/common.go @@ -18,7 +18,6 @@ import ( "crypto/x509" "errors" "fmt" - "internal/godebug" "io" "net" "strings" @@ -974,9 +973,6 @@ var supportedVersions = []uint16{ VersionTLS10, } -// debugEnableTLS10 enables TLS 1.0. See issue 45428. -var debugEnableTLS10 = godebug.Get("tls10default") == "1" - // roleClient and roleServer are meant to call supportedVersions and parents // with more readability at the callsite. const roleClient = true @@ -985,7 +981,7 @@ const roleServer = false func (c *Config) supportedVersions(isClient bool) []uint16 { versions := make([]uint16, 0, len(supportedVersions)) for _, v := range supportedVersions { - if (c == nil || c.MinVersion == 0) && !debugEnableTLS10 && + if (c == nil || c.MinVersion == 0) && isClient && v < VersionTLS12 { continue } diff --git a/handshake_server_test.go b/handshake_server_test.go index 16a2254..1f3a174 100644 --- a/handshake_server_test.go +++ b/handshake_server_test.go @@ -400,16 +400,6 @@ func TestVersion(t *testing.T) { if err == nil { t.Fatalf("expected failure to connect with TLS 1.0/1.1") } - - defer func(old bool) { debugEnableTLS10 = old }(debugEnableTLS10) - debugEnableTLS10 = true - _, _, err = testHandshake(t, clientConfig, serverConfig) - if err != nil { - t.Fatalf("handshake failed: %s", err) - } - if state.Version != VersionTLS11 { - t.Fatalf("incorrect version %x, should be %x", state.Version, VersionTLS11) - } } func TestCipherSuitePreference(t *testing.T) {