diff -up nss/gtests/ssl_gtest/ssl_gather_unittest.cc.ssl3gthr nss/gtests/ssl_gtest/ssl_gather_unittest.cc
--- nss/gtests/ssl_gtest/ssl_gather_unittest.cc.ssl3gthr 2017-04-28 14:40:23.579583263 +0200
+++ nss/gtests/ssl_gtest/ssl_gather_unittest.cc 2017-04-28 14:40:23.579583263 +0200
@@ -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 -up nss/gtests/ssl_gtest/ssl_gtest.gyp.ssl3gthr nss/gtests/ssl_gtest/ssl_gtest.gyp
--- nss/gtests/ssl_gtest/ssl_gtest.gyp.ssl3gthr 2017-04-28 14:40:23.579583263 +0200
+++ nss/gtests/ssl_gtest/ssl_gtest.gyp 2017-04-28 14:42:07.853153503 +0200
@@ -25,6 +25,7 @@
'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',
diff -up nss/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc.ssl3gthr nss/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc
--- nss/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc.ssl3gthr 2017-04-05 14:23:56.000000000 +0200
+++ nss/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc 2017-04-28 14:40:23.579583263 +0200
@@ -202,6 +202,28 @@ TEST_P(SSLv2ClientHelloTest, Connect) {
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();
diff -up nss/lib/ssl/ssl3gthr.c.ssl3gthr nss/lib/ssl/ssl3gthr.c
--- nss/lib/ssl/ssl3gthr.c.ssl3gthr 2017-04-05 14:23:56.000000000 +0200
+++ nss/lib/ssl/ssl3gthr.c 2017-04-28 14:40:23.579583263 +0200
@@ -32,6 +32,7 @@ ssl3_InitGather(sslGather *gs)
gs->readOffset = 0;
gs->dtlsPacketOffset = 0;
gs->dtlsPacket.len = 0;
+ gs->rejectV2Records = PR_FALSE;
status = sslBuffer_Grow(&gs->buf, 4096);
return status;
}
@@ -147,8 +148,11 @@ ssl3_GatherData(sslSocket *ss, sslGather
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. */
@@ -183,7 +187,7 @@ ssl3_GatherData(sslSocket *ss, sslGather
/* 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;
@@ -205,13 +209,28 @@ ssl3_GatherData(sslSocket *ss, sslGather
* 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:
/*
@@ -219,6 +238,10 @@ ssl3_GatherData(sslSocket *ss, sslGather
*/
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;
}
diff -up nss/lib/ssl/ssldef.c.ssl3gthr nss/lib/ssl/ssldef.c
--- nss/lib/ssl/ssldef.c.ssl3gthr 2017-04-05 14:23:56.000000000 +0200
+++ nss/lib/ssl/ssldef.c 2017-04-28 14:40:23.579583263 +0200
@@ -66,6 +66,8 @@ ssl_DefRecv(sslSocket *ss, unsigned char
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
diff -up nss/lib/ssl/sslimpl.h.ssl3gthr nss/lib/ssl/sslimpl.h
--- nss/lib/ssl/sslimpl.h.ssl3gthr 2017-04-28 14:40:23.566583566 +0200
+++ nss/lib/ssl/sslimpl.h 2017-04-28 14:40:23.580583240 +0200
@@ -367,6 +367,10 @@ struct sslGatherStr {
/* 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 */