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>
diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index f0f450e..e9b2066 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -109,6 +109,7 @@
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
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index f588f46..796949d 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4566,6 +4566,7 @@
#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
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 21005f0..19c36d7 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1977,11 +1977,12 @@
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 @@
*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;
}
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index caa9cde..9fbf5dc 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1217,6 +1217,10 @@
// 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
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 2ebd0dc..e138f22 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -1676,11 +1676,16 @@
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)
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 35742cb..578d735 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -5979,6 +5979,36 @@
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",
resumeSession: true,
config: Config{
@@ -9502,7 +9532,8 @@
config: Config{
MaxVersion: VersionTLS13,
Bugs: ProtocolBugs{
- ExtraPSKIdentity: true,
+ ExtraPSKIdentity: true,
+ SendExtraPSKBinder: true,
},
},
resumeSession: true,
