From d931716cde778a3a4c9ab14410f791e9e8b72785 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 11 Aug 2015 23:22:57 +0300 Subject: [PATCH] net/http: fix races cloning TLS config Found in a Google program running under the race detector. No test, but verified that this fixes the race with go run -race of: package main import ( "crypto/tls" "fmt" "net" "net/http" "net/http/httptest" ) func main() { for { ts := httptest.NewTLSServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {})) conf := &tls.Config{} // non-nil a, b := net.Pipe() go func() { sconn := tls.Server(a, conf) sconn.Handshake() }() tr := &http.Transport{ TLSClientConfig: conf, } req, _ := http.NewRequest("GET", ts.URL, nil) _, err := tr.RoundTrip(req) println(fmt.Sprint(err)) a.Close() b.Close() ts.Close() } } Also modified cmd/vet to report the copy-of-mutex bug statically in CL 13646, and fixed two other instances in the code found by vet. But vet could not have told us about cloneTLSConfig vs cloneTLSClientConfig. Confirmed that original report is also fixed by this. Fixes #12099. Change-Id: Iba0171549e01852a5ec3438c25a1951c98524dec Reviewed-on: https://go-review.googlesource.com/13453 Reviewed-by: Ian Lance Taylor Reviewed-by: Brad Fitzpatrick Reviewed-by: Austin Clements Run-TryBot: Russ Cox --- src/net/http/server.go | 7 +--- src/net/http/transport.go | 80 ++++++++++++++++++++++++++++++++++----- 2 files changed, 72 insertions(+), 15 deletions(-) diff --git a/src/net/http/server.go b/src/net/http/server.go index 1b292ea2de..a3e43555bb 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -473,7 +473,7 @@ func (srv *Server) newConn(rwc net.Conn) (c *conn, err error) { if debugServerConnections { c.rwc = newLoggingConn("server", c.rwc) } - c.sr = liveSwitchReader{r: c.rwc} + c.sr.r = c.rwc c.lr = io.LimitReader(&c.sr, noLimit).(*io.LimitedReader) br := newBufioReader(c.lr) bw := newBufioWriterSize(checkConnErrorWriter{c}, 4<<10) @@ -2015,10 +2015,7 @@ func (srv *Server) ListenAndServeTLS(certFile, keyFile string) error { if addr == "" { addr = ":https" } - config := &tls.Config{} - if srv.TLSConfig != nil { - *config = *srv.TLSConfig - } + config := cloneTLSConfig(srv.TLSConfig) if config.NextProtos == nil { config.NextProtos = []string{"http/1.1"} } diff --git a/src/net/http/transport.go b/src/net/http/transport.go index 09434f1234..70d1864605 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -645,16 +645,9 @@ func (t *Transport) dialConn(cm connectMethod) (*persistConn, error) { if cm.targetScheme == "https" && !tlsDial { // Initiate TLS and check remote host name against certificate. - cfg := t.TLSClientConfig - if cfg == nil || cfg.ServerName == "" { - host := cm.tlsHost() - if cfg == nil { - cfg = &tls.Config{ServerName: host} - } else { - clone := *cfg // shallow clone - clone.ServerName = host - cfg = &clone - } + cfg := cloneTLSClientConfig(t.TLSClientConfig) + if cfg.ServerName == "" { + cfg.ServerName = cm.tlsHost() } plainConn := pconn.conn tlsConn := tls.Client(plainConn, cfg) @@ -1399,3 +1392,70 @@ func isNetWriteError(err error) bool { return false } } + +// cloneTLSConfig returns a shallow clone of the exported +// fields of cfg, ignoring the unexported sync.Once, which +// contains a mutex and must not be copied. +// +// The cfg must not be in active use by tls.Server, or else +// there can still be a race with tls.Server updating SessionTicketKey +// and our copying it, and also a race with the server setting +// SessionTicketsDisabled=false on failure to set the random +// ticket key. +// +// If cfg is nil, a new zero tls.Config is returned. +func cloneTLSConfig(cfg *tls.Config) *tls.Config { + if cfg == nil { + return &tls.Config{} + } + return &tls.Config{ + Rand: cfg.Rand, + Time: cfg.Time, + Certificates: cfg.Certificates, + NameToCertificate: cfg.NameToCertificate, + GetCertificate: cfg.GetCertificate, + RootCAs: cfg.RootCAs, + NextProtos: cfg.NextProtos, + ServerName: cfg.ServerName, + ClientAuth: cfg.ClientAuth, + ClientCAs: cfg.ClientCAs, + InsecureSkipVerify: cfg.InsecureSkipVerify, + CipherSuites: cfg.CipherSuites, + PreferServerCipherSuites: cfg.PreferServerCipherSuites, + SessionTicketsDisabled: cfg.SessionTicketsDisabled, + SessionTicketKey: cfg.SessionTicketKey, + ClientSessionCache: cfg.ClientSessionCache, + MinVersion: cfg.MinVersion, + MaxVersion: cfg.MaxVersion, + CurvePreferences: cfg.CurvePreferences, + } +} + +// cloneTLSClientConfig is like cloneTLSConfig but omits +// the fields SessionTicketsDisabled and SessionTicketKey. +// This makes it safe to call cloneTLSClientConfig on a config +// in active use by a server. +func cloneTLSClientConfig(cfg *tls.Config) *tls.Config { + if cfg == nil { + return &tls.Config{} + } + return &tls.Config{ + Rand: cfg.Rand, + Time: cfg.Time, + Certificates: cfg.Certificates, + NameToCertificate: cfg.NameToCertificate, + GetCertificate: cfg.GetCertificate, + RootCAs: cfg.RootCAs, + NextProtos: cfg.NextProtos, + ServerName: cfg.ServerName, + ClientAuth: cfg.ClientAuth, + ClientCAs: cfg.ClientCAs, + InsecureSkipVerify: cfg.InsecureSkipVerify, + CipherSuites: cfg.CipherSuites, + PreferServerCipherSuites: cfg.PreferServerCipherSuites, + ClientSessionCache: cfg.ClientSessionCache, + MinVersion: cfg.MinVersion, + MaxVersion: cfg.MaxVersion, + CurvePreferences: cfg.CurvePreferences, + } +}