From a4ee74dadfecc8c1bc6fdf974fb5f5c747f1e902 Mon Sep 17 00:00:00 2001 From: Steven Valdez Date: Tue, 29 Nov 2016 13:36:45 -0500 Subject: [PATCH] Skipping early data on 0RTT rejection. BUG=101 Change-Id: Ia1edbccee535b0bc3a0e18465286d5bcca240035 Reviewed-on: https://boringssl-review.googlesource.com/12470 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/internal.h | 8 ++ ssl/t1_lib.c | 38 ++++++++ ssl/test/runner/common.go | 19 ++++ ssl/test/runner/conn.go | 13 +++ ssl/test/runner/handshake_client.go | 32 ++++++- ssl/test/runner/runner.go | 140 +++++++++++++++++++++++++++- ssl/tls_record.c | 39 +++++++- 9 files changed, 287 insertions(+), 4 deletions(-) diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata index b50f9ab8..f0f450e8 100644 --- a/crypto/err/ssl.errordata +++ b/crypto/err/ssl.errordata @@ -170,6 +170,7 @@ SSL,218,TLS_RSA_ENCRYPTED_VALUE_LENGTH_IS_WRONG SSL,219,TOO_MANY_EMPTY_FRAGMENTS SSL,260,TOO_MANY_KEY_UPDATES SSL,220,TOO_MANY_WARNING_ALERTS +SSL,270,TOO_MUCH_SKIPPED_EARLY_DATA SSL,221,UNABLE_TO_FIND_ECDH_PARAMETERS SSL,222,UNEXPECTED_EXTENSION SSL,223,UNEXPECTED_MESSAGE diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index f5b9f9d3..0de49672 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -4552,6 +4552,7 @@ BORINGSSL_MAKE_DELETER(SSL_SESSION, SSL_SESSION_free) #define SSL_R_PRE_SHARED_KEY_MUST_BE_LAST 267 #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_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/internal.h b/ssl/internal.h index 1d78ec17..5893d4da 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1393,6 +1393,10 @@ typedef struct ssl3_state_st { * completed. */ unsigned initial_handshake_complete:1; + /* skip_early_data instructs the record layer to skip unexpected early data + * messages when 0RTT is rejected. */ + unsigned skip_early_data:1; + /* read_buffer holds data from the transport to be processed. */ SSL3_BUFFER read_buffer; /* write_buffer holds data to be written to the transport. */ @@ -1423,6 +1427,10 @@ typedef struct ssl3_state_st { /* recv_shutdown is the shutdown state for the send half of the connection. */ enum ssl_shutdown_t send_shutdown; + /* early_data_skipped is the amount of early data that has been skipped by the + * record layer. */ + uint16_t early_data_skipped; + int alert_dispatch; uint8_t send_alert[2]; diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 421232f1..21005f00 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -2034,6 +2034,7 @@ int ssl_ext_pre_shared_key_add_serverhello(SSL *ssl, CBB *out) { /* Pre-Shared Key Exchange Modes * * https://tools.ietf.org/html/draft-ietf-tls-tls13-18#section-4.2.7 */ + static int ext_psk_key_exchange_modes_add_clienthello(SSL *ssl, CBB *out) { uint16_t min_version, max_version; if (!ssl_get_version_range(ssl, &min_version, &max_version)) { @@ -2078,6 +2079,35 @@ static int ext_psk_key_exchange_modes_parse_clienthello(SSL *ssl, } +/* Early Data Indication + * + * https://tools.ietf.org/html/draft-ietf-tls-tls13-18#section-4.2.8 */ + +static int ext_early_data_add_clienthello(SSL *ssl, CBB *out) { + /* TODO(svaldez): Support 0RTT. */ + return 1; +} + +static int ext_early_data_parse_clienthello(SSL *ssl, uint8_t *out_alert, + CBS *contents) { + if (contents == NULL) { + return 1; + } + + if (CBS_len(contents) != 0) { + *out_alert = SSL_AD_DECODE_ERROR; + return 0; + } + + /* Since we don't currently accept 0-RTT, we have to skip past any early data + * the client might have sent. */ + if (ssl3_protocol_version(ssl) >= TLS1_3_VERSION) { + ssl->s3->skip_early_data = 1; + } + return 1; +} + + /* Key Share * * https://tools.ietf.org/html/draft-ietf-tls-tls13-16#section-4.2.5 */ @@ -2560,6 +2590,14 @@ static const struct tls_extension kExtensions[] = { ext_psk_key_exchange_modes_parse_clienthello, dont_add_serverhello, }, + { + TLSEXT_TYPE_early_data, + NULL, + ext_early_data_add_clienthello, + forbid_parse_serverhello, + ext_early_data_parse_clienthello, + dont_add_serverhello, + }, { TLSEXT_TYPE_supported_versions, NULL, diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index a6496fdd..caa9cde8 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -1109,6 +1109,25 @@ type ProtocolBugs struct { // copies of each KeyShareEntry. DuplicateKeyShares bool + // SendEarlyAlert, if true, sends a fatal alert after the ClientHello. + SendEarlyAlert bool + + // SendEarlyDataLength, if non-zero, is the amount of early data to send after + // the ClientHello. + SendEarlyDataLength int + + // OmitEarlyDataExtension, if true, causes the early data extension to + // be omitted in the ClientHello. + OmitEarlyDataExtension bool + + // SendEarlyDataOnSecondClientHello, if true, causes the TLS 1.3 client to + // send early data after the second ClientHello. + SendEarlyDataOnSecondClientHello bool + + // InterleaveEarlyData, if true, causes the TLS 1.3 client to send early + // data interleaved with the second ClientHello and the client Finished. + InterleaveEarlyData bool + // EmptyEncryptedExtensions, if true, causes the TLS 1.3 server to // emit an empty EncryptedExtensions block. EmptyEncryptedExtensions bool diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go index 39c27850..80a5b06d 100644 --- a/ssl/test/runner/conn.go +++ b/ssl/test/runner/conn.go @@ -1768,3 +1768,16 @@ func (c *Conn) sendKeyUpdateLocked(keyUpdateRequest byte) error { c.out.doKeyUpdate(c, true) return nil } + +func (c *Conn) sendFakeEarlyData(len int) error { + // Assemble a fake early data record. This does not use writeRecord + // because the record layer may be using different keys at this point. + payload := make([]byte, 5+len) + payload[0] = byte(recordTypeApplicationData) + payload[1] = 3 + payload[2] = 1 + payload[3] = byte(len >> 8) + payload[4] = byte(len) + _, err := c.conn.Write(payload) + return err +} diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index d074778c..2ebd0dcd 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go @@ -319,6 +319,10 @@ NextCipherSuite: hello.cipherSuites = c.config.Bugs.SendCipherSuites } + if c.config.Bugs.SendEarlyDataLength > 0 && !c.config.Bugs.OmitEarlyDataExtension { + hello.hasEarlyData = true + } + var helloBytes []byte if c.config.Bugs.SendV2ClientHello { // Test that the peer left-pads random. @@ -356,6 +360,12 @@ NextCipherSuite: if err := c.simulatePacketLoss(nil); err != nil { return err } + if c.config.Bugs.SendEarlyAlert { + c.sendAlert(alertHandshakeFailure) + } + if c.config.Bugs.SendEarlyDataLength > 0 { + c.sendFakeEarlyData(c.config.Bugs.SendEarlyDataLength) + } msg, err := c.readHandshake() if err != nil { return err @@ -452,16 +462,28 @@ NextCipherSuite: hello.hasKeyShares = false } - hello.hasEarlyData = false + hello.hasEarlyData = c.config.Bugs.SendEarlyDataOnSecondClientHello hello.raw = nil if len(hello.pskIdentities) > 0 { generatePSKBinders(hello, pskCipherSuite, session.masterSecret, append(helloBytes, helloRetryRequest.marshal()...), c.config) } secondHelloBytes = hello.marshal() - c.writeRecord(recordTypeHandshake, secondHelloBytes) + + if c.config.Bugs.InterleaveEarlyData { + c.sendFakeEarlyData(4) + c.writeRecord(recordTypeHandshake, secondHelloBytes[:16]) + c.sendFakeEarlyData(4) + c.writeRecord(recordTypeHandshake, secondHelloBytes[16:]) + } else { + c.writeRecord(recordTypeHandshake, secondHelloBytes) + } c.flushHandshake() + if c.config.Bugs.SendEarlyDataOnSecondClientHello { + c.sendFakeEarlyData(4) + } + msg, err = c.readHandshake() if err != nil { return err @@ -871,6 +893,12 @@ func (hs *clientHandshakeState) doTLS13Handshake() error { if c.config.Bugs.PartialClientFinishedWithClientHello { // The first byte has already been sent. c.writeRecord(recordTypeHandshake, finished.marshal()[1:]) + } else if c.config.Bugs.InterleaveEarlyData { + finishedBytes := finished.marshal() + c.sendFakeEarlyData(4) + c.writeRecord(recordTypeHandshake, finishedBytes[:1]) + c.sendFakeEarlyData(4) + c.writeRecord(recordTypeHandshake, finishedBytes[1:]) } else { c.writeRecord(recordTypeHandshake, finished.marshal()) } diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 683f07cb..35742cb1 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -5568,7 +5568,7 @@ func addExtensionTests() { "-signed-cert-timestamps", base64.StdEncoding.EncodeToString([]byte{0, 0}), }, - shouldFail: true, + shouldFail: true, expectedError: ":INVALID_SCT_LIST:", }) } @@ -9032,6 +9032,144 @@ func addTLS13HandshakeTests() { expectedError: ":DUPLICATE_KEY_SHARE:", }) + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SkipEarlyData", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SendEarlyDataLength: 4, + }, + }, + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SkipEarlyData-OmitEarlyDataExtension", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SendEarlyDataLength: 4, + OmitEarlyDataExtension: true, + }, + }, + shouldFail: true, + expectedError: ":DECRYPTION_FAILED_OR_BAD_RECORD_MAC:", + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SkipEarlyData-TooMuchData", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SendEarlyDataLength: 16384 + 1, + }, + }, + shouldFail: true, + expectedError: ":TOO_MUCH_SKIPPED_EARLY_DATA:", + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SkipEarlyData-Interleaved", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SendEarlyDataLength: 4, + InterleaveEarlyData: true, + }, + }, + shouldFail: true, + expectedError: ":DECRYPTION_FAILED_OR_BAD_RECORD_MAC:", + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SkipEarlyData-EarlyDataInTLS12", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SendEarlyDataLength: 4, + }, + }, + shouldFail: true, + expectedError: ":UNEXPECTED_RECORD:", + flags: []string{"-max-version", strconv.Itoa(VersionTLS12)}, + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SkipEarlyData-HRR", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SendEarlyDataLength: 4, + }, + DefaultCurves: []CurveID{}, + }, + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SkipEarlyData-HRR-Interleaved", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SendEarlyDataLength: 4, + InterleaveEarlyData: true, + }, + DefaultCurves: []CurveID{}, + }, + shouldFail: true, + expectedError: ":UNEXPECTED_RECORD:", + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SkipEarlyData-HRR-TooMuchData", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SendEarlyDataLength: 16384 + 1, + }, + DefaultCurves: []CurveID{}, + }, + shouldFail: true, + expectedError: ":TOO_MUCH_SKIPPED_EARLY_DATA:", + }) + + // Test that skipping early data looking for cleartext correctly + // processes an alert record. + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SkipEarlyData-HRR-FatalAlert", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SendEarlyAlert: true, + SendEarlyDataLength: 4, + }, + DefaultCurves: []CurveID{}, + }, + shouldFail: true, + expectedError: ":SSLV3_ALERT_HANDSHAKE_FAILURE:", + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "SkipEarlyData-SecondClientHelloEarlyData", + config: Config{ + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SendEarlyDataOnSecondClientHello: true, + }, + DefaultCurves: []CurveID{}, + }, + shouldFail: true, + expectedLocalError: "remote error: bad record MAC", + }) + testCases = append(testCases, testCase{ testType: clientTest, name: "EmptyEncryptedExtensions", diff --git a/ssl/tls_record.c b/ssl/tls_record.c index 59319225..c52909ce 100644 --- a/ssl/tls_record.c +++ b/ssl/tls_record.c @@ -125,6 +125,12 @@ * forever. */ static const uint8_t kMaxEmptyRecords = 32; +/* kMaxEarlyDataSkipped is the maximum amount of data processed when skipping + * over early data. Without this limit an attacker could send records at a + * faster rate than we can process and cause trial decryption to loop + * forever. */ +static const size_t kMaxEarlyDataSkipped = 16384; + /* kMaxWarningAlerts is the number of consecutive warning alerts that will be * processed. */ static const uint8_t kMaxWarningAlerts = 4; @@ -246,15 +252,32 @@ enum ssl_open_record_t tls_open_record(SSL *ssl, uint8_t *out_type, CBS *out, ssl_do_msg_callback(ssl, 0 /* read */, SSL3_RT_HEADER, in, SSL3_RT_HEADER_LENGTH); + *out_consumed = in_len - CBS_len(&cbs); + + /* Skip early data received when expecting a second ClientHello if we rejected + * 0RTT. */ + if (ssl->s3->skip_early_data && + ssl->s3->aead_read_ctx == NULL && + type == SSL3_RT_APPLICATION_DATA) { + goto skipped_data; + } + /* Decrypt the body in-place. */ if (!SSL_AEAD_CTX_open(ssl->s3->aead_read_ctx, out, type, version, ssl->s3->read_sequence, (uint8_t *)CBS_data(&body), CBS_len(&body))) { + if (ssl->s3->skip_early_data && + ssl->s3->aead_read_ctx != NULL) { + ERR_clear_error(); + goto skipped_data; + } + OPENSSL_PUT_ERROR(SSL, SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC); *out_alert = SSL_AD_BAD_RECORD_MAC; return ssl_open_record_error; } - *out_consumed = in_len - CBS_len(&cbs); + + ssl->s3->skip_early_data = 0; if (!ssl_record_sequence_update(ssl->s3->read_sequence, 8)) { *out_alert = SSL_AD_INTERNAL_ERROR; @@ -310,6 +333,20 @@ enum ssl_open_record_t tls_open_record(SSL *ssl, uint8_t *out_type, CBS *out, *out_type = type; return ssl_open_record_success; + +skipped_data: + ssl->s3->early_data_skipped += *out_consumed; + if (ssl->s3->early_data_skipped < *out_consumed) { + ssl->s3->early_data_skipped = kMaxEarlyDataSkipped + 1; + } + + if (ssl->s3->early_data_skipped > kMaxEarlyDataSkipped) { + OPENSSL_PUT_ERROR(SSL, SSL_R_TOO_MUCH_SKIPPED_EARLY_DATA); + *out_alert = SSL_AD_UNEXPECTED_MESSAGE; + return ssl_open_record_error; + } + + return ssl_open_record_discard; } static int do_seal_record(SSL *ssl, uint8_t *out, size_t *out_len,