Parse the entire PSK extension.

Although we ignore all but the first identity, keep clients honest by
parsing the whole thing. Also explicitly check that the binder and
identity counts match.

Change-Id: Ib9c4caae18398360f3b80f8db1b22d4549bd5746
Reviewed-on: https://boringssl-review.googlesource.com/12469
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

crypto/err/ssl.errordata

@@ -109,6 +109,7 @@ SSL,192,PEER_DID_NOT_RETURN_A_CERTIFICATE
 SSL,193,PEER_ERROR_UNSUPPORTED_CERTIFICATE_TYPE
 SSL,267,PRE_SHARED_KEY_MUST_BE_LAST
 SSL,194,PROTOCOL_IS_SHUTDOWN
+SSL,271,PSK_IDENTITY_BINDER_COUNT_MISMATCH
 SSL,195,PSK_IDENTITY_NOT_FOUND
 SSL,196,PSK_NO_CLIENT_CB
 SSL,197,PSK_NO_SERVER_CB

include/openssl/ssl.h

@@ -4566,6 +4566,7 @@ BORINGSSL_MAKE_DELETER(SSL_SESSION, SSL_SESSION_free)
 #define SSL_R_OLD_SESSION_PRF_HASH_MISMATCH 268
 #define SSL_R_INVALID_SCT_LIST 269
 #define SSL_R_TOO_MUCH_SKIPPED_EARLY_DATA 270
+#define SSL_R_PSK_IDENTITY_BINDER_COUNT_MISMATCH 271
 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000
 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020

ssl/t1_lib.c

@@ -1977,11 +1977,12 @@ int ssl_ext_pre_shared_key_parse_clienthello(SSL *ssl,
                                              CBS *contents) {
   /* We only process the first PSK identity since we don't support pure PSK. */
   uint32_t obfuscated_ticket_age;
-  CBS identity, ticket, binders;
-  if (!CBS_get_u16_length_prefixed(contents, &identity) ||
-      !CBS_get_u16_length_prefixed(&identity, &ticket) ||
-      !CBS_get_u32(&identity, &obfuscated_ticket_age) ||
+  CBS identities, ticket, binders;
+  if (!CBS_get_u16_length_prefixed(contents, &identities) ||
+      !CBS_get_u16_length_prefixed(&identities, &ticket) ||
+      !CBS_get_u32(&identities, &obfuscated_ticket_age) ||
       !CBS_get_u16_length_prefixed(contents, &binders) ||
+      CBS_len(&binders) == 0 ||
       CBS_len(contents) != 0) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
     *out_alert = SSL_AD_DECODE_ERROR;
@@ -1990,11 +1991,38 @@ int ssl_ext_pre_shared_key_parse_clienthello(SSL *ssl,
 
   *out_binders = binders;
 
-  /* The PSK identity must have a corresponding binder. */
-  CBS binder;
-  if (!CBS_get_u8_length_prefixed(&binders, &binder)) {
-    OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
-    *out_alert = SSL_AD_DECODE_ERROR;
+  /* Check the syntax of the remaining identities, but do not process them. */
+  size_t num_identities = 1;
+  while (CBS_len(&identities) != 0) {
+    CBS unused_ticket;
+    uint32_t unused_obfuscated_ticket_age;
+    if (!CBS_get_u16_length_prefixed(&identities, &unused_ticket) ||
+        !CBS_get_u32(&identities, &unused_obfuscated_ticket_age)) {
+      OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+      *out_alert = SSL_AD_DECODE_ERROR;
+      return 0;
+    }
+
+    num_identities++;
+  }
+
+  /* Check the syntax of the binders. The value will be checked later if
+   * resuming. */
+  size_t num_binders = 0;
+  while (CBS_len(&binders) != 0) {
+    CBS binder;
+    if (!CBS_get_u8_length_prefixed(&binders, &binder)) {
+      OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+      *out_alert = SSL_AD_DECODE_ERROR;
+      return 0;
+    }
+
+    num_binders++;
+  }
+
+  if (num_identities != num_binders) {
+    OPENSSL_PUT_ERROR(SSL, SSL_R_PSK_IDENTITY_BINDER_COUNT_MISMATCH);
+    *out_alert = SSL_AD_ILLEGAL_PARAMETER;
     return 0;
   }
 

ssl/test/runner/common.go

@@ -1217,6 +1217,10 @@ type ProtocolBugs struct {
 	// SendNoPSKBinder, if true, causes the client to send no PSK binders.
 	SendNoPSKBinder bool
 
+	// SendExtraPSKBinder, if true, causes the client to send an extra PSK
+	// binder.
+	SendExtraPSKBinder bool
+
 	// PSKBinderFirst, if true, causes the client to send the PSK Binder
 	// extension as the first extension instead of the last extension.
 	PSKBinderFirst bool

ssl/test/runner/handshake_client.go

@@ -1676,11 +1676,16 @@ func generatePSKBinders(hello *clientHelloMsg, pskCipherSuite *cipherSuite, psk,
 		binderLen--
 	}
 
+	numBinders := 1
+	if config.Bugs.SendExtraPSKBinder {
+		numBinders++
+	}
+
 	// Fill hello.pskBinders with appropriate length arrays of zeros so the
 	// length prefixes are correct when computing the binder over the truncated
 	// ClientHello message.
-	hello.pskBinders = make([][]byte, len(hello.pskIdentities))
-	for i := range hello.pskIdentities {
+	hello.pskBinders = make([][]byte, numBinders)
+	for i := range hello.pskBinders {
 		hello.pskBinders[i] = make([]byte, binderLen)
 	}
 

ssl/test/runner/runner.go

@@ -5977,6 +5977,36 @@ func addResumptionVersionTests() {
 		expectedError:      ":DECODE_ERROR:",
 	})
 
+	testCases = append(testCases, testCase{
+		testType:      serverTest,
+		name:          "Resume-Server-ExtraPSKBinder",
+		resumeSession: true,
+		config: Config{
+			MaxVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				SendExtraPSKBinder: true,
+			},
+		},
+		shouldFail:         true,
+		expectedLocalError: "remote error: illegal parameter",
+		expectedError:      ":PSK_IDENTITY_BINDER_COUNT_MISMATCH:",
+	})
+
+	testCases = append(testCases, testCase{
+		testType:      serverTest,
+		name:          "Resume-Server-ExtraIdentityNoBinder",
+		resumeSession: true,
+		config: Config{
+			MaxVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				ExtraPSKIdentity: true,
+			},
+		},
+		shouldFail:         true,
+		expectedLocalError: "remote error: illegal parameter",
+		expectedError:      ":PSK_IDENTITY_BINDER_COUNT_MISMATCH:",
+	})
+
 	testCases = append(testCases, testCase{
 		testType:      serverTest,
 		name:          "Resume-Server-InvalidPSKBinder",
@@ -9502,7 +9532,8 @@ func addTLS13HandshakeTests() {
 		config: Config{
 			MaxVersion: VersionTLS13,
 			Bugs: ProtocolBugs{
-				ExtraPSKIdentity: true,
+				ExtraPSKIdentity:   true,
+				SendExtraPSKBinder: true,
 			},
 		},
 		resumeSession: true,