From e6a7d3bdfbee14340502f3113fbc935f3288fa55 Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Thu, 29 Nov 2018 18:13:10 +0100 Subject: [PATCH] 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 --- gtests/ssl_gtest/ssl_custext_unittest.cc | 8 +++---- gtests/ssl_gtest/ssl_extension_unittest.cc | 18 ++++++++-------- gtests/ssl_gtest/ssl_hrr_unittest.cc | 4 ++-- gtests/ssl_gtest/ssl_loopback_unittest.cc | 22 +++++++++++++++++++- gtests/ssl_gtest/ssl_resumption_unittest.cc | 8 +++---- gtests/ssl_gtest/ssl_tls13compat_unittest.cc | 17 +++++++++++++-- lib/ssl/SSLerrs.h | 3 +++ lib/ssl/sslerr.h | 1 + lib/ssl/tls13con.c | 22 ++++++++++---------- 9 files changed, 70 insertions(+), 33 deletions(-) diff --git a/gtests/ssl_gtest/ssl_custext_unittest.cc b/gtests/ssl_gtest/ssl_custext_unittest.cc index 5be62e5065..68c789a383 100644 --- a/gtests/ssl_gtest/ssl_custext_unittest.cc +++ b/gtests/ssl_gtest/ssl_custext_unittest.cc @@ -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(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(); } diff --git a/gtests/ssl_gtest/ssl_extension_unittest.cc b/gtests/ssl_gtest/ssl_extension_unittest.cc index ed0a000752..5819af7468 100644 --- a/gtests/ssl_gtest/ssl_extension_unittest.cc +++ b/gtests/ssl_gtest/ssl_extension_unittest.cc @@ -563,10 +563,10 @@ TEST_F(TlsExtensionTest13Stream, DropServerKeyShare) { EnsureTlsSetup(); MakeTlsFilter(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(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(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(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(); } diff --git a/gtests/ssl_gtest/ssl_hrr_unittest.cc b/gtests/ssl_gtest/ssl_hrr_unittest.cc index 6cb74f655e..27bc036547 100644 --- a/gtests/ssl_gtest/ssl_hrr_unittest.cc +++ b/gtests/ssl_gtest/ssl_hrr_unittest.cc @@ -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 diff --git a/gtests/ssl_gtest/ssl_loopback_unittest.cc b/gtests/ssl_gtest/ssl_loopback_unittest.cc index fa63fd900c..12c2496a6e 100644 --- a/gtests/ssl_gtest/ssl_loopback_unittest.cc +++ b/gtests/ssl_gtest/ssl_loopback_unittest.cc @@ -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(); diff --git a/gtests/ssl_gtest/ssl_resumption_unittest.cc b/gtests/ssl_gtest/ssl_resumption_unittest.cc index f9588bcf18..264bde67f6 100644 --- a/gtests/ssl_gtest/ssl_resumption_unittest.cc +++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc @@ -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) { diff --git a/gtests/ssl_gtest/ssl_tls13compat_unittest.cc b/gtests/ssl_gtest/ssl_tls13compat_unittest.cc index b6166b52ea..ecb63d4764 100644 --- a/gtests/ssl_gtest/ssl_tls13compat_unittest.cc +++ b/gtests/ssl_gtest/ssl_tls13compat_unittest.cc @@ -299,11 +299,11 @@ TEST_F(TlsConnectTest, TLS13NonCompatModeSessionID) { MakeTlsFilter(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); diff --git a/lib/ssl/SSLerrs.h b/lib/ssl/SSLerrs.h index dda6dc2fde..9be2194949 100644 --- a/lib/ssl/SSLerrs.h +++ b/lib/ssl/SSLerrs.h @@ -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.") diff --git a/lib/ssl/sslerr.h b/lib/ssl/sslerr.h index 98247afb73..a4aa276576 100644 --- a/lib/ssl/sslerr.h +++ b/lib/ssl/sslerr.h @@ -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 */ diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c index c0cfa5e5a2..461cd2eb9c 100644 --- a/lib/ssl/tls13con.c +++ b/lib/ssl/tls13con.c @@ -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; }