# 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
 
 /*