Bug 1411475 - Google Hack, r=ekr

This makes the TLS 1.3 handshake look like TLS 1.2.

The trickiest part here is in 0-RTT.  I've chosen to remember that the
alternative handshake was used and send a ChangeCipherSpec if the previous
session used the alternative AND if the client enables the alternative.

This assumes that a server will commit to supporting - and selecting - this
alternative handshake type for as long as it supports 0-RTT from sessions that
have the alternative handshake type.  That is, if you negotiate the alternative
handshake and the server supports 0-RTT, then it will not just support TLS 1.3
for the duration of the ticket, but also the alternative handshake type.  A
client can disable the alternative handshake because the version in the
ClientHello indicates whether the client intended to send a CCS, but the server
cannot refuse to pick it if the client offers.

Of course, if we agree that the final TLS 1.3 is in this form, we don't have a
problem, it's only an issue because we need to switch-hit.

I chose to remove the Facebook alternative content type hack as all signs
indicate that it doesn't help.

8 files changed, 224 insertions, 44 deletions

diff --git a/gtests/ssl_gtest/manifest.mn b/gtests/ssl_gtest/manifest.mn
index cf29bb183..eaab28021 100644
--- a/gtests/ssl_gtest/manifest.mn
+++ b/gtests/ssl_gtest/manifest.mn
@@ -14,6 +14,7 @@ CSRCS = \
 CPPSRCS = \
       ssl_0rtt_unittest.cc \
       ssl_agent_unittest.cc \
+      ssl_alths_unittest.cc \
       ssl_auth_unittest.cc \
       ssl_cert_ext_unittest.cc \
       ssl_ciphersuite_unittest.cc \
