# HG changeset patch # User Daiki Ueno # Date 1603691171 -3600 # Node ID b03a4fc5b902498414b02640dcb2717dfef9682f # Parent 6f79a76958129dc09c353c288f115fd9a51ab7d4 Bug 1672703, always tolerate the first CCS in TLS 1.3, r=mt Summary: This flips the meaning of the flag for checking excessive CCS messages, so it only rejects multiple CCS messages while the first CCS message is always accepted. Reviewers: mt Reviewed By: mt Bug #: 1672703 Differential Revision: https://phabricator.services.mozilla.com/D94603 --- a/gtests/ssl_gtest/ssl_tls13compat_unittest.cc +++ b/gtests/ssl_gtest/ssl_tls13compat_unittest.cc @@ -343,29 +343,28 @@ TEST_F(TlsConnectStreamTls13, ChangeCiph // Client sends CCS before starting the handshake. client_->SendDirect(DataBuffer(kCannedCcs, sizeof(kCannedCcs))); client_->SendDirect(DataBuffer(kCannedCcs, sizeof(kCannedCcs))); ConnectExpectAlert(server_, kTlsAlertUnexpectedMessage); server_->CheckErrorCode(SSL_ERROR_RX_UNEXPECTED_CHANGE_CIPHER); client_->CheckErrorCode(SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT); } -// The server rejects a ChangeCipherSpec if the client advertises an -// empty session ID. +// The server accepts a ChangeCipherSpec even if the client advertises +// an empty session ID. TEST_F(TlsConnectStreamTls13, ChangeCipherSpecAfterClientHelloEmptySid) { EnsureTlsSetup(); ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); StartConnect(); client_->Handshake(); // Send ClientHello client_->SendDirect(DataBuffer(kCannedCcs, sizeof(kCannedCcs))); // Send CCS - server_->ExpectSendAlert(kTlsAlertUnexpectedMessage); - server_->Handshake(); // Consume ClientHello and CCS - server_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER); + Handshake(); + CheckConnected(); } // The server rejects multiple ChangeCipherSpec even if the client // indicates compatibility mode with non-empty session ID. TEST_F(Tls13CompatTest, ChangeCipherSpecAfterClientHelloTwice) { EnsureTlsSetup(); ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); EnableCompatMode(); @@ -376,36 +375,37 @@ TEST_F(Tls13CompatTest, ChangeCipherSpec client_->SendDirect(DataBuffer(kCannedCcs, sizeof(kCannedCcs))); client_->SendDirect(DataBuffer(kCannedCcs, sizeof(kCannedCcs))); server_->ExpectSendAlert(kTlsAlertUnexpectedMessage); server_->Handshake(); // Consume ClientHello and CCS. server_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER); } -// The client rejects a ChangeCipherSpec if it advertises an empty +// The client accepts a ChangeCipherSpec even if it advertises an empty // session ID. TEST_F(TlsConnectStreamTls13, ChangeCipherSpecAfterServerHelloEmptySid) { EnsureTlsSetup(); ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); // To replace Finished with a CCS below auto filter = MakeTlsFilter(server_); filter->SetHandshakeTypes({kTlsHandshakeFinished}); filter->EnableDecryption(); StartConnect(); client_->Handshake(); // Send ClientHello server_->Handshake(); // Consume ClientHello, and // send ServerHello..CertificateVerify // Send CCS server_->SendDirect(DataBuffer(kCannedCcs, sizeof(kCannedCcs))); - client_->ExpectSendAlert(kTlsAlertUnexpectedMessage); - client_->Handshake(); // Consume ClientHello and CCS - client_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER); + + // No alert is sent from the client. As Finished is dropped, we + // can't use Handshake() and CheckConnected(). + client_->Handshake(); } // The client rejects multiple ChangeCipherSpec in a row even if the // client indicates compatibility mode with non-empty session ID. TEST_F(Tls13CompatTest, ChangeCipherSpecAfterServerHelloTwice) { EnsureTlsSetup(); ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); EnableCompatMode(); --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -6640,21 +6640,17 @@ ssl_CheckServerSessionIdCorrectness(sslS if (sentFakeSid) { return !sidMatch; } return PR_TRUE; } /* TLS 1.3: We sent a session ID. The server's should match. */ if (!IS_DTLS(ss) && (sentRealSid || sentFakeSid)) { - if (sidMatch) { - ss->ssl3.hs.allowCcs = PR_TRUE; - return PR_TRUE; - } - return PR_FALSE; + return sidMatch; } /* TLS 1.3 (no SID)/DTLS 1.3: The server shouldn't send a session ID. */ return sidBytes->len == 0; } static SECStatus ssl_CheckServerRandom(sslSocket *ss) @@ -8691,17 +8687,16 @@ ssl3_HandleClientHello(sslSocket *ss, PR if (sidBytes.len > 0 && !IS_DTLS(ss)) { SECITEM_FreeItem(&ss->ssl3.hs.fakeSid, PR_FALSE); rv = SECITEM_CopyItem(NULL, &ss->ssl3.hs.fakeSid, &sidBytes); if (rv != SECSuccess) { desc = internal_error; errCode = PORT_GetError(); goto alert_loser; } - ss->ssl3.hs.allowCcs = PR_TRUE; } /* TLS 1.3 requires that compression include only null. */ if (comps.len != 1 || comps.data[0] != ssl_compression_null) { goto alert_loser; } /* If there is a cookie, then this is a second ClientHello (TLS 1.3). */ @@ -13061,25 +13056,24 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Cip * will fail if the server fails to negotiate compatibility mode in a * 0-RTT session that is resumed from a session that did negotiate it. * We don't care about that corner case right now. */ if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3 && cText->hdr[0] == ssl_ct_change_cipher_spec && ss->ssl3.hs.ws != idle_handshake && cText->buf->len == 1 && cText->buf->buf[0] == change_cipher_spec_choice) { - if (ss->ssl3.hs.allowCcs) { - /* Ignore the first CCS. */ - ss->ssl3.hs.allowCcs = PR_FALSE; + if (!ss->ssl3.hs.rejectCcs) { + /* Allow only the first CCS. */ + ss->ssl3.hs.rejectCcs = PR_TRUE; return SECSuccess; - } - - /* Compatibility mode is not negotiated. */ - alert = unexpected_message; - PORT_SetError(SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER); + } else { + alert = unexpected_message; + PORT_SetError(SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER); + } } if ((IS_DTLS(ss) && !dtls13_AeadLimitReached(spec)) || (!IS_DTLS(ss) && ss->sec.isServer && ss->ssl3.hs.zeroRttIgnore == ssl_0rtt_ignore_trial)) { /* Silently drop the packet unless we sent a fatal alert. */ if (ss->ssl3.fatalAlertSent) { return SECFailure; --- a/lib/ssl/sslimpl.h +++ b/lib/ssl/sslimpl.h @@ -705,20 +705,17 @@ typedef struct SSL3HandshakeStateStr { sslZeroRttIgnore zeroRttIgnore; /* Are we ignoring 0-RTT? */ ssl3CipherSuite zeroRttSuite; /* The cipher suite we used for 0-RTT. */ PRCList bufferedEarlyData; /* Buffered TLS 1.3 early data * on server.*/ PRBool helloRetry; /* True if HelloRetryRequest has been sent * or received. */ PRBool receivedCcs; /* A server received ChangeCipherSpec * before the handshake started. */ - PRBool allowCcs; /* A server allows ChangeCipherSpec - * as the middlebox compatibility mode - * is explicitly indicarted by - * legacy_session_id in TLS 1.3 ClientHello. */ + PRBool rejectCcs; /* Excessive ChangeCipherSpecs are rejected. */ PRBool clientCertRequested; /* True if CertificateRequest received. */ PRBool endOfFlight; /* Processed a full flight (DTLS 1.3). */ ssl3KEADef kea_def_mutable; /* Used to hold the writable kea_def * we use for TLS 1.3 */ PRUint16 ticketNonce; /* A counter we use for tickets. */ SECItem fakeSid; /* ... (server) the SID the client used. */ /* rttEstimate is used to guess the round trip time between server and client.