# HG changeset patch # User Tim Taubert <ttaubert@mozilla.com> # Date 1483435195 -3600 # Node ID 55ea60effd0d7c427f9b57a0bd43fb0fcbdae0e9 # Parent 316fcf7c1ca35a1d1bb8e12605cca6f9af933bde Bug 1328122 - Fix various ssl3_GatherData() issues r=mt,franziskus Differential Revision: https://nss-review.dev.mozaws.net/D135 diff --git a/gtests/ssl_gtest/manifest.mn b/gtests/ssl_gtest/manifest.mn --- a/gtests/ssl_gtest/manifest.mn +++ b/gtests/ssl_gtest/manifest.mn @@ -21,15 +21,16 @@ CPPSRCS = \ ssl_dhe_unittest.cc \ ssl_drop_unittest.cc \ ssl_ecdh_unittest.cc \ ssl_ems_unittest.cc \ ssl_exporter_unittest.cc \ ssl_extension_unittest.cc \ ssl_fuzz_unittest.cc \ + ssl_gather_unittest.cc \ ssl_gtest.cc \ ssl_hrr_unittest.cc \ ssl_loopback_unittest.cc \ ssl_record_unittest.cc \ ssl_resumption_unittest.cc \ ssl_skip_unittest.cc \ ssl_staticrsa_unittest.cc \ ssl_v2_client_hello_unittest.cc \ diff --git a/gtests/ssl_gtest/ssl_gather_unittest.cc b/gtests/ssl_gtest/ssl_gather_unittest.cc new file mode 100644 --- /dev/null +++ b/gtests/ssl_gtest/ssl_gather_unittest.cc @@ -0,0 +1,153 @@ +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* vim: set ts=2 et sw=2 tw=80: */ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "gtest_utils.h" +#include "tls_connect.h" + +namespace nss_test { + +class GatherV2ClientHelloTest : public TlsConnectTestBase { + public: + GatherV2ClientHelloTest() : TlsConnectTestBase(STREAM, 0) {} + + void ConnectExpectMalformedClientHello(const DataBuffer &data) { + EnsureTlsSetup(); + + auto alert_recorder = new TlsAlertRecorder(); + server_->SetPacketFilter(alert_recorder); + + client_->SendDirect(data); + server_->StartConnect(); + server_->Handshake(); + ASSERT_TRUE_WAIT( + (server_->error_code() == SSL_ERROR_RX_MALFORMED_CLIENT_HELLO), 2000); + + EXPECT_EQ(kTlsAlertFatal, alert_recorder->level()); + EXPECT_EQ(illegal_parameter, alert_recorder->description()); + } +}; + +// Gather a 5-byte v3 record, with a zero fragment length. The empty handshake +// message should be ignored, and the connection will succeed afterwards. +TEST_F(TlsConnectTest, GatherEmptyV3Record) { + DataBuffer buffer; + + size_t idx = 0; + idx = buffer.Write(idx, 0x16, 1); // handshake + idx = buffer.Write(idx, 0x0301, 2); // record_version + (void)buffer.Write(idx, 0U, 2); // length=0 + + EnsureTlsSetup(); + client_->SendDirect(buffer); + Connect(); +} + +// Gather a 5-byte v3 record, with a fragment length exceeding the maximum. +TEST_F(TlsConnectTest, GatherExcessiveV3Record) { + DataBuffer buffer; + + size_t idx = 0; + idx = buffer.Write(idx, 0x16, 1); // handshake + idx = buffer.Write(idx, 0x0301, 2); // record_version + (void)buffer.Write(idx, MAX_FRAGMENT_LENGTH + 2048 + 1, 2); // length=max+1 + + EnsureTlsSetup(); + auto alert_recorder = new TlsAlertRecorder(); + server_->SetPacketFilter(alert_recorder); + client_->SendDirect(buffer); + server_->StartConnect(); + server_->Handshake(); + ASSERT_TRUE_WAIT((server_->error_code() == SSL_ERROR_RX_RECORD_TOO_LONG), + 2000); + + EXPECT_EQ(kTlsAlertFatal, alert_recorder->level()); + EXPECT_EQ(record_overflow, alert_recorder->description()); +} + +// Gather a 3-byte v2 header, with a fragment length of 2. +TEST_F(GatherV2ClientHelloTest, GatherV2RecordLongHeader) { + DataBuffer buffer; + + size_t idx = 0; + idx = buffer.Write(idx, 0x0002, 2); // length=2 (long header) + idx = buffer.Write(idx, 0U, 1); // padding=0 + (void)buffer.Write(idx, 0U, 2); // data + + ConnectExpectMalformedClientHello(buffer); +} + +// Gather a 3-byte v2 header, with a fragment length of 1. +TEST_F(GatherV2ClientHelloTest, GatherV2RecordLongHeader2) { + DataBuffer buffer; + + size_t idx = 0; + idx = buffer.Write(idx, 0x0001, 2); // length=1 (long header) + idx = buffer.Write(idx, 0U, 1); // padding=0 + idx = buffer.Write(idx, 0U, 1); // data + (void)buffer.Write(idx, 0U, 1); // surplus (need 5 bytes total) + + ConnectExpectMalformedClientHello(buffer); +} + +// Gather a 3-byte v2 header, with a zero fragment length. +TEST_F(GatherV2ClientHelloTest, GatherEmptyV2RecordLongHeader) { + DataBuffer buffer; + + size_t idx = 0; + idx = buffer.Write(idx, 0U, 2); // length=0 (long header) + idx = buffer.Write(idx, 0U, 1); // padding=0 + (void)buffer.Write(idx, 0U, 2); // surplus (need 5 bytes total) + + ConnectExpectMalformedClientHello(buffer); +} + +// Gather a 2-byte v2 header, with a fragment length of 3. +TEST_F(GatherV2ClientHelloTest, GatherV2RecordShortHeader) { + DataBuffer buffer; + + size_t idx = 0; + idx = buffer.Write(idx, 0x8003, 2); // length=3 (short header) + (void)buffer.Write(idx, 0U, 3); // data + + ConnectExpectMalformedClientHello(buffer); +} + +// Gather a 2-byte v2 header, with a fragment length of 2. +TEST_F(GatherV2ClientHelloTest, GatherEmptyV2RecordShortHeader2) { + DataBuffer buffer; + + size_t idx = 0; + idx = buffer.Write(idx, 0x8002, 2); // length=2 (short header) + idx = buffer.Write(idx, 0U, 2); // data + (void)buffer.Write(idx, 0U, 1); // surplus (need 5 bytes total) + + ConnectExpectMalformedClientHello(buffer); +} + +// Gather a 2-byte v2 header, with a fragment length of 1. +TEST_F(GatherV2ClientHelloTest, GatherEmptyV2RecordShortHeader3) { + DataBuffer buffer; + + size_t idx = 0; + idx = buffer.Write(idx, 0x8001, 2); // length=1 (short header) + idx = buffer.Write(idx, 0U, 1); // data + (void)buffer.Write(idx, 0U, 2); // surplus (need 5 bytes total) + + ConnectExpectMalformedClientHello(buffer); +} + +// Gather a 2-byte v2 header, with a zero fragment length. +TEST_F(GatherV2ClientHelloTest, GatherEmptyV2RecordShortHeader) { + DataBuffer buffer; + + size_t idx = 0; + idx = buffer.Write(idx, 0x8000, 2); // length=0 (short header) + (void)buffer.Write(idx, 0U, 3); // surplus (need 5 bytes total) + + ConnectExpectMalformedClientHello(buffer); +} + +} // namespace nss_test diff --git a/gtests/ssl_gtest/ssl_gtest.gyp b/gtests/ssl_gtest/ssl_gtest.gyp --- a/gtests/ssl_gtest/ssl_gtest.gyp +++ b/gtests/ssl_gtest/ssl_gtest.gyp @@ -21,15 +21,16 @@ 'ssl_dhe_unittest.cc', 'ssl_drop_unittest.cc', 'ssl_ecdh_unittest.cc', 'ssl_ems_unittest.cc', 'ssl_exporter_unittest.cc', 'ssl_extension_unittest.cc', 'ssl_fuzz_unittest.cc', + 'ssl_gather_unittest.cc', 'ssl_gtest.cc', 'ssl_hrr_unittest.cc', 'ssl_loopback_unittest.cc', 'ssl_record_unittest.cc', 'ssl_resumption_unittest.cc', 'ssl_skip_unittest.cc', 'ssl_staticrsa_unittest.cc', 'ssl_v2_client_hello_unittest.cc', diff --git a/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc b/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc --- a/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc +++ b/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc @@ -197,16 +197,38 @@ class SSLv2ClientHelloTest : public SSLv }; // Test negotiating TLS 1.0 - 1.2. TEST_P(SSLv2ClientHelloTest, Connect) { SetAvailableCipherSuite(TLS_DHE_RSA_WITH_AES_128_CBC_SHA); Connect(); } +// Sending a v2 ClientHello after a no-op v3 record must fail. +TEST_P(SSLv2ClientHelloTest, ConnectAfterEmptyV3Record) { + DataBuffer buffer; + + size_t idx = 0; + idx = buffer.Write(idx, 0x16, 1); // handshake + idx = buffer.Write(idx, 0x0301, 2); // record_version + (void)buffer.Write(idx, 0U, 2); // length=0 + + SetAvailableCipherSuite(TLS_DHE_RSA_WITH_AES_128_CBC_SHA); + EnsureTlsSetup(); + client_->SendDirect(buffer); + + // Need padding so the connection doesn't just time out. With a v2 + // ClientHello parsed as a v3 record we will use the record version + // as the record length. + SetPadding(255); + + ConnectExpectFail(); + EXPECT_EQ(SSL_ERROR_BAD_CLIENT, server_->error_code()); +} + // Test negotiating TLS 1.3. TEST_F(SSLv2ClientHelloTestF, Connect13) { EnsureTlsSetup(); SetExpectedVersion(SSL_LIBRARY_VERSION_TLS_1_3); ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); std::vector<uint16_t> cipher_suites = {TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}; SetAvailableCipherSuites(cipher_suites); diff --git a/lib/ssl/ssl3gthr.c b/lib/ssl/ssl3gthr.c --- a/lib/ssl/ssl3gthr.c +++ b/lib/ssl/ssl3gthr.c @@ -27,16 +27,17 @@ ssl3_InitGather(sslGather *gs) { SECStatus status; gs->state = GS_INIT; gs->writeOffset = 0; gs->readOffset = 0; gs->dtlsPacketOffset = 0; gs->dtlsPacket.len = 0; + gs->rejectV2Records = PR_FALSE; status = sslBuffer_Grow(&gs->buf, 4096); return status; } /* Caller must hold RecvBufLock. */ void ssl3_DestroyGather(sslGather *gs) { @@ -142,18 +143,21 @@ ssl3_GatherData(sslSocket *ss, sslGather if (gs->remainder > 0) { continue; } /* have received entire record header, or entire record. */ switch (gs->state) { case GS_HEADER: /* Check for SSLv2 handshakes. Always assume SSLv3 on clients, - * support SSLv2 handshakes only when ssl2gs != NULL. */ - if (!ssl2gs || ssl3_isLikelyV3Hello(gs->hdr)) { + * support SSLv2 handshakes only when ssl2gs != NULL. + * Always assume v3 after we received the first record. */ + if (!ssl2gs || + ss->gs.rejectV2Records || + ssl3_isLikelyV3Hello(gs->hdr)) { /* Should have a non-SSLv2 record header in gs->hdr. Extract * the length of the following encrypted data, and then * read in the rest of the record into gs->inbuf. */ if (ss->ssl3.hs.shortHeaders) { PRUint16 len = (gs->hdr[0] << 8) | gs->hdr[1]; if (!(len & 0x8000)) { SSL_DBG(("%d: SSL3[%d]: incorrectly formatted header")); SSL3_SendAlert(ss, alert_fatal, illegal_parameter); @@ -178,17 +182,17 @@ ssl3_GatherData(sslSocket *ss, sslGather ssl2gs->padding = gs->hdr[2]; v2HdrLength++; } } /* This is the max length for an encrypted SSLv3+ fragment. */ if (!v2HdrLength && gs->remainder > (MAX_FRAGMENT_LENGTH + 2048)) { - SSL3_SendAlert(ss, alert_fatal, unexpected_message); + SSL3_SendAlert(ss, alert_fatal, record_overflow); gs->state = GS_INIT; PORT_SetError(SSL_ERROR_RX_RECORD_TOO_LONG); return SECFailure; } gs->state = GS_DATA; gs->offset = 0; gs->inbuf.len = 0; @@ -200,30 +204,49 @@ ssl3_GatherData(sslSocket *ss, sslGather } lbp = gs->inbuf.buf; } /* When we encounter an SSLv2 hello we've read 2 or 3 bytes too * many into the gs->hdr[] buffer. Copy them over into inbuf so * that we can properly process the hello record later. */ if (v2HdrLength) { + /* Reject v2 records that don't even carry enough data to + * resemble a valid ClientHello header. */ + if (gs->remainder < SSL_HL_CLIENT_HELLO_HBYTES) { + SSL3_SendAlert(ss, alert_fatal, illegal_parameter); + PORT_SetError(SSL_ERROR_RX_MALFORMED_CLIENT_HELLO); + return SECFailure; + } + + PORT_Assert(lbp); gs->inbuf.len = 5 - v2HdrLength; PORT_Memcpy(lbp, gs->hdr + v2HdrLength, gs->inbuf.len); gs->remainder -= gs->inbuf.len; lbp += gs->inbuf.len; } - break; /* End this case. Continue around the loop. */ + if (gs->remainder > 0) { + break; /* End this case. Continue around the loop. */ + } + + /* FALL THROUGH if (gs->remainder == 0) as we just received + * an empty record and there's really no point in calling + * ssl_DefRecv() with buf=NULL and len=0. */ case GS_DATA: /* ** SSL3 record has been completely received. */ SSL_TRC(10, ("%d: SSL[%d]: got record of %d bytes", SSL_GETPID(), ss->fd, gs->inbuf.len)); + + /* reject any v2 records from now on */ + ss->gs.rejectV2Records = PR_TRUE; + gs->state = GS_INIT; return 1; } } return rv; } diff --git a/lib/ssl/ssldef.c b/lib/ssl/ssldef.c --- a/lib/ssl/ssldef.c +++ b/lib/ssl/ssldef.c @@ -61,16 +61,18 @@ ssl_DefShutdown(sslSocket *ss, int how) } int ssl_DefRecv(sslSocket *ss, unsigned char *buf, int len, int flags) { PRFileDesc *lower = ss->fd->lower; int rv; + PORT_Assert(buf && len > 0); + rv = lower->methods->recv(lower, (void *)buf, len, flags, ss->rTimeout); if (rv < 0) { DEFINE_ERROR MAP_ERROR(PR_SOCKET_SHUTDOWN_ERROR, PR_CONNECT_RESET_ERROR) } else if (rv > len) { PORT_Assert(rv <= len); PORT_SetError(PR_BUFFER_OVERFLOW_ERROR); rv = SECFailure; diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h --- a/lib/ssl/sslimpl.h +++ b/lib/ssl/sslimpl.h @@ -365,16 +365,20 @@ struct sslGatherStr { */ unsigned char hdr[13]; /* Buffer for DTLS data read off the wire as a single datagram */ sslBuffer dtlsPacket; /* the start of the buffered DTLS record in dtlsPacket */ unsigned int dtlsPacketOffset; + + /* tracks whether we've seen a v3-type record before and must reject + * any further v2-type records. */ + PRBool rejectV2Records; }; /* sslGather.state */ #define GS_INIT 0 #define GS_HEADER 1 #define GS_DATA 2 /*