diff options
-rw-r--r-- | gtests/ssl_gtest/ssl_record_unittest.cc | 238 | ||||
-rw-r--r-- | gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc | 8 | ||||
-rw-r--r-- | lib/ssl/ssl3con.c | 19 | ||||
-rw-r--r-- | lib/ssl/ssl3gthr.c | 10 | ||||
-rw-r--r-- | lib/ssl/tls13con.c | 9 |
5 files changed, 256 insertions, 28 deletions
diff --git a/gtests/ssl_gtest/ssl_record_unittest.cc b/gtests/ssl_gtest/ssl_record_unittest.cc index 6e06657be..548952b06 100644 --- a/gtests/ssl_gtest/ssl_record_unittest.cc +++ b/gtests/ssl_gtest/ssl_record_unittest.cc @@ -310,22 +310,16 @@ auto kTrueFalse = ::testing::ValuesIn(kTrueFalseArr); INSTANTIATE_TEST_SUITE_P(TlsPadding, TlsPaddingTest, ::testing::Combine(kContentSizes, kTrueFalse)); -/* This filter modifies any application data record, changes its inner content - * type to the specified one (default handshake) and optionally add padding, - * to create records/fragments invalid in TLS 1.3. - * - * Implementations MUST NOT send Handshake and Alert records that have a - * zero-length TLSInnerPlaintext.content; if such a message is received, - * the receiving implementation MUST terminate the connection with an - * "unexpected_message" alert [RFC8446, Section 5.4]. - * - * !!! TLS 1.3 ONLY !!! */ -class ZeroLengthInnerPlaintextCreatorFilter : public TlsRecordFilter { +/* Filter to modify record header and content */ +class Tls13RecordModifier : public TlsRecordFilter { public: - ZeroLengthInnerPlaintextCreatorFilter( - const std::shared_ptr<TlsAgent>& a, - SSLContentType contentType = ssl_ct_handshake, size_t padding = 0) - : TlsRecordFilter(a), contentType_(contentType), padding_(padding) {} + Tls13RecordModifier(const std::shared_ptr<TlsAgent>& a, + uint8_t contentType = ssl_ct_handshake, size_t size = 0, + size_t padding = 0) + : TlsRecordFilter(a), + contentType_(contentType), + size_(size), + padding_(padding) {} protected: PacketFilter::Action FilterRecord(const TlsRecordHeader& header, @@ -350,7 +344,7 @@ class ZeroLengthInnerPlaintextCreatorFilter : public TlsRecordFilter { DataBuffer ciphertext; bool ok = Protect(spec(protection_epoch), out_header, contentType_, - DataBuffer(0), &ciphertext, &out_header, padding_); + DataBuffer(size_), &ciphertext, &out_header, padding_); EXPECT_TRUE(ok); if (!ok) { return KEEP; @@ -361,7 +355,8 @@ class ZeroLengthInnerPlaintextCreatorFilter : public TlsRecordFilter { } private: - SSLContentType contentType_; + uint8_t contentType_; + size_t size_; size_t padding_; }; @@ -389,13 +384,18 @@ class ZeroLengthInnerPlaintextSetupTls13 }; /* Test correct rejection of TLS 1.3 encrypted handshake/alert records with - * zero-length inner plaintext content length with and without padding. */ + * zero-length inner plaintext content length with and without padding. + * + * Implementations MUST NOT send Handshake and Alert records that have a + * zero-length TLSInnerPlaintext.content; if such a message is received, + * the receiving implementation MUST terminate the connection with an + * "unexpected_message" alert [RFC8446, Section 5.4]. */ TEST_P(ZeroLengthInnerPlaintextSetupTls13, ZeroLengthInnerPlaintextRun) { EnsureTlsSetup(); // Filter modifies record to be zero-length - auto filter = MakeTlsFilter<ZeroLengthInnerPlaintextCreatorFilter>( - client_, contentType_, padding_); + auto filter = + MakeTlsFilter<Tls13RecordModifier>(client_, contentType_, 0, padding_); filter->EnableDecryption(); filter->Disable(); @@ -603,4 +603,202 @@ INSTANTIATE_TEST_SUITE_P( return variant + "ZeroLength" + contentType + "Test"; }); +/* Test correct handling of records with invalid content types. + * + * TLS: + * If a TLS implementation receives an unexpected record type, it MUST + * terminate the connection with an "unexpected_message" alert + * [RFC8446, Section 5]. + * + * DTLS: + * In general, invalid records SHOULD be silently discarded... + * [RFC6347, Section 4.1.2.7]. */ +class UndefinedContentTypeSetup : public TlsConnectGeneric { + public: + UndefinedContentTypeSetup() : TlsConnectGeneric() { StartConnect(); }; + + void createUndefinedContentTypeRecord(DataBuffer& buffer, unsigned epoch = 0, + unsigned seqn = 0) { + // dummy data + uint8_t data[] = {0xAA, 0xBB, 0xCC, 0xDD, 0xEE}; + + size_t idx = 0; + // Set undefined content type + idx = buffer.Write(idx, 0xFF, 1); + // The record version is not checked during record layer handling + idx = buffer.Write(idx, 0xDEAD, 2); + // DTLS (version always < TLS 1.3) + if (variant_ == ssl_variant_datagram) { + // Set epoch (Should be 0 before/during handshake) + idx = buffer.Write(idx, epoch, 2); + // Set 6B sequence number (0 if send as first message) + idx = buffer.Write(idx, 0U, 2); + idx = buffer.Write(idx, seqn, 4); + } + // Set fragment length + idx = buffer.Write(idx, 5U, 2); + // Add data to record + (void)buffer.Write(idx, data, 5); + } + + void checkUndefinedContentTypeHandling(std::shared_ptr<TlsAgent> sender, + std::shared_ptr<TlsAgent> receiver) { + if (variant_ == ssl_variant_stream) { + // Handle record and expect alert to be sent + receiver->ExpectSendAlert(kTlsAlertUnexpectedMessage); + receiver->ReadBytes(); + /* Digest and assert that the correct alert was received at peer + * + * The 1.3 server expects all messages other than the ClientHello to be + * encrypted and responds with an unexpected message alert to alerts. */ + if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3 && sender == server_) { + sender->ExpectSendAlert(kTlsAlertUnexpectedMessage); + } else { + sender->ExpectReceiveAlert(kTlsAlertUnexpectedMessage); + } + sender->ReadBytes(); + } else { // DTLS drops invalid records silently + size_t received = receiver->received_bytes(); + receiver->ReadBytes(); + // Ensure no bytes were received/record was dropped + ASSERT_EQ(received, receiver->received_bytes()); + } + } + + protected: + DataBuffer buffer_; +}; + +INSTANTIATE_TEST_SUITE_P( + UndefinedContentTypePreHandshakeStream, UndefinedContentTypeSetup, + ::testing::Combine(TlsConnectTestBase::kTlsVariantsStream, + TlsConnectTestBase::kTlsVAll)); + +INSTANTIATE_TEST_SUITE_P( + UndefinedContentTypePreHandshakeDatagram, UndefinedContentTypeSetup, + ::testing::Combine(TlsConnectTestBase::kTlsVariantsDatagram, + TlsConnectTestBase::kTlsV11Plus)); + +TEST_P(UndefinedContentTypeSetup, + ServerReceiveUndefinedContentTypePreClientHello) { + createUndefinedContentTypeRecord(buffer_); + + // Send undefined content type record + client_->SendDirect(buffer_); + + checkUndefinedContentTypeHandling(client_, server_); +} + +TEST_P(UndefinedContentTypeSetup, + ServerReceiveUndefinedContentTypePostClientHello) { + // Set epoch to 0 (handshake), and sequence number to 1 since hello is sent + createUndefinedContentTypeRecord(buffer_, 0, 1); + + // Send ClientHello + client_->Handshake(); + // Send undefined content type record + client_->SendDirect(buffer_); + + checkUndefinedContentTypeHandling(client_, server_); +} + +TEST_P(UndefinedContentTypeSetup, + ClientReceiveUndefinedContentTypePreClientHello) { + createUndefinedContentTypeRecord(buffer_); + + // Send undefined content type record + server_->SendDirect(buffer_); + + checkUndefinedContentTypeHandling(server_, client_); +} + +TEST_P(UndefinedContentTypeSetup, + ClientReceiveUndefinedContentTypePostClientHello) { + // Set epoch to 0 (handshake), and sequence number to 1 since hello is sent + createUndefinedContentTypeRecord(buffer_, 0, 1); + + // Send ClientHello + client_->Handshake(); + // Send undefined content type record + server_->SendDirect(buffer_); + + checkUndefinedContentTypeHandling(server_, client_); +} + +class RecordOuterContentTypeSetter : public TlsRecordFilter { + public: + RecordOuterContentTypeSetter(const std::shared_ptr<TlsAgent>& a, + uint8_t contentType = ssl_ct_handshake) + : TlsRecordFilter(a), contentType_(contentType) {} + + protected: + PacketFilter::Action FilterRecord(const TlsRecordHeader& header, + const DataBuffer& record, size_t* offset, + DataBuffer* output) override { + TlsRecordHeader hdr(header.variant(), header.version(), contentType_, + header.sequence_number()); + + *offset = hdr.Write(output, *offset, record); + return CHANGE; + } + + private: + uint8_t contentType_; +}; + +/* Test correct handling of invalid inner and outer record content type. + * This is only possible for TLS 1.3, since only for this version decryption + * and encryption of manipulated records is supported by the test suite. */ +TEST_P(TlsConnectTls13, UndefinedOuterContentType13) { + EnsureTlsSetup(); + Connect(); + + // Manipulate record: set invalid content type 0xff + MakeTlsFilter<RecordOuterContentTypeSetter>(client_, 0xff); + client_->SendData(50); + + if (variant_ == ssl_variant_stream) { + // Handle invalid record + server_->ExpectSendAlert(kTlsAlertUnexpectedMessage); + server_->ReadBytes(); + // Handle alert at peer + client_->ExpectReceiveAlert(kTlsAlertUnexpectedMessage); + client_->ReadBytes(); + } else { + // Make sure DTLS drops invalid record silently + size_t received = server_->received_bytes(); + server_->ReadBytes(); + ASSERT_EQ(received, server_->received_bytes()); + } +} + +TEST_P(TlsConnectTls13, UndefinedInnerContentType13) { + EnsureTlsSetup(); + + // Manipulate record: set invalid content type 0xff and length to 50. + auto filter = MakeTlsFilter<Tls13RecordModifier>(client_, 0xff, 50, 0); + filter->EnableDecryption(); + filter->Disable(); + + Connect(); + + filter->Enable(); + // Send manipulate record with invalid content type + client_->SendData(50); + + if (variant_ == ssl_variant_stream) { + // Handle invalid record + server_->ExpectSendAlert(kTlsAlertUnexpectedMessage); + server_->ReadBytes(); + // Handle alert at peer + client_->ExpectReceiveAlert(kTlsAlertUnexpectedMessage); + client_->ReadBytes(); + } else { + // Make sure DTLS drops invalid record silently + size_t received = server_->received_bytes(); + server_->ReadBytes(); + ASSERT_EQ(received, server_->received_bytes()); + } +} + } // namespace nss_test
\ No newline at end of file diff --git a/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc b/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc index 373ce54d0..9aa6542d6 100644 --- a/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc +++ b/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc @@ -214,7 +214,7 @@ TEST_P(SSLv2ClientHelloTest, ConnectDisabled) { // But to be certain, feed in more data to see if an error comes out. uint8_t zeros[SSL_LIBRARY_VERSION_TLS_1_2] = {0}; client_->SendDirect(DataBuffer(zeros, sizeof(zeros))); - ExpectAlert(server_, kTlsAlertIllegalParameter); + ExpectAlert(server_, kTlsAlertUnexpectedMessage); server_->Handshake(); client_->Handshake(); } @@ -237,8 +237,8 @@ TEST_P(SSLv2ClientHelloTest, ConnectAfterEmptyV3Record) { // as the record length. SetPadding(255); - ConnectExpectAlert(server_, kTlsAlertIllegalParameter); - EXPECT_EQ(SSL_ERROR_BAD_CLIENT, server_->error_code()); + ConnectExpectAlert(server_, kTlsAlertUnexpectedMessage); + EXPECT_EQ(SSL_ERROR_RX_UNKNOWN_RECORD_TYPE, server_->error_code()); } // Test negotiating TLS 1.3. @@ -277,7 +277,7 @@ TEST_P(SSLv2ClientHelloTest, SendSecurityEscape) { // Set a big padding so that the server fails instead of timing out. SetPadding(255); - ConnectExpectAlert(server_, kTlsAlertIllegalParameter); + ConnectExpectAlert(server_, kTlsAlertUnexpectedMessage); } // Invalid SSLv2 client hello padding must fail the handshake. diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c index 27847f0f9..09d7f098b 100644 --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -13197,10 +13197,25 @@ ssl3_HandleNonApplicationData(sslSocket *ss, SSLContentType rType, } /* Fall through. */ default: + /* If a TLS implementation receives an unexpected record type, + * it MUST terminate the connection with an "unexpected_message" + * alert [RFC8446, Section 5]. + * + * For TLS 1.3 the outer content type is checked before in + * tls13con.c/tls13_UnprotectRecord(), + * For DTLS 1.3 the outer content type is checked before in + * ssl3gthr.c/dtls_GatherData. + * The inner content types will be checked here. + * + * In DTLS generally invalid records SHOULD be silently discarded, + * no alert is sent [RFC6347, Section 4.1.2.7]. + */ + if (!IS_DTLS(ss)) { + SSL3_SendAlert(ss, alert_fatal, unexpected_message); + } + PORT_SetError(SSL_ERROR_RX_UNKNOWN_RECORD_TYPE); SSL_DBG(("%d: SSL3[%d]: bogus content type=%d", SSL_GETPID(), ss->fd, rType)); - PORT_SetError(SSL_ERROR_RX_UNKNOWN_RECORD_TYPE); - ssl3_DecodeError(ss); rv = SECFailure; break; } diff --git a/lib/ssl/ssl3gthr.c b/lib/ssl/ssl3gthr.c index 101241f0a..08cbe7fd8 100644 --- a/lib/ssl/ssl3gthr.c +++ b/lib/ssl/ssl3gthr.c @@ -348,7 +348,15 @@ dtls_GatherData(sslSocket *ss, sslGather *gs, int flags) } else if (contentType == ssl_ct_application_data) { headerLen = 7; } else if (dtls_IsDtls13Ciphertext(ss->version, contentType)) { - /* We don't support CIDs. */ + /* We don't support CIDs. + * + * This condition is met on all invalid outer content types. + * For lower DTLS versions as well as the inner content types, + * this is checked in ssl3con.c/ssl3_HandleNonApplicationData(). + * + * In DTLS generally invalid records SHOULD be silently discarded, + * no alert is sent [RFC6347, Section 4.1.2.7]. + */ if (contentType & 0x10) { PORT_Assert(PR_FALSE); PORT_SetError(SSL_ERROR_RX_UNKNOWN_RECORD_TYPE); diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c index 8c54555d3..0188ac1d9 100644 --- a/lib/ssl/tls13con.c +++ b/lib/ssl/tls13con.c @@ -5817,7 +5817,14 @@ tls13_UnprotectRecord(sslSocket *ss, SSL_GETPID(), ss->fd, spec, spec->epoch, spec->phase, cText->seqNum, cText->buf->len)); - /* Verify that the content type is right. + /* Verify that the outer content type is right. + * + * For the inner content type as well as lower TLS versions this is checked + * in ssl3con.c/ssl3_HandleNonApllicationData(). + * + * For DTLS 1.3 this is checked in ssl3gthr.c/dtls_GatherData(). DTLS drops + * invalid records silently [RFC6347, Section 4.1.2.7]. + * * Also allow the DTLS short header in TLS 1.3. */ if (!(cText->hdr[0] == ssl_ct_application_data || (IS_DTLS(ss) && |