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>

handshake_server_test.go

@@ -52,16 +52,33 @@ func testClientHelloFailure(t *testing.T, serverConfig *Config, m handshakeMessa
 	ctx := context.Background()
 	conn := Server(s, serverConfig)
 	ch, err := conn.readClientHello(ctx)
-	hs := serverHandshakeState{
-		c:           conn,
-		ctx:         ctx,
-		clientHello: ch,
-	}
-	if err == nil {
-		err = hs.processClientHello()
-	}
-	if err == nil {
-		err = hs.pickCipherSuite()
+	if conn.vers == VersionTLS13 {
+		hs := serverHandshakeStateTLS13{
+			c:           conn,
+			ctx:         ctx,
+			clientHello: ch,
+		}
+		if err == nil {
+			err = hs.processClientHello()
+		}
+		if err == nil {
+			err = hs.checkForResumption()
+		}
+		if err == nil {
+			err = hs.pickCertificate()
+		}
+	} else {
+		hs := serverHandshakeState{
+			c:           conn,
+			ctx:         ctx,
+			clientHello: ch,
+		}
+		if err == nil {
+			err = hs.processClientHello()
+		}
+		if err == nil {
+			err = hs.pickCipherSuite()
+		}
 	}
 	s.Close()
 	if len(expectedSubStr) == 0 {
@@ -903,10 +920,52 @@ func TestHandshakeServerHelloRetryRequest(t *testing.T) {
 		name:    "HelloRetryRequest",
 		command: []string{"openssl", "s_client", "-no_ticket", "-ciphersuites", "TLS_CHACHA20_POLY1305_SHA256", "-curves", "X25519:P-256"},
 		config:  config,
+		validate: func(cs ConnectionState) error {
+			if !cs.testingOnlyDidHRR {
+				return errors.New("expected HelloRetryRequest")
+			}
+			return nil
+		},
 	}
 	runServerTestTLS13(t, test)
 }
 
+// TestHandshakeServerKeySharePreference checks that we prefer a key share even
+// if it's later in the CurvePreferences order.
+func TestHandshakeServerKeySharePreference(t *testing.T) {
+	config := testConfig.Clone()
+	config.CurvePreferences = []CurveID{X25519, CurveP256}
+
+	test := &serverTest{
+		name:    "KeySharePreference",
+		command: []string{"openssl", "s_client", "-no_ticket", "-ciphersuites", "TLS_CHACHA20_POLY1305_SHA256", "-curves", "P-256:X25519"},
+		config:  config,
+		validate: func(cs ConnectionState) error {
+			if cs.testingOnlyDidHRR {
+				return errors.New("unexpected HelloRetryRequest")
+			}
+			return nil
+		},
+	}
+	runServerTestTLS13(t, test)
+}
+
+// TestHandshakeServerUnsupportedKeyShare tests a client that sends a key share
+// that's not in the supported groups list.
+func TestHandshakeServerUnsupportedKeyShare(t *testing.T) {
+	pk, _ := ecdh.X25519().GenerateKey(rand.Reader)
+	clientHello := &clientHelloMsg{
+		vers:               VersionTLS12,
+		random:             make([]byte, 32),
+		supportedVersions:  []uint16{VersionTLS13},
+		cipherSuites:       []uint16{TLS_CHACHA20_POLY1305_SHA256},
+		compressionMethods: []uint8{compressionNone},
+		keyShares:          []keyShare{{group: X25519, data: pk.PublicKey().Bytes()}},
+		supportedCurves:    []CurveID{CurveP256},
+	}
+	testClientHelloFailure(t, testConfig, clientHello, "client sent key share for group it does not support")
+}
+
 func TestHandshakeServerALPN(t *testing.T) {
 	config := testConfig.Clone()
 	config.NextProtos = []string{"proto1", "proto2"}
@@ -1932,6 +1991,7 @@ func TestAESCipherReorderingTLS13(t *testing.T) {
 					supportedVersions:  []uint16{VersionTLS13},
 					compressionMethods: []uint8{compressionNone},
 					keyShares:          []keyShare{{group: X25519, data: pk.PublicKey().Bytes()}},
+					supportedCurves:    []CurveID{X25519},
 				},
 			}