[release-branch.go1.24] net/http: don't modify caller's tls.Config.NextProtos

Clone the input slice before adjusting NextProtos
to add or remove "http/1.1" and "h2" entries,
so as not to modify a slice that the caller might be using.
(We clone the tls.Config that contains the slice, but
that's a shallow clone.)

For #72100
Fixes #72103

Change-Id: I9f228b8fb6f6f2ca5023179ec114929c002dbda9
Reviewed-on: https://go-review.googlesource.com/c/go/+/654875
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/657215

Author: neild

src/net/http/serve_test.go

@@ -13,6 +13,7 @@ import (
 	"compress/zlib"
 	"context"
 	"crypto/tls"
+	"crypto/x509"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -7303,3 +7304,71 @@ func testServerReadAfterHandlerAbort100Continue(t *testing.T, mode testMode) {
 	readyc <- struct{}{} // server starts reading from the request body
 	readyc <- struct{}{} // server finishes reading from the request body
 }
+
+// Issue #72100: Verify that we don't modify the caller's TLS.Config.NextProtos slice.
+func TestServerTLSNextProtos(t *testing.T) {
+	run(t, testServerTLSNextProtos, []testMode{https1Mode, http2Mode})
+}
+func testServerTLSNextProtos(t *testing.T, mode testMode) {
+	CondSkipHTTP2(t)
+
+	cert, err := tls.X509KeyPair(testcert.LocalhostCert, testcert.LocalhostKey)
+	if err != nil {
+		t.Fatal(err)
+	}
+	leafCert, err := x509.ParseCertificate(cert.Certificate[0])
+	if err != nil {
+		t.Fatal(err)
+	}
+	certpool := x509.NewCertPool()
+	certpool.AddCert(leafCert)
+
+	protos := new(Protocols)
+	switch mode {
+	case https1Mode:
+		protos.SetHTTP1(true)
+	case http2Mode:
+		protos.SetHTTP2(true)
+	}
+
+	wantNextProtos := []string{"http/1.1", "h2", "other"}
+	nextProtos := slices.Clone(wantNextProtos)
+
+	// We don't use httptest here because it overrides the tls.Config.
+	srv := &Server{
+		TLSConfig: &tls.Config{
+			Certificates: []tls.Certificate{cert},
+			NextProtos:   nextProtos,
+		},
+		Handler:   HandlerFunc(func(w ResponseWriter, req *Request) {}),
+		Protocols: protos,
+	}
+	tr := &Transport{
+		TLSClientConfig: &tls.Config{
+			RootCAs:    certpool,
+			NextProtos: nextProtos,
+		},
+		Protocols: protos,
+	}
+
+	listener := newLocalListener(t)
+	srvc := make(chan error, 1)
+	go func() {
+		srvc <- srv.ServeTLS(listener, "", "")
+	}()
+	t.Cleanup(func() {
+		srv.Close()
+		<-srvc
+	})
+
+	client := &Client{Transport: tr}
+	resp, err := client.Get("https://" + listener.Addr().String())
+	if err != nil {
+		t.Fatal(err)
+	}
+	resp.Body.Close()
+
+	if !slices.Equal(nextProtos, wantNextProtos) {
+		t.Fatalf("after running test: original NextProtos slice = %v, want %v", nextProtos, wantNextProtos)
+	}
+}

src/net/http/server.go

@@ -3521,6 +3521,12 @@ func (s *Server) protocols() Protocols {
 // adjustNextProtos adds or removes "http/1.1" and "h2" entries from
 // a tls.Config.NextProtos list, according to the set of protocols in protos.
 func adjustNextProtos(nextProtos []string, protos Protocols) []string {
+	// Make a copy of NextProtos since it might be shared with some other tls.Config.
+	// (tls.Config.Clone doesn't do a deep copy.)
+	//
+	// We could avoid an allocation in the common case by checking to see if the slice
+	// is already in order, but this is just one small allocation per connection.
+	nextProtos = slices.Clone(nextProtos)
 	var have Protocols
 	nextProtos = slices.DeleteFunc(nextProtos, func(s string) bool {
 		switch s {