commit 3af62269df3525ea526a2b6de0d7c70e8f8d5919
author David Benjamin <davidben@google.com> Tue Mar 30 13:37:53 2021 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Tue Mar 30 18:15:23 2021 +0000
tree 93bac2193958ac265be703adef72b3169d71271d
parent 4aef687fcff7d0b4a6f2942a5b21c97f8da33097

Enforce that pre_shared_key must come with psk_key_exchange_modes.

Omitting the extension means we'll never issue tickets, but if the
client were to offer a ticket anyway, RFC8446 4.2.9 says we MUST reject
the ClientHello. It's not clear on what alert to use, but
missing_extension is probably appropriate.

Thanks to Ben Kaduk for pointing this out.

Change-Id: Ie5c720eac9dd2e1a27ba8a13c59b707c109eaa4e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46464
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>

ssl/test/runner/common.go

diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index f209f4a..34f0fd1 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1240,8 +1240,8 @@
 	// session ticket.
 	SendEmptySessionTicket bool
 
-	// SendPSKKeyExchangeModes, if present, determines the PSK key exchange modes
-	// to send.
+	// SendPSKKeyExchangeModes, if not nil, determines the PSK key exchange
+	// modes to send. If a non-nil empty slice, no extension will be sent.
 	SendPSKKeyExchangeModes []byte
 
 	// ExpectNoNewSessionTicket, if present, means that the client will fail upon

ssl/test/runner/handshake_client.go

diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index efb8a18..510cbce 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -186,7 +186,7 @@
 		hello.supportedCurves = nil
 	}
 
-	if len(c.config.Bugs.SendPSKKeyExchangeModes) != 0 {
+	if c.config.Bugs.SendPSKKeyExchangeModes != nil {
 		hello.pskKEModes = c.config.Bugs.SendPSKKeyExchangeModes
 	}
 

ssl/test/runner/runner.go

diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 86974e9..e555f46 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -12169,6 +12169,26 @@
 		expectResumeRejected: true,
 	})
 
+	// Test that the server rejects ClientHellos with pre_shared_key but without
+	// psk_key_exchange_modes.
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		name:     "TLS13-SendNoKEMModesWithPSK-Server",
+		config: Config{
+			MaxVersion: VersionTLS13,
+		},
+		resumeConfig: &Config{
+			MaxVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				SendPSKKeyExchangeModes: []byte{},
+			},
+		},
+		resumeSession:      true,
+		shouldFail:         true,
+		expectedLocalError: "remote error: missing extension",
+		expectedError:      ":MISSING_EXTENSION:",
+	})
+
 	// Test that the client ticket age is sent correctly.
 	testCases = append(testCases, testCase{
 		testType: clientTest,

ssl/tls13_server.cc

diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index 6e9cd07..d33afa5 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -252,6 +252,16 @@
     return ssl_ticket_aead_ignore_ticket;
   }
 
+  // Per RFC8446, section 4.2.9, servers MUST abort the handshake if the client
+  // sends pre_shared_key without psk_key_exchange_modes.
+  CBS unused;
+  if (!ssl_client_hello_get_extension(client_hello, &unused,
+                                      TLSEXT_TYPE_psk_key_exchange_modes)) {
+    *out_alert = SSL_AD_MISSING_EXTENSION;
+    OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_EXTENSION);
+    return ssl_ticket_aead_error;
+  }
+
   CBS ticket, binders;
   uint32_t client_ticket_age;
   if (!ssl_ext_pre_shared_key_parse_clienthello(