From ba5934b77f0058dc963b075e0ec1f401a24c0b2b Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Tue, 2 Jun 2015 10:50:35 -0700 Subject: [PATCH] Tighten up EMS resumption behaviour. The client and server both have to decide on behaviour when resuming a session where the EMS state of the session doesn't match the EMS state as exchanged in the handshake. Original handshake | No Yes ------+-------------------------------------------------------------- | R | Server: ok [1] Server: abort [3] e No | Client: ok [2] Client: abort [4] s | u | m | e | Yes | Server: don't resume No problem | Client: abort; server | shouldn't have resumed [1] Servers want to accept legacy clients. The draft[5] says that resumptions SHOULD be rejected so that Triple-Handshake can't be done, but we'll rather enforce that EMS was used when using tls-unique etc. [2] The draft[5] says that even the initial handshake should be aborted if the server doesn't support EMS, but we need to be able to talk to the world. [3] This is a very weird case where a client has regressed without flushing the session cache. Hopefully we can be strict and reject these. [4] This can happen when a server-farm shares a session cache but frontends are not all updated at once. If Chrome is strict here then hopefully we can prevent any servers from existing that will try to resume an EMS session that they don't understand. OpenSSL appears to be ok here: https://www.ietf.org/mail-archive/web/tls/current/msg16570.html [5] https://tools.ietf.org/html/draft-ietf-tls-session-hash-05#section-5.2 BUG=492200 Change-Id: Ie1225a3960d49117b05eefa5a36263d8e556e467 Reviewed-on: https://boringssl-review.googlesource.com/4981 Reviewed-by: Adam Langley --- crypto/err/ssl.errordata | 3 ++ include/openssl/ssl.h | 3 ++ ssl/s3_clnt.c | 13 +++++ ssl/s3_srvr.c | 48 +++++++++++++++---- ssl/test/runner/runner.go | 99 ++++++++++++++++++++++++++++++++++----- 5 files changed, 146 insertions(+), 20 deletions(-) diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata index 630227b7..4ae0a518 100644 --- a/crypto/err/ssl.errordata +++ b/crypto/err/ssl.errordata @@ -224,6 +224,7 @@ SSL,reason,145,DIGEST_CHECK_FAILED SSL,reason,146,DTLS_MESSAGE_TOO_BIG SSL,reason,147,ECC_CERT_NOT_FOR_SIGNING SSL,reason,148,EMPTY_SRTP_PROTECTION_PROFILE_LIST +SSL,reason,276,EMS_STATE_INCONSISTENT SSL,reason,149,ENCRYPTED_LENGTH_TOO_LONG SSL,reason,150,ERROR_IN_RECEIVED_CIPHER_LIST SSL,reason,151,EVP_DIGESTSIGNFINAL_FAILED @@ -294,6 +295,8 @@ SSL,reason,212,RENEGOTIATE_EXT_TOO_LONG SSL,reason,213,RENEGOTIATION_ENCODING_ERR SSL,reason,214,RENEGOTIATION_MISMATCH SSL,reason,215,REQUIRED_CIPHER_MISSING +SSL,reason,275,RESUMED_EMS_SESSION_WITHOUT_EMS_EXTENSION +SSL,reason,277,RESUMED_NON_EMS_SESSION_WITH_EMS_EXTENSION SSL,reason,216,SCSV_RECEIVED_WHEN_RENEGOTIATING SSL,reason,217,SERVERHELLO_TLSEXT SSL,reason,218,SESSION_ID_CONTEXT_UNINITIALIZED diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index f13696c8..043053ef 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2899,6 +2899,9 @@ OPENSSL_EXPORT const char *SSLeay_version(int unused); #define SSL_R_BUFFER_TOO_SMALL 272 #define SSL_R_OLD_SESSION_VERSION_NOT_RETURNED 273 #define SSL_R_OUTPUT_ALIASES_INPUT 274 +#define SSL_R_RESUMED_EMS_SESSION_WITHOUT_EMS_EXTENSION 275 +#define SSL_R_EMS_STATE_INCONSISTENT 276 +#define SSL_R_RESUMED_NON_EMS_SESSION_WITH_EMS_EXTENSION 277 #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/s3_clnt.c b/ssl/s3_clnt.c index dbfa3552..159e2d76 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -895,6 +895,19 @@ int ssl3_get_server_hello(SSL *s) { goto f_err; } + if (s->hit && + s->s3->tmp.extended_master_secret != s->session->extended_master_secret) { + al = SSL_AD_HANDSHAKE_FAILURE; + if (s->session->extended_master_secret) { + OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello, + SSL_R_RESUMED_EMS_SESSION_WITHOUT_EMS_EXTENSION); + } else { + OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello, + SSL_R_RESUMED_NON_EMS_SESSION_WITH_EMS_EXTENSION); + } + goto f_err; + } + return 1; f_err: diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 17238282..a72e17ee 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -943,15 +943,41 @@ int ssl3_get_client_hello(SSL *s) { goto err; } - /* Only resume if the session's version matches the negotiated version: - * most clients do not accept a mismatch. */ - if (session_ret == 1 && s->version == s->session->ssl_version) { - s->hit = 1; - } else { - /* No session was found or it was unacceptable. */ - if (!ssl_get_new_session(s, 1)) { - goto err; + /* The EMS state is needed when making the resumption decision, but + * extensions are not normally parsed until later. This detects the EMS + * extension for the resumption decision and it's checked against the result + * of the normal parse later in this function. */ + const uint8_t *ems_data; + size_t ems_len; + int have_extended_master_secret = + s->version != SSL3_VERSION && + SSL_early_callback_ctx_extension_get(&early_ctx, + TLSEXT_TYPE_extended_master_secret, + &ems_data, &ems_len) && + ems_len == 0; + + if (session_ret == 1) { + if (s->session->extended_master_secret && + !have_extended_master_secret) { + /* A ClientHello without EMS that attempts to resume a session with EMS + * is fatal to the connection. */ + al = SSL_AD_HANDSHAKE_FAILURE; + OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, + SSL_R_RESUMED_EMS_SESSION_WITHOUT_EMS_EXTENSION); + goto f_err; } + + s->hit = + /* Only resume if the session's version matches the negotiated version: + * most clients do not accept a mismatch. */ + s->version == s->session->ssl_version && + /* If the client offers the EMS extension, but the previous session + * didn't use it, then negotiate a new session. */ + have_extended_master_secret == s->session->extended_master_secret; + } + + if (!s->hit && !ssl_get_new_session(s, 1)) { + goto err; } if (s->ctx->dos_protection_cb != NULL && s->ctx->dos_protection_cb(&early_ctx) == 0) { @@ -1024,6 +1050,12 @@ int ssl3_get_client_hello(SSL *s) { goto f_err; } + if (have_extended_master_secret != s->s3->tmp.extended_master_secret) { + al = SSL_AD_INTERNAL_ERROR; + OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_EMS_STATE_INCONSISTENT); + goto f_err; + } + /* Given ciphers and SSL_get_ciphers, we must pick a cipher */ if (!s->hit) { if (ciphers == NULL) { diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 3e633166..2b25d35e 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -1948,18 +1948,93 @@ func addExtendedMasterSecretTests() { } } - // When a session is resumed, it should still be aware that its master - // secret was generated via EMS and thus it's safe to use tls-unique. - testCases = append(testCases, testCase{ - name: "ExtendedMasterSecret-Resume", - config: Config{ - Bugs: ProtocolBugs{ - RequireExtendedMasterSecret: true, - }, - }, - flags: []string{expectEMSFlag}, - resumeSession: true, - }) + for _, isClient := range []bool{false, true} { + for _, supportedInFirstConnection := range []bool{false, true} { + for _, supportedInResumeConnection := range []bool{false, true} { + boolToWord := func(b bool) string { + if b { + return "Yes" + } + return "No" + } + suffix := boolToWord(supportedInFirstConnection) + "To" + boolToWord(supportedInResumeConnection) + "-" + if isClient { + suffix += "Client" + } else { + suffix += "Server" + } + + supportedConfig := Config{ + Bugs: ProtocolBugs{ + RequireExtendedMasterSecret: true, + }, + } + + noSupportConfig := Config{ + Bugs: ProtocolBugs{ + NoExtendedMasterSecret: true, + }, + } + + test := testCase{ + name: "ExtendedMasterSecret-" + suffix, + resumeSession: true, + } + + if !isClient { + test.testType = serverTest + } + + if supportedInFirstConnection { + test.config = supportedConfig + } else { + test.config = noSupportConfig + } + + if supportedInResumeConnection { + test.resumeConfig = &supportedConfig + } else { + test.resumeConfig = &noSupportConfig + } + + switch suffix { + case "YesToYes-Client", "YesToYes-Server": + // When a session is resumed, it should + // still be aware that its master + // secret was generated via EMS and + // thus it's safe to use tls-unique. + test.flags = []string{expectEMSFlag} + case "NoToYes-Server": + // If an original connection did not + // contain EMS, but a resumption + // handshake does, then a server should + // not resume the session. + test.expectResumeRejected = true + case "YesToNo-Server": + // Resuming an EMS session without the + // EMS extension should cause the + // server to abort the connection. + test.shouldFail = true + test.expectedError = ":RESUMED_EMS_SESSION_WITHOUT_EMS_EXTENSION:" + case "NoToYes-Client": + // A client should abort a connection + // where the server resumed a non-EMS + // session but echoed the EMS + // extension. + test.shouldFail = true + test.expectedError = ":RESUMED_NON_EMS_SESSION_WITH_EMS_EXTENSION:" + case "YesToNo-Client": + // A client should abort a connection + // where the server didn't echo EMS + // when the session used it. + test.shouldFail = true + test.expectedError = ":RESUMED_EMS_SESSION_WITHOUT_EMS_EXTENSION:" + } + + testCases = append(testCases, test) + } + } + } } // Adds tests that try to cover the range of the handshake state machine, under