From aedf303cc2edbe3fc404b73186477cbb614b65ad Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 1 Dec 2016 16:47:56 -0500 Subject: [PATCH] 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 Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org --- crypto/err/ssl.errordata | 1 + include/openssl/ssl.h | 1 + ssl/t1_lib.c | 46 +++++++++++++++++++++++------ ssl/test/runner/common.go | 4 +++ ssl/test/runner/handshake_client.go | 9 ++++-- ssl/test/runner/runner.go | 33 ++++++++++++++++++++- 6 files changed, 82 insertions(+), 12 deletions(-) diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata index f0f450e8..e9b20664 100644 --- a/crypto/err/ssl.errordata +++ b/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 diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index f588f46b..796949d8 100644 --- a/include/openssl/ssl.h +++ b/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 diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 21005f00..19c36d76 100644 --- a/ssl/t1_lib.c +++ b/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; } diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index caa9cde8..9fbf5dc9 100644 --- a/ssl/test/runner/common.go +++ b/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 diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index 2ebd0dcd..e138f228 100644 --- a/ssl/test/runner/handshake_client.go +++ b/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) } diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 35742cb1..578d7355 100644 --- a/ssl/test/runner/runner.go +++ b/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,