From 97fe6957a4e8d76eabc2fb87c5f6eafc3fc59f5d Mon Sep 17 00:00:00 2001
From: Filippo Valsorda <filippo@golang.org>
Date: Mon, 12 Nov 2018 16:04:07 -0500
Subject: [PATCH] crypto/tls: don't modify Config.Certificates in
 BuildNameToCertificate

The Config does not own the memory pointed to by the Certificate slice.
Instead, opportunistically use Certificate.Leaf and let the application
set it if it desires the performance gain.

This is a partial rollback of CL 107627. See the linked issue for the
full explanation.

Fixes #28744

Change-Id: I33ce9e6712e3f87939d9d0932a06d24e48ba4567
Reviewed-on: https://go-review.googlesource.com/c/149098
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
---
 common.go   |  8 ++++----
 tls_test.go | 22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/common.go b/common.go
index 25e4a7d..3ba3aac 100644
--- a/common.go
+++ b/common.go
@@ -871,14 +871,14 @@ func (c *Config) BuildNameToCertificate() {
 	c.NameToCertificate = make(map[string]*Certificate)
 	for i := range c.Certificates {
 		cert := &c.Certificates[i]
-		if cert.Leaf == nil {
-			x509Cert, err := x509.ParseCertificate(cert.Certificate[0])
+		x509Cert := cert.Leaf
+		if x509Cert == nil {
+			var err error
+			x509Cert, err = x509.ParseCertificate(cert.Certificate[0])
 			if err != nil {
 				continue
 			}
-			cert.Leaf = x509Cert
 		}
-		x509Cert := cert.Leaf
 		if len(x509Cert.Subject.CommonName) > 0 {
 			c.NameToCertificate[x509Cert.Subject.CommonName] = cert
 		}
diff --git a/tls_test.go b/tls_test.go
index e23068c..00bb6e4 100644
--- a/tls_test.go
+++ b/tls_test.go
@@ -1087,3 +1087,25 @@ func TestEscapeRoute(t *testing.T) {
 		t.Errorf("Client negotiated version %x, expected %x", cs.Version, VersionTLS12)
 	}
 }
+
+// Issue 28744: Ensure that we don't modify memory
+// that Config doesn't own such as Certificates.
+func TestBuildNameToCertificate_doesntModifyCertificates(t *testing.T) {
+	c0 := Certificate{
+		Certificate: [][]byte{testRSACertificate},
+		PrivateKey:  testRSAPrivateKey,
+	}
+	c1 := Certificate{
+		Certificate: [][]byte{testSNICertificate},
+		PrivateKey:  testRSAPrivateKey,
+	}
+	config := testConfig.Clone()
+	config.Certificates = []Certificate{c0, c1}
+
+	config.BuildNameToCertificate()
+	got := config.Certificates
+	want := []Certificate{c0, c1}
+	if !reflect.DeepEqual(got, want) {
+		t.Fatalf("Certificates were mutated by BuildNameToCertificate\nGot: %#v\nWant: %#v\n", got, want)
+	}
+}