Commit e6a7d3bd authored by Daiki Ueno's avatar Daiki Ueno

Bug 1507179, reject CCS after handshake is complete in TLS 1.3, r=mt

Reviewers: mt

Reviewed By: mt

Subscribers: mt, ekr, franziskus, ueno

Tags: #secure-revision, PHID-PROJ-ffhf7tdvqze7zrdn6dh3

Bug #: 1507179

Differential Revision: https://phabricator.services.mozilla.com/D12887

--HG--
extra : amend_source : a72dffdac77d80df36076bd4951d3edbb76cf5a1
parent 924a1415
......@@ -132,7 +132,7 @@ TEST_F(TlsConnectStreamTls13, CustomExtensionEmptyWriterServer) {
// Sending extensions that the client doesn't expect leads to extensions
// appearing even if the client didn't send one, or in the wrong messages.
client_->ExpectSendAlert(kTlsAlertUnsupportedExtension);
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
ConnectExpectFail();
}
......@@ -350,7 +350,7 @@ TEST_F(TlsConnectStreamTls13, CustomExtensionUnsolicitedServer) {
auto capture = MakeTlsFilter<TlsExtensionCapture>(server_, extension_code);
client_->ExpectSendAlert(kTlsAlertUnsupportedExtension);
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
ConnectExpectFail();
EXPECT_TRUE(capture->captured());
......@@ -401,7 +401,7 @@ TEST_F(TlsConnectStreamTls13, CustomExtensionClientReject) {
EXPECT_EQ(SECSuccess, rv);
client_->ExpectSendAlert(kTlsAlertHandshakeFailure);
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
ConnectExpectFail();
}
......@@ -451,7 +451,7 @@ TEST_F(TlsConnectStreamTls13, CustomExtensionClientRejectAlert) {
EXPECT_EQ(SECSuccess, rv);
client_->ExpectSendAlert(kCustomAlert);
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
ConnectExpectFail();
}
......
......@@ -563,10 +563,10 @@ TEST_F(TlsExtensionTest13Stream, DropServerKeyShare) {
EnsureTlsSetup();
MakeTlsFilter<TlsExtensionDropper>(server_, ssl_tls13_key_share_xtn);
client_->ExpectSendAlert(kTlsAlertMissingExtension);
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
ConnectExpectFail();
EXPECT_EQ(SSL_ERROR_MISSING_KEY_SHARE, client_->error_code());
EXPECT_EQ(SSL_ERROR_BAD_MAC_READ, server_->error_code());
EXPECT_EQ(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE, server_->error_code());
}
TEST_F(TlsExtensionTest13Stream, WrongServerKeyShare) {
......@@ -583,10 +583,10 @@ TEST_F(TlsExtensionTest13Stream, WrongServerKeyShare) {
EnsureTlsSetup();
MakeTlsFilter<TlsExtensionReplacer>(server_, ssl_tls13_key_share_xtn, buf);
client_->ExpectSendAlert(kTlsAlertIllegalParameter);
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
ConnectExpectFail();
EXPECT_EQ(SSL_ERROR_RX_MALFORMED_KEY_SHARE, client_->error_code());
EXPECT_EQ(SSL_ERROR_BAD_MAC_READ, server_->error_code());
EXPECT_EQ(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE, server_->error_code());
}
TEST_F(TlsExtensionTest13Stream, UnknownServerKeyShare) {
......@@ -603,10 +603,10 @@ TEST_F(TlsExtensionTest13Stream, UnknownServerKeyShare) {
EnsureTlsSetup();
MakeTlsFilter<TlsExtensionReplacer>(server_, ssl_tls13_key_share_xtn, buf);
client_->ExpectSendAlert(kTlsAlertIllegalParameter);
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
ConnectExpectFail();
EXPECT_EQ(SSL_ERROR_RX_MALFORMED_KEY_SHARE, client_->error_code());
EXPECT_EQ(SSL_ERROR_BAD_MAC_READ, server_->error_code());
EXPECT_EQ(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE, server_->error_code());
}
TEST_F(TlsExtensionTest13Stream, AddServerSignatureAlgorithmsOnResumption) {
......@@ -615,10 +615,10 @@ TEST_F(TlsExtensionTest13Stream, AddServerSignatureAlgorithmsOnResumption) {
MakeTlsFilter<TlsExtensionInjector>(server_, ssl_signature_algorithms_xtn,
empty);
client_->ExpectSendAlert(kTlsAlertUnsupportedExtension);
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
ConnectExpectFail();
EXPECT_EQ(SSL_ERROR_EXTENSION_DISALLOWED_FOR_VERSION, client_->error_code());
EXPECT_EQ(SSL_ERROR_BAD_MAC_READ, server_->error_code());
EXPECT_EQ(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE, server_->error_code());
}
struct PskIdentity {
......@@ -1025,7 +1025,7 @@ class TlsBogusExtensionTest13 : public TlsBogusExtensionTest {
client_->ExpectSendAlert(alert);
client_->Handshake();
if (variant_ == ssl_variant_stream) {
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
}
server_->Handshake();
}
......
......@@ -924,10 +924,10 @@ TEST_F(TlsConnectStreamTls13, RetryWithDifferentCipherSuite) {
TLS_CHACHA20_POLY1305_SHA256);
client_->ExpectSendAlert(kTlsAlertIllegalParameter);
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
ConnectExpectFail();
EXPECT_EQ(SSL_ERROR_RX_MALFORMED_SERVER_HELLO, client_->error_code());
EXPECT_EQ(SSL_ERROR_BAD_MAC_READ, server_->error_code());
EXPECT_EQ(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE, server_->error_code());
}
// This tests that the second attempt at sending a ClientHello (after receiving
......
......@@ -130,7 +130,7 @@ TEST_P(TlsConnectTls13, CaptureAlertClient) {
client_->Handshake();
if (variant_ == ssl_variant_stream) {
// DTLS just drops the alert it can't decrypt.
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
}
server_->Handshake();
EXPECT_EQ(kTlsAlertFatal, alert_recorder->level());
......@@ -526,6 +526,26 @@ TEST_P(TlsConnectTls13, AlertWrongLevel) {
client_->WaitForErrorCode(SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT, 2000);
}
TEST_P(TlsConnectTls13, UnknownRecord) {
static const uint8_t kUknownRecord[] = {
0xff, SSL_LIBRARY_VERSION_TLS_1_2 >> 8,
SSL_LIBRARY_VERSION_TLS_1_2 & 0xff, 0, 0};
Connect();
if (variant_ == ssl_variant_stream) {
// DTLS just drops the record with an invalid type.
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
}
client_->SendDirect(DataBuffer(kUknownRecord, sizeof(kUknownRecord)));
server_->ExpectReadWriteError();
server_->ReadBytes();
if (variant_ == ssl_variant_stream) {
EXPECT_EQ(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE, server_->error_code());
} else {
EXPECT_EQ(SSL_ERROR_RX_UNKNOWN_RECORD_TYPE, server_->error_code());
}
}
TEST_F(TlsConnectStreamTls13, Tls13FailedWriteSecondFlight) {
EnsureTlsSetup();
StartConnect();
......
......@@ -653,7 +653,7 @@ TEST_P(TlsConnectStream, TestResumptionOverrideCipher) {
if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
client_->ExpectSendAlert(kTlsAlertIllegalParameter);
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
} else {
ExpectAlert(client_, kTlsAlertHandshakeFailure);
}
......@@ -662,7 +662,7 @@ TEST_P(TlsConnectStream, TestResumptionOverrideCipher) {
if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
// The reason this test is stream only: the server is unable to decrypt
// the alert that the client sends, see bug 1304603.
server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
server_->CheckErrorCode(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE);
} else {
server_->CheckErrorCode(SSL_ERROR_HANDSHAKE_FAILURE_ALERT);
}
......@@ -1046,10 +1046,10 @@ TEST_F(TlsConnectTest, TestTls13ResumptionForcedDowngrade) {
// client expects to receive an unencrypted TLS 1.2 Certificate message.
// The server can't decrypt the alert.
client_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
server_->ExpectSendAlert(kTlsAlertBadRecordMac); // Server can't read
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage); // Server can't read
ConnectExpectFail();
client_->CheckErrorCode(SSL_ERROR_RX_UNEXPECTED_APPLICATION_DATA);
server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
server_->CheckErrorCode(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE);
}
TEST_P(TlsConnectGenericResumption, ReConnectTicket) {
......
......@@ -299,11 +299,11 @@ TEST_F(TlsConnectTest, TLS13NonCompatModeSessionID) {
MakeTlsFilter<TlsSessionIDInjectFilter>(server_);
client_->ExpectSendAlert(kTlsAlertIllegalParameter);
server_->ExpectSendAlert(kTlsAlertBadRecordMac);
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
ConnectExpectFail();
client_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_SERVER_HELLO);
server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
server_->CheckErrorCode(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE);
}
static const uint8_t kCannedCcs[] = {
......@@ -362,6 +362,19 @@ TEST_F(TlsConnectStreamTls13, ChangeCipherSpecBeforeClientHello12) {
client_->CheckErrorCode(SSL_ERROR_HANDSHAKE_UNEXPECTED_ALERT);
}
TEST_F(TlsConnectStreamTls13, ChangeCipherSpecAfterFinished13) {
EnsureTlsSetup();
ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
Connect();
SendReceive(10);
// Client sends CCS after the handshake.
client_->SendDirect(DataBuffer(kCannedCcs, sizeof(kCannedCcs)));
server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
server_->ExpectReadWriteError();
server_->ReadBytes();
EXPECT_EQ(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE, server_->error_code());
}
TEST_F(TlsConnectDatagram13, CompatModeDtlsClient) {
EnsureTlsSetup();
client_->SetOption(SSL_ENABLE_TLS13_COMPAT_MODE, PR_TRUE);
......
......@@ -561,3 +561,6 @@ ER3(SSL_ERROR_RX_MALFORMED_ESNI_EXTENSION, (SSL_ERROR_BASE + 177),
ER3(SSL_ERROR_MISSING_ESNI_EXTENSION, (SSL_ERROR_BASE + 178),
"SSL did not receive an ESNI extension")
ER3(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE, (SSL_ERROR_BASE + 179),
"SSL received an unexpected record type.")
......@@ -267,6 +267,7 @@ typedef enum {
SSL_ERROR_RX_MALFORMED_ESNI_KEYS = (SSL_ERROR_BASE + 176),
SSL_ERROR_RX_MALFORMED_ESNI_EXTENSION = (SSL_ERROR_BASE + 177),
SSL_ERROR_MISSING_ESNI_EXTENSION = (SSL_ERROR_BASE + 178),
SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE = (SSL_ERROR_BASE + 179),
SSL_ERROR_END_OF_LIST /* let the c compiler determine the value of this. */
} SSLErrorCodes;
#endif /* NO_SECURITY_ERROR_ENUM */
......
......@@ -5016,16 +5016,6 @@ tls13_UnprotectRecord(sslSocket *ss,
SSL_GETPID(), ss->fd, spec, spec->epoch, spec->phase,
cText->seqNum, cText->buf->len));
/* We can perform this test in variable time because the record's total
* length and the ciphersuite are both public knowledge. */
if (cText->buf->len < cipher_def->tag_size) {
SSL_TRC(3,
("%d: TLS13[%d]: record too short to contain valid AEAD data",
SSL_GETPID(), ss->fd));
PORT_SetError(SSL_ERROR_BAD_MAC_READ);
return SECFailure;
}
/* Verify that the content type is right, even though we overwrite it.
* Also allow the DTLS short header in TLS 1.3. */
if (!(cText->hdr[0] == ssl_ct_application_data ||
......@@ -5035,7 +5025,17 @@ tls13_UnprotectRecord(sslSocket *ss,
SSL_TRC(3,
("%d: TLS13[%d]: record has invalid exterior type=%2.2x",
SSL_GETPID(), ss->fd, cText->hdr[0]));
/* Do we need a better error here? */
PORT_SetError(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE);
*alert = unexpected_message;
return SECFailure;
}
/* We can perform this test in variable time because the record's total
* length and the ciphersuite are both public knowledge. */
if (cText->buf->len < cipher_def->tag_size) {
SSL_TRC(3,
("%d: TLS13[%d]: record too short to contain valid AEAD data",
SSL_GETPID(), ss->fd));
PORT_SetError(SSL_ERROR_BAD_MAC_READ);
return SECFailure;
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment