Bug 1320093 - Correctly Handle 1.3->1.2 downgrades when attempting re…

…sumption r=ekr

Differential Revision: https://nss-review.dev.mozaws.net/D94

gtests/ssl_gtest/ssl_0rtt_unittest.cc

@@ -200,4 +200,82 @@ TEST_P(TlsConnectTls13, TestTls13ZeroRttAlpnChangeBoth) {
   CheckAlpn("b");
 }
 
+// The client should abort the connection when sending a 0-rtt handshake but
+// the servers responds with a TLS 1.2 ServerHello. (no app data sent)
+TEST_P(TlsConnectTls13, TestTls13ZeroRttDowngrade) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  server_->Set0RttEnabled(true);  // set ticket_allow_early_data
+  Connect();
+
+  SendReceive();  // Need to read so that we absorb the session tickets.
+  CheckKeys();
+
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
+                           SSL_LIBRARY_VERSION_TLS_1_3);
+  server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
+                           SSL_LIBRARY_VERSION_TLS_1_2);
+  client_->StartConnect();
+  server_->StartConnect();
+
+  // We will send the early data xtn without sending actual early data. Thus
+  // a 1.2 server shouldn't fail until the client sends an alert because the
+  // client sends end_of_early_data only after reading the server's flight.
+  client_->Set0RttEnabled(true);
+
+  client_->Handshake();
+  server_->Handshake();
+  ASSERT_TRUE_WAIT(
+      (client_->error_code() == SSL_ERROR_RX_MALFORMED_SERVER_HELLO), 2000);
+
+  // DTLS will timeout as we bump the epoch when installing the early app data
+  // cipher suite. Thus the encrypted alert will be ignored.
+  if (mode_ == STREAM) {
+    // The client sends an encrypted alert message.
+    ASSERT_TRUE_WAIT(
+        (server_->error_code() == SSL_ERROR_RX_UNEXPECTED_APPLICATION_DATA),
+        2000);
+  }
+}
+
+// The client should abort the connection when sending a 0-rtt handshake but
+// the servers responds with a TLS 1.2 ServerHello. (with app data)
+TEST_P(TlsConnectTls13, TestTls13ZeroRttDowngradeEarlyData) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  server_->Set0RttEnabled(true);  // set ticket_allow_early_data
+  Connect();
+
+  SendReceive();  // Need to read so that we absorb the session tickets.
+  CheckKeys();
+
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
+                           SSL_LIBRARY_VERSION_TLS_1_3);
+  server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
+                           SSL_LIBRARY_VERSION_TLS_1_2);
+  client_->StartConnect();
+  server_->StartConnect();
+
+  // Send the early data xtn in the CH, followed by early app data. The server
+  // will fail right after sending its flight, when receiving the early data.
+  client_->Set0RttEnabled(true);
+  ZeroRttSendReceive(true, false);
+
+  client_->Handshake();
+  server_->Handshake();
+  ASSERT_TRUE_WAIT(
+      (client_->error_code() == SSL_ERROR_RX_MALFORMED_SERVER_HELLO), 2000);
+
+  // DTLS will timeout as we bump the epoch when installing the early app data
+  // cipher suite. Thus the encrypted alert will be ignored.
+  if (mode_ == STREAM) {
+    // The server sends an alert when receiving the early app data record.
+    ASSERT_TRUE_WAIT(
+        (server_->error_code() == SSL_ERROR_RX_UNEXPECTED_APPLICATION_DATA),
+        2000);
+  }
+}
+
 }  // namespace nss_test

gtests/ssl_gtest/ssl_resumption_unittest.cc

@@ -579,4 +579,63 @@ TEST_F(TlsConnectTest, TestTls13ResumptionDuplicateNST) {
   SendReceive();
 }
 
+TEST_F(TlsConnectTest, TestTls13ResumptionDowngrade) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  Connect();
+
+  SendReceive();  // Need to read so that we absorb the session tickets.
+  CheckKeys();
+
+  // Try resuming the connection. This will fail resuming the 1.3 session
+  // from before, but will successfully establish a 1.2 connection.
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
+                           SSL_LIBRARY_VERSION_TLS_1_3);
+  server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
+                           SSL_LIBRARY_VERSION_TLS_1_2);
+  Connect();
+
+  // Renegotiate to ensure we don't carryover any state
+  // from the 1.3 resumption attempt.
+  client_->SetExpectedVersion(SSL_LIBRARY_VERSION_TLS_1_2);
+  client_->PrepareForRenegotiate();
+  server_->StartRenegotiate();
+  Handshake();
+
+  SendReceive();
+  CheckKeys();
+}
+
+TEST_F(TlsConnectTest, TestTls13ResumptionForcedDowngrade) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  Connect();
+
+  SendReceive();  // Need to read so that we absorb the session tickets.
+  CheckKeys();
+
+  // Try resuming the connection.
+  Reset();
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  // Enable the lower version on the client.
+  client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
+                           SSL_LIBRARY_VERSION_TLS_1_3);
+
+  // Add filters that set downgrade SH.version to 1.2 and the cipher suite
+  // to one that works with 1.2, so that we don't run into early sanity checks.
+  // We will eventually fail the (sid.version == SH.version) check.
+  std::vector<PacketFilter*> filters;
+  filters.push_back(new SelectedCipherSuiteReplacer(
+      TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256));
+  filters.push_back(new SelectedVersionReplacer(SSL_LIBRARY_VERSION_TLS_1_2));
+  server_->SetPacketFilter(new ChainedPacketFilter(filters));
+
+  ConnectExpectFail();
+  client_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_SERVER_HELLO);
+  server_->CheckErrorCode(SSL_ERROR_BAD_MAC_READ);
+}
+
 }  // namespace nss_test

lib/ssl/SSLerrs.h

@@ -504,4 +504,4 @@ ER3(SSL_ERROR_MALFORMED_PSK_KEY_EXCHANGE_MODES, (SSL_ERROR_BASE + 158),
     "SSL received a malformed PSK key exchange modes extension.")
 
 ER3(SSL_ERROR_MISSING_PSK_KEY_EXCHANGE_MODES, (SSL_ERROR_BASE + 159),
-    "SSL expected a missing PSK key exchange modes extension.")
+    "SSL expected a PSK key exchange modes extension.")

lib/ssl/ssl3con.c

@@ -6629,8 +6629,11 @@ ssl3_HandleServerHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
         goto loser; /* alert has been sent */
     }
 
-    /* We got a HelloRetryRequest, but the server didn't pick 1.3.  Scream. */
-    if (ss->ssl3.hs.helloRetry && ss->version < SSL_LIBRARY_VERSION_TLS_1_3) {
+    /* The server didn't pick 1.3 although we either received a
+     * HelloRetryRequest, or we prepared to send early app data. */
+    if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3 &&
+        (ss->ssl3.hs.helloRetry || ss->ssl3.hs.zeroRttState == ssl_0rtt_sent)) {
+        /* SSL3_SendAlert() will uncache the SID. */
         desc = illegal_parameter;
         errCode = SSL_ERROR_RX_MALFORMED_SERVER_HELLO;
         goto alert_loser;
@@ -6990,6 +6993,19 @@ ssl3_HandleServerHelloPart2(sslSocket *ss, const SECItem *sidBytes,
     else
         SSL_AtomicIncrementLong(&ssl3stats.hsh_sid_cache_misses);
 
+    /* We tried to resume a 1.3 session but the server negotiated 1.2. */
+    if (ss->statelessResume) {
+        PORT_Assert(sid->version == SSL_LIBRARY_VERSION_TLS_1_3);
+        PORT_Assert(ss->ssl3.hs.currentSecret);
+
+        /* Reset resumption state, only used by 1.3 code. */
+        ss->statelessResume = PR_FALSE;
+
+        /* Clear TLS 1.3 early data traffic key. */
+        PK11_FreeSymKey(ss->ssl3.hs.currentSecret);
+        ss->ssl3.hs.currentSecret = NULL;
+    }
+
     /* throw the old one away */
     sid->u.ssl3.keys.resumable = PR_FALSE;
     ss->sec.uncache(sid);