1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
|
commit b03a4fc5b902498414b02640dcb2717dfef9682f
Author: Daiki Ueno <dueno@redhat.com>
Date: Mon Oct 26 06:46:11 2020 +0100
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
---
gtests/ssl_gtest/ssl_tls13compat_unittest.cc | 18 +++++++++---------
lib/ssl/ssl3con.c | 20 +++++++-------------
lib/ssl/sslimpl.h | 5 +----
3 files changed, 17 insertions(+), 26 deletions(-)
diff --git gtests/ssl_gtest/ssl_tls13compat_unittest.cc gtests/ssl_gtest/ssl_tls13compat_unittest.cc
index dcede798cc..645f84ff02 100644
--- gtests/ssl_gtest/ssl_tls13compat_unittest.cc
+++ gtests/ssl_gtest/ssl_tls13compat_unittest.cc
@@ -348,8 +348,8 @@ TEST_F(TlsConnectStreamTls13, ChangeCipherSpecBeforeClientHelloTwice) {
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);
@@ -358,9 +358,8 @@ TEST_F(TlsConnectStreamTls13, ChangeCipherSpecAfterClientHelloEmptySid) {
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
@@ -381,7 +380,7 @@ TEST_F(Tls13CompatTest, ChangeCipherSpecAfterClientHelloTwice) {
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();
@@ -398,9 +397,10 @@ TEST_F(TlsConnectStreamTls13, ChangeCipherSpecAfterServerHelloEmptySid) {
// 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
diff --git lib/ssl/ssl3con.c lib/ssl/ssl3con.c
index 767ffc30f1..b652dcea34 100644
--- lib/ssl/ssl3con.c
+++ lib/ssl/ssl3con.c
@@ -6645,11 +6645,7 @@ ssl_CheckServerSessionIdCorrectness(sslSocket *ss, SECItem *sidBytes)
/* 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. */
@@ -8696,7 +8692,6 @@ ssl3_HandleClientHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
errCode = PORT_GetError();
goto alert_loser;
}
- ss->ssl3.hs.allowCcs = PR_TRUE;
}
/* TLS 1.3 requires that compression include only null. */
@@ -13066,15 +13061,14 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText)
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;
+ } else {
+ alert = unexpected_message;
+ PORT_SetError(SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER);
}
-
- /* Compatibility mode is not negotiated. */
- alert = unexpected_message;
- PORT_SetError(SSL_ERROR_RX_MALFORMED_CHANGE_CIPHER);
}
if ((IS_DTLS(ss) && !dtls13_AeadLimitReached(spec)) ||
diff --git lib/ssl/sslimpl.h lib/ssl/sslimpl.h
index 44c43a0e6c..35d0c2d6bc 100644
--- lib/ssl/sslimpl.h
+++ lib/ssl/sslimpl.h
@@ -710,10 +710,7 @@ typedef struct SSL3HandshakeStateStr {
* 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
|
