diff --git a/cmd/derper/cert.go b/cmd/derper/cert.go index 979c0d671..ceeb36c39 100644 --- a/cmd/derper/cert.go +++ b/cmd/derper/cert.go @@ -35,10 +35,9 @@ type certProvider interface { // TLSConfig creates a new TLS config suitable for net/http.Server servers. // - // The returned Config must have a GetCertificate function set and that - // function must return a unique *tls.Certificate for each call. The - // returned *tls.Certificate will be mutated by the caller to append to the - // (*tls.Certificate).Certificate field. + // The returned Config must have a GetCertificate function set. The + // *tls.Certificate values it returns may be shared and cached, so + // callers must not mutate them. TLSConfig() *tls.Config // HTTPHandler handle ACME related request, if any. HTTPHandler(fallback http.Handler) http.Handler @@ -157,8 +156,8 @@ func (m *manualCertManager) getCertificate(hi *tls.ClientHelloInfo) (*tls.Certif return nil, fmt.Errorf("cert mismatch with hostname: %q", hi.ServerName) } - // Return a shallow copy of the cert so the caller can append to its - // Certificate field. + // Return a shallow copy of the cert with a capacity-clamped chain + // so callers can never mutate the manager's long-lived certificate. certCopy := new(tls.Certificate) *certCopy = *m.cert certCopy.Certificate = certCopy.Certificate[:len(certCopy.Certificate):len(certCopy.Certificate)] diff --git a/cmd/derper/derper.go b/cmd/derper/derper.go index 0e3ab1bc2..ba5100ae3 100644 --- a/cmd/derper/derper.go +++ b/cmd/derper/derper.go @@ -354,15 +354,7 @@ func main() { log.Fatalf("derper: can not start cert provider: %v", err) } httpsrv.TLSConfig = certManager.TLSConfig() - getCert := httpsrv.TLSConfig.GetCertificate - httpsrv.TLSConfig.GetCertificate = func(hi *tls.ClientHelloInfo) (*tls.Certificate, error) { - cert, err := getCert(hi) - if err != nil { - return nil, err - } - cert.Certificate = append(cert.Certificate, s.MetaCert()) - return cert, nil - } + s.ModifyTLSConfigToAddMetaCert(httpsrv.TLSConfig) // Disable TLS 1.0 and 1.1, which are obsolete and have security issues. httpsrv.TLSConfig.MinVersion = tls.VersionTLS12 httpsrv.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/derp/derpserver/derpserver.go b/derp/derpserver/derpserver.go index 947f4b005..3da6f2bfa 100644 --- a/derp/derpserver/derpserver.go +++ b/derp/derpserver/derpserver.go @@ -719,7 +719,9 @@ func (s *Server) initMetacert() { func (s *Server) MetaCert() []byte { return s.metaCert } // ModifyTLSConfigToAddMetaCert modifies c.GetCertificate to make -// it append s.MetaCert to the returned certificates. +// it append s.MetaCert to the returned certificates. The certificate +// returned by the underlying GetCertificate is not mutated; a copy +// with the meta cert appended is returned instead. // // It panics if c or c.GetCertificate is nil. func (s *Server) ModifyTLSConfigToAddMetaCert(c *tls.Config) { @@ -732,8 +734,19 @@ func (s *Server) ModifyTLSConfigToAddMetaCert(c *tls.Config) { if err != nil { return nil, err } - cert.Certificate = append(cert.Certificate, s.MetaCert()) - return cert, nil + if cert == nil { + // Underlying GetCertificate returned (nil, nil) to signal + // fallback to Config.Certificates et al. Pass that through. + return nil, nil + } + // Don't mutate the *tls.Certificate pointed to by cert: the + // underlying GetCertificate implementation may return a shared + // cached value. Return a shallow copy with the meta cert + // appended to a freshly allocated chain slice. + certCopy := *cert + chain := cert.Certificate + certCopy.Certificate = append(chain[:len(chain):len(chain)], s.MetaCert()) + return &certCopy, nil } } diff --git a/derp/derpserver/derpserver_test.go b/derp/derpserver/derpserver_test.go index 13826819b..afb329b39 100644 --- a/derp/derpserver/derpserver_test.go +++ b/derp/derpserver/derpserver_test.go @@ -5,8 +5,10 @@ import ( "bufio" + "bytes" "cmp" "context" + "crypto/tls" "crypto/x509" "encoding/asn1" "encoding/binary" @@ -360,6 +362,70 @@ func TestMetaCert(t *testing.T) { } } +// TestModifyTLSConfigToAddMetaCert verifies that the wrapped GetCertificate +// appends the meta cert to a copy of the provider's chain without mutating +// the shared *tls.Certificate returned by the underlying provider (issue +// 20352). Cert providers such as autocert cache and return the same +// *tls.Certificate for concurrent handshakes, so appending to its +// Certificate slice in place is both a data race and unbounded growth of +// the served chain. +func TestModifyTLSConfigToAddMetaCert(t *testing.T) { + s := New(key.NewNode(), t.Logf) + + // Give the shared chain slice spare capacity so that a regression to a + // plain append (rather than a copy into a freshly allocated slice) + // writes into the shared backing array. Concurrent goroutines doing so + // is a write-write race caught by the race detector, and the write + // itself is caught by the backing-array check after wg.Wait below. + chain := make([][]byte, 1, 2) + chain[0] = []byte{1, 2, 3} + shared := &tls.Certificate{ + Certificate: chain, + } + c := &tls.Config{ + GetCertificate: func(*tls.ClientHelloInfo) (*tls.Certificate, error) { + return shared, nil + }, + } + s.ModifyTLSConfigToAddMetaCert(c) + + var wg sync.WaitGroup + // The goroutine count and iteration count are arbitrary: correctness is + // checked deterministically by the assertions below; the concurrency + // exists only to give the race detector overlapping calls to observe. + for range 10 { + wg.Go(func() { + for range 10 { + cert, err := c.GetCertificate(&tls.ClientHelloInfo{}) + if err != nil { + t.Errorf("GetCertificate: %v", err) + return + } + if len(cert.Certificate) != 2 { + t.Errorf("chain length = %d; want 2", len(cert.Certificate)) + return + } + if !bytes.Equal(cert.Certificate[0], []byte{1, 2, 3}) { + t.Errorf("chain[0] = %v; want provider's leaf cert %v", cert.Certificate[0], []byte{1, 2, 3}) + return + } + if !bytes.Equal(cert.Certificate[1], s.MetaCert()) { + t.Errorf("chain[1] = %v; want meta cert", cert.Certificate[1]) + return + } + } + }) + } + wg.Wait() + + if len(shared.Certificate) != 1 { + t.Errorf("shared cert chain length = %d; want 1 (must not be mutated)", len(shared.Certificate)) + } + if got := chain[:cap(chain)][1]; got != nil { + t.Errorf("shared cert backing array was written: %v", got) + } +} + func TestServerDupClients(t *testing.T) { serverPriv := key.NewNode() var s *Server