You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
201 lines
8.3 KiB
201 lines
8.3 KiB
|
|
# HG changeset patch
|
|
# User Daiki Ueno <dueno@redhat.com>
|
|
# 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<TlsHandshakeDropper>(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.
|
|
|