derp/derpserver,cmd/derper: don't mutate cert provider's shared tls.Certificate (#20478)

ModifyTLSConfigToAddMetaCert (and its inline copy in cmd/derper) appended
the DERP meta cert directly to the *tls.Certificate returned by the
underlying GetCertificate. autocert returns a certificate sharing a cached
chain slice (and, on the TLS-ALPN token path, the same pointer) across
concurrent handshakes, so the in-place append was a data race and could
grow the served chain unboundedly.

Return a shallow copy with the meta cert appended to a fresh backing
array instead, and have cmd/derper reuse ModifyTLSConfigToAddMetaCert
rather than duplicating the wrapper.

Fixes #20352

Signed-off-by: Mike O'Driscoll <mikeo@tailscale.com>

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)]

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) {

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
 	}
 }
 

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