diff --git a/gtests/ssl_gtest/ssl_alths_unittest.cc b/gtests/ssl_gtest/ssl_alths_unittest.cc
new file mode 100644
index 000000000..d0e900abd
--- /dev/null
+++ b/gtests/ssl_gtest/ssl_alths_unittest.cc
@@ -0,0 +1,178 @@
+/* -*- 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 <memory>
+#include <vector>
+#include "ssl.h"
+#include "sslerr.h"
+#include "sslproto.h"
+
+#include "gtest_utils.h"
+#include "tls_connect.h"
+#include "tls_filter.h"
+#include "tls_parser.h"
+
+namespace nss_test {
+
+static const uint32_t kServerHelloVersionAlt = SSL_LIBRARY_VERSION_TLS_1_2;
+static const uint32_t kServerHelloVersionRegular =
+    0x7f00 | TLS_1_3_DRAFT_VERSION;
+
+class AltHandshakeTest : public TlsConnectStreamTls13 {
+ protected:
+  void SetUp() {
+    TlsConnectStreamTls13::SetUp();
+    client_ccs_recorder_ =
+        std::make_shared<TlsRecordRecorder>(kTlsChangeCipherSpecType);
+    server_ccs_recorder_ =
+        std::make_shared<TlsRecordRecorder>(kTlsChangeCipherSpecType);
+    server_hello_recorder_ =
+        std::make_shared<TlsInspectorRecordHandshakeMessage>(
+            kTlsHandshakeServerHello);
+  }
+
+  void SetAltHandshakeTypeEnabled() {
+    client_->SetAltHandshakeTypeEnabled();
+    server_->SetAltHandshakeTypeEnabled();
+  }
+
+  void InstallFilters() {
+    client_->SetPacketFilter(client_ccs_recorder_);
+    auto chain = std::make_shared<ChainedPacketFilter>(ChainedPacketFilterInit(
+        {server_ccs_recorder_, server_hello_recorder_}));
+    server_->SetPacketFilter(chain);
+  }
+
+  void CheckServerHelloVersion(uint32_t server_hello_version) {
+    uint32_t ver;
+    ASSERT_TRUE(server_hello_recorder_->buffer().Read(0, 2, &ver));
+    ASSERT_EQ(server_hello_version, ver);
+  }
+
+  void CheckForRegularHandshake() {
+    EXPECT_EQ(0U, client_ccs_recorder_->count());
+    EXPECT_EQ(0U, server_ccs_recorder_->count());
+    CheckServerHelloVersion(kServerHelloVersionRegular);
+  }
+
+  void CheckForAltHandshake() {
+    EXPECT_EQ(1U, client_ccs_recorder_->count());
+    EXPECT_EQ(1U, server_ccs_recorder_->count());
+    CheckServerHelloVersion(kServerHelloVersionAlt);
+  }
+
+  std::shared_ptr<TlsRecordRecorder> client_ccs_recorder_;
+  std::shared_ptr<TlsRecordRecorder> server_ccs_recorder_;
+  std::shared_ptr<TlsInspectorRecordHandshakeMessage> server_hello_recorder_;
+};
+
+TEST_F(AltHandshakeTest, ClientOnly) {
+  client_->SetAltHandshakeTypeEnabled();
+  InstallFilters();
+  Connect();
+  CheckForRegularHandshake();
+}
+
+TEST_F(AltHandshakeTest, ServerOnly) {
+  server_->SetAltHandshakeTypeEnabled();
+  InstallFilters();
+  Connect();
+  CheckForRegularHandshake();
+}
+
+TEST_F(AltHandshakeTest, Enabled) {
+  SetAltHandshakeTypeEnabled();
+  InstallFilters();
+  Connect();
+  CheckForAltHandshake();
+}
+
+TEST_F(AltHandshakeTest, ZeroRtt) {
+  SetAltHandshakeTypeEnabled();
+  SetupForZeroRtt();
+  SetAltHandshakeTypeEnabled();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+
+  InstallFilters();
+
+  ExpectResumption(RESUME_TICKET);
+  ZeroRttSendReceive(true, true);
+  Handshake();
+  ExpectEarlyDataAccepted(true);
+  CheckConnected();
+
+  CheckForAltHandshake();
+}
+
+// Neither client nor server has the extension prior to resumption, so the
+// client doesn't send a CCS before its 0-RTT data.
+TEST_F(AltHandshakeTest, DisabledBeforeZeroRtt) {
+  SetupForZeroRtt();
+  SetAltHandshakeTypeEnabled();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+
+  InstallFilters();
+
+  ExpectResumption(RESUME_TICKET);
+  ZeroRttSendReceive(true, true);
+  Handshake();
+  ExpectEarlyDataAccepted(true);
+  CheckConnected();
+
+  EXPECT_EQ(0U, client_ccs_recorder_->count());
+  EXPECT_EQ(1U, server_ccs_recorder_->count());
+  CheckServerHelloVersion(kServerHelloVersionAlt);
+}
+
+// Both use the alternative in the initial handshake but only the server enables
+// it on resumption.
+TEST_F(AltHandshakeTest, ClientDisabledAfterZeroRtt) {
+  SetAltHandshakeTypeEnabled();
+  SetupForZeroRtt();
+  server_->SetAltHandshakeTypeEnabled();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+
+  InstallFilters();
+
+  ExpectResumption(RESUME_TICKET);
+  ZeroRttSendReceive(true, true);
+  Handshake();
+  ExpectEarlyDataAccepted(true);
+  CheckConnected();
+
+  CheckForRegularHandshake();
+}
+
+// If the alternative handshake isn't negotiated after 0-RTT, and the client has
+// it enabled, it will send a ChangeCipherSpec.  The server chokes on it if it
+// hasn't negotiated the alternative handshake.
+TEST_F(AltHandshakeTest, ServerDisabledAfterZeroRtt) {
+  SetAltHandshakeTypeEnabled();
+  SetupForZeroRtt();
+  client_->SetAltHandshakeTypeEnabled();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+
+  client_->ExpectSendAlert(kTlsAlertEndOfEarlyData);
+  client_->Handshake();  // Send ClientHello (and CCS)
+
+  server_->Handshake();  // Consume the ClientHello, which is OK.
+  client_->ExpectResumption();
+  client_->Handshake();  // Read the server handshake.
+  EXPECT_EQ(TlsAgent::STATE_CONNECTED, client_->state());
+
+  // Now the server reads the CCS instead of more handshake messages.
+  ExpectAlert(server_, kTlsAlertBadRecordMac);
+  server_->Handshake();
+  EXPECT_EQ(TlsAgent::STATE_ERROR, server_->state());
+  client_->Handshake();  // Consume the alert.
+  EXPECT_EQ(TlsAgent::STATE_ERROR, client_->state());
+}
+
+}  // nss_test
diff --git a/gtests/ssl_gtest/ssl_extension_unittest.cc b/gtests/ssl_gtest/ssl_extension_unittest.cc
index d15139419..159527fb2 100644
--- a/gtests/ssl_gtest/ssl_extension_unittest.cc
+++ b/gtests/ssl_gtest/ssl_extension_unittest.cc
@@ -1079,10 +1079,6 @@ TEST_P(TlsBogusExtensionTest13, AddBogusExtensionHelloRetryRequest) {
   Run(kTlsHandshakeHelloRetryRequest);
 }
 
-TEST_P(TlsBogusExtensionTest13, AddVersionExtensionServerHello) {
-  Run(kTlsHandshakeServerHello, ssl_tls13_supported_versions_xtn);
-}
-
 TEST_P(TlsBogusExtensionTest13, AddVersionExtensionEncryptedExtensions) {
   Run(kTlsHandshakeEncryptedExtensions, ssl_tls13_supported_versions_xtn);
 }
diff --git a/gtests/ssl_gtest/ssl_gtest.gyp b/gtests/ssl_gtest/ssl_gtest.gyp
index 8b024f18b..d4b77df5f 100644
--- a/gtests/ssl_gtest/ssl_gtest.gyp
+++ b/gtests/ssl_gtest/ssl_gtest.gyp
@@ -15,6 +15,7 @@
         'selfencrypt_unittest.cc',
         'ssl_0rtt_unittest.cc',
         'ssl_agent_unittest.cc',
+        'ssl_alths_unittest.cc',
         'ssl_auth_unittest.cc',
         'ssl_cert_ext_unittest.cc',
         'ssl_ciphersuite_unittest.cc',
diff --git a/gtests/ssl_gtest/ssl_loopback_unittest.cc b/gtests/ssl_gtest/ssl_loopback_unittest.cc
index 074b51630..602947cfc 100644
--- a/gtests/ssl_gtest/ssl_loopback_unittest.cc
+++ b/gtests/ssl_gtest/ssl_loopback_unittest.cc
@@ -11,7 +11,6 @@
 #include "ssl.h"
 #include "sslerr.h"
 #include "sslproto.h"
-#include "ssl3prot.h"
 
 extern "C" {
 // This is not something that should make you happy.
@@ -104,9 +103,9 @@ TEST_P(TlsConnectGeneric, CaptureAlertServer) {
   auto alert_recorder = std::make_shared<TlsAlertRecorder>();
   server_->SetPacketFilter(alert_recorder);
 
-  ConnectExpectAlert(server_, kTlsAlertIllegalParameter);
+  ConnectExpectAlert(server_, kTlsAlertDecodeError);
   EXPECT_EQ(kTlsAlertFatal, alert_recorder->level());
-  EXPECT_EQ(kTlsAlertIllegalParameter, alert_recorder->description());
+  EXPECT_EQ(kTlsAlertDecodeError, alert_recorder->description());
 }
 
 TEST_P(TlsConnectGenericPre13, CaptureAlertClient) {
@@ -307,42 +306,6 @@ TEST_F(TlsConnectStreamTls13, NegotiateShortHeaders) {
   Connect();
 }
 
-TEST_F(TlsConnectStreamTls13, ClientAltHandshakeType) {
-  client_->SetAltHandshakeTypeEnabled();
-  auto filter = std::make_shared<TlsHeaderRecorder>();
-  server_->SetPacketFilter(filter);
-  Connect();
-  ASSERT_EQ(kTlsHandshakeType, filter->header(0)->content_type());
-}
-
-TEST_F(TlsConnectStreamTls13, ServerAltHandshakeType) {
-  server_->SetAltHandshakeTypeEnabled();
-  auto filter = std::make_shared<TlsHeaderRecorder>();
-  server_->SetPacketFilter(filter);
-  Connect();
-  ASSERT_EQ(kTlsHandshakeType, filter->header(0)->content_type());
-}
-
-TEST_F(TlsConnectStreamTls13, BothAltHandshakeType) {
-  client_->SetAltHandshakeTypeEnabled();
-  server_->SetAltHandshakeTypeEnabled();
-  auto header_filter = std::make_shared<TlsHeaderRecorder>();
-  auto sh_filter = std::make_shared<TlsInspectorRecordHandshakeMessage>(
-      kTlsHandshakeServerHello);
-  std::vector<std::shared_ptr<PacketFilter>> filters = {header_filter,
-                                                        sh_filter};
-  auto chained = std::make_shared<ChainedPacketFilter>(filters);
-  server_->SetPacketFilter(chained);
-  header_filter->SetAgent(server_.get());
-  header_filter->EnableDecryption();
-  Connect();
-  ASSERT_EQ(kTlsAltHandshakeType, header_filter->header(0)->content_type());
-  ASSERT_EQ(kTlsHandshakeType, header_filter->header(1)->content_type());
-  uint32_t ver;
-  ASSERT_TRUE(sh_filter->buffer().Read(0, 2, &ver));
-  ASSERT_EQ((uint32_t)(0x7a00 | TLS_1_3_DRAFT_VERSION), ver);
-}
-
 INSTANTIATE_TEST_CASE_P(
     GenericStream, TlsConnectGeneric,
     ::testing::Combine(TlsConnectTestBase::kTlsVariantsStream,
diff --git a/gtests/ssl_gtest/tls_agent.cc b/gtests/ssl_gtest/tls_agent.cc
index e84ffc203..77b848b9c 100644
--- a/gtests/ssl_gtest/tls_agent.cc
+++ b/gtests/ssl_gtest/tls_agent.cc
@@ -387,7 +387,7 @@ void TlsAgent::SetShortHeadersEnabled() {
 void TlsAgent::SetAltHandshakeTypeEnabled() {
   EXPECT_TRUE(EnsureTlsSetup());
 
-  SECStatus rv = SSL_UseAltServerHelloType(ssl_fd(), true);
+  SECStatus rv = SSL_UseAltHandshakeType(ssl_fd(), PR_TRUE);
   EXPECT_EQ(SECSuccess, rv);
 }
 
diff --git a/gtests/ssl_gtest/tls_filter.cc b/gtests/ssl_gtest/tls_filter.cc
index 433b9bf7b..6e1a643a8 100644
--- a/gtests/ssl_gtest/tls_filter.cc
+++ b/gtests/ssl_gtest/tls_filter.cc
@@ -363,6 +363,15 @@ PacketFilter::Action TlsInspectorReplaceHandshakeMessage::FilterHandshake(
   return KEEP;
 }
 
+PacketFilter::Action TlsRecordRecorder::FilterRecord(
+    const TlsRecordHeader& header, const DataBuffer& input,
+    DataBuffer* output) {
+  if (!filter_ || (header.content_type() == ct_)) {
+    records_.push_back({header, input});
+  }
+  return KEEP;
+}
+
 PacketFilter::Action TlsConversationRecorder::FilterRecord(
     const TlsRecordHeader& header, const DataBuffer& input,
     DataBuffer* output) {
diff --git a/gtests/ssl_gtest/tls_filter.h b/gtests/ssl_gtest/tls_filter.h
index bb05664a5..7db7d76d3 100644
--- a/gtests/ssl_gtest/tls_filter.h
+++ b/gtests/ssl_gtest/tls_filter.h
@@ -63,6 +63,11 @@ class TlsRecordHeader : public TlsVersioned {
   uint64_t sequence_number_;
 };
 
+struct TlsRecord {
+  const TlsRecordHeader header;
+  const DataBuffer buffer;
+};
+
 // Abstract filter that operates on entire (D)TLS records.
 class TlsRecordFilter : public PacketFilter {
  public:
@@ -221,6 +226,28 @@ class TlsInspectorReplaceHandshakeMessage : public TlsHandshakeFilter {
   DataBuffer buffer_;
 };
 
+class TlsRecordRecorder : public TlsRecordFilter {
+ public:
+  TlsRecordRecorder(uint8_t ct) : filter_(true), ct_(ct), records_() {}
+  TlsRecordRecorder()
+      : filter_(false),
+        ct_(content_handshake),  // dummy (<optional> is C++14)
+        records_() {}
+  virtual PacketFilter::Action FilterRecord(const TlsRecordHeader& header,
+                                            const DataBuffer& input,
+                                            DataBuffer* output);
+
+  size_t count() const { return records_.size(); }
+  void Clear() { records_.clear(); }
+
+  const TlsRecord& record(size_t i) const { return records_[i]; }
+
+ private:
+  bool filter_;
+  uint8_t ct_;
+  std::vector<TlsRecord> records_;
+};
+
 // Make a copy of the complete conversation.
 class TlsConversationRecorder : public TlsRecordFilter {
  public:
@@ -247,11 +274,16 @@ class TlsHeaderRecorder : public TlsRecordFilter {
 };
 
 // Runs multiple packet filters in series.
+typedef std::initializer_list<std::shared_ptr<PacketFilter>>
+    ChainedPacketFilterInit;
+
+// Runs multiple packet filters in series.
 class ChainedPacketFilter : public PacketFilter {
  public:
   ChainedPacketFilter() {}
   ChainedPacketFilter(const std::vector<std::shared_ptr<PacketFilter>> filters)
       : filters_(filters.begin(), filters.end()) {}
+  ChainedPacketFilter(ChainedPacketFilterInit il) : filters_(il) {}
   virtual ~ChainedPacketFilter() {}
 
   virtual PacketFilter::Action Filter(const DataBuffer& input,