crypto/tls: clarify group selection logic

I initially thought the logic was broken, but writing the test I
realized it was actually very clever (derogative). It was relying on the
outer loop continuing after a supported match without a key share,
allowing a later key share to override it (but not a later supported
match because of the "if selectedGroup != 0 { continue }").

Replaced the clever loop with two hopefully more understandable loops,
and added a test (which was already passing).

We were however not checking that the selected group is in the supported
list if we found it in key shares first. (This was only a MAY.) Fixed.

Fixes #65686

Change-Id: I09ea44f90167ffa36809deb78255ed039a217b6d
Reviewed-on: https://go-review.googlesource.com/c/go/+/586655
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Filippo Valsorda <filippo@golang.org>

common.go

@@ -298,6 +298,9 @@ type ConnectionState struct {
 
 	// ekm is a closure exposed via ExportKeyingMaterial.
 	ekm func(label string, context []byte, length int) ([]byte, error)
+
+	// testingOnlyDidHRR is true if a HelloRetryRequest was sent/received.
+	testingOnlyDidHRR bool
 }
 
 // ExportKeyingMaterial returns length bytes of exported key material in a new

conn.go

@@ -52,6 +52,7 @@ type Conn struct {
 	handshakes       int
 	extMasterSecret  bool
 	didResume        bool // whether this connection was a session resumption
+	didHRR           bool // whether a HelloRetryRequest was sent/received
 	cipherSuite      uint16
 	ocspResponse     []byte   // stapled OCSP response
 	scts             [][]byte // signed certificate timestamps from server
@@ -1669,6 +1670,7 @@ func (c *Conn) connectionStateLocked() ConnectionState {
 	state.Version = c.vers
 	state.NegotiatedProtocol = c.clientProtocol
 	state.DidResume = c.didResume
+	state.testingOnlyDidHRR = c.didHRR
 	state.NegotiatedProtocolIsMutual = true
 	state.ServerName = c.serverName
 	state.CipherSuite = c.cipherSuite

handshake_client_tls13.go

@@ -307,6 +307,7 @@ func (hs *clientHandshakeStateTLS13) processHelloRetryRequest() error {
 		return err
 	}
 
+	c.didHRR = true
 	return nil
 }
 

handshake_server_tls13.go

@@ -19,6 +19,7 @@ import (
 	"hash"
 	"io"
 	"math/big"
+	"slices"
 	"time"
 )
 
@@ -233,21 +234,25 @@ func (hs *serverHandshakeStateTLS13) processClientHello() error {
 	// groups with a key share, to avoid a HelloRetryRequest round-trip.
 	var selectedGroup CurveID
 	var clientKeyShare *keyShare
-GroupSelection:
-	for _, preferredGroup := range c.config.curvePreferences() {
-		for _, ks := range hs.clientHello.keyShares {
-			if ks.group == preferredGroup {
-				selectedGroup = ks.group
-				clientKeyShare = &ks
-				break GroupSelection
+	preferredGroups := c.config.curvePreferences()
+	for _, preferredGroup := range preferredGroups {
+		ki := slices.IndexFunc(hs.clientHello.keyShares, func(ks keyShare) bool {
+			return ks.group == preferredGroup
+		})
+		if ki != -1 {
+			clientKeyShare = &hs.clientHello.keyShares[ki]
+			selectedGroup = clientKeyShare.group
+			if !slices.Contains(hs.clientHello.supportedCurves, selectedGroup) {
+				c.sendAlert(alertIllegalParameter)
+				return errors.New("tls: client sent key share for group it does not support")
 			}
+			break
 		}
-		if selectedGroup != 0 {
-			continue
-		}
-		for _, group := range hs.clientHello.supportedCurves {
-			if group == preferredGroup {
-				selectedGroup = group
+	}
+	if selectedGroup == 0 {
+		for _, preferredGroup := range preferredGroups {
+			if slices.Contains(hs.clientHello.supportedCurves, preferredGroup) {
+				selectedGroup = preferredGroup
 				break
 			}
 		}
@@ -584,6 +589,7 @@ func (hs *serverHandshakeStateTLS13) doHelloRetryRequest(selectedGroup CurveID)
 		return errors.New("tls: client illegally modified second ClientHello")
 	}
 
+	c.didHRR = true
 	hs.clientHello = clientHello
 	return nil
 }