Bug 1311213 - Handle repeated NST messages correctly r=ekr,mt

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

gtests/nss_bogo_shim/config.json

@@ -40,12 +40,13 @@
         "ClientAuth-SHA1-Fallback-RSA":"We fail when the sig_algs_ext is empty",
         "Downgrade-TLS12-*":"NSS implements downgrade detection",
         "TrailingMessageData-*": "Bug 1304575",
-        "DuplicateKeyShares":"Bug 1304578"
+        "DuplicateKeyShares":"Bug 1304578",
+        "Resume-Server-TLS13-TLS13":"Bug 1314351"
     },
     "ErrorMap" : {
         ":HANDSHAKE_FAILURE_ON_CLIENT_HELLO:":"SSL_ERROR_NO_CYPHER_OVERLAP",
         ":UNKNOWN_CIPHER_RETURNED:":"SSL_ERROR_NO_CYPHER_OVERLAP",
-        ":OLD_SESSION_CIPHER_NOT_RETURNED:":"SSL_ERROR_NO_CYPHER_OVERLAP",
+        ":OLD_SESSION_CIPHER_NOT_RETURNED:":"SSL_ERROR_RX_MALFORMED_SERVER_HELLO",
         ":NO_SHARED_CIPHER:":"SSL_ERROR_NO_CYPHER_OVERLAP",
         ":DIGEST_CHECK_FAILED:":"SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE"
     }

gtests/nss_bogo_shim/nss_bogo_shim.cc

@@ -265,7 +265,7 @@ std::unique_ptr<const Config> ReadConfig(int argc, char** argv) {
 
   cfg->AddEntry<int>("port", 0);
   cfg->AddEntry<bool>("server", false);
-  cfg->AddEntry<bool>("resume", false);
+  cfg->AddEntry<int>("resume-count", 0);
   cfg->AddEntry<std::string>("key-file", "");
   cfg->AddEntry<std::string>("cert-file", "");
 
@@ -321,7 +321,8 @@ int main(int argc, char** argv) {
   // Run a single test cycle.
   bool success = RunCycle(cfg);
 
-  if (success && cfg->get<bool>("resume")) {
+  int resume_count = cfg->get<int>("resume-count");
+  while (success && resume_count-- > 0) {
     std::cout << "Resuming" << std::endl;
     success = RunCycle(cfg);
   }

gtests/ssl_gtest/libssl_internals.c

@@ -210,6 +210,26 @@ PRBool SSLInt_SendAlert(PRFileDesc *fd, uint8_t level, uint8_t type) {
   return PR_TRUE;
 }
 
+PRBool SSLInt_SendNewSessionTicket(PRFileDesc *fd) {
+  sslSocket *ss = ssl_FindSocket(fd);
+  if (!ss) {
+    return PR_FALSE;
+  }
+
+  ssl_GetSSL3HandshakeLock(ss);
+  ssl_GetXmitBufLock(ss);
+
+  SECStatus rv = tls13_SendNewSessionTicket(ss);
+  if (rv == SECSuccess) {
+    rv = ssl3_FlushHandshake(ss, 0);
+  }
+
+  ssl_ReleaseXmitBufLock(ss);
+  ssl_ReleaseSSL3HandshakeLock(ss);
+
+  return rv == SECSuccess;
+}
+
 SECStatus SSLInt_AdvanceReadSeqNum(PRFileDesc *fd, PRUint64 to) {
   PRUint64 epoch;
   sslSocket *ss;

gtests/ssl_gtest/libssl_internals.h

@@ -31,6 +31,7 @@ PRBool SSLInt_DamageEarlyTrafficSecret(PRFileDesc *fd);
 SECStatus SSLInt_Set0RttAlpn(PRFileDesc *fd, PRUint8 *data, unsigned int len);
 PRBool SSLInt_HasCertWithAuthType(PRFileDesc *fd, SSLAuthType authType);
 PRBool SSLInt_SendAlert(PRFileDesc *fd, uint8_t level, uint8_t type);
+PRBool SSLInt_SendNewSessionTicket(PRFileDesc *fd);
 SECStatus SSLInt_AdvanceWriteSeqNum(PRFileDesc *fd, PRUint64 to);
 SECStatus SSLInt_AdvanceReadSeqNum(PRFileDesc *fd, PRUint64 to);
 SECStatus SSLInt_AdvanceWriteSeqByAWindow(PRFileDesc *fd, PRInt32 extra);

gtests/ssl_gtest/ssl_resumption_unittest.cc

@@ -557,4 +557,26 @@ TEST_F(TlsConnectTest, TestTls13ResumptionTwice) {
   ASSERT_NE(initialTicket, c2->extension());
 }
 
+// Check that resumption works after receiving two NST messages.
+TEST_F(TlsConnectTest, TestTls13ResumptionDuplicateNST) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  Connect();
+
+  // Clear the session ticket keys to invalidate the old ticket.
+  SSLInt_ClearSessionTicketKey();
+  SSLInt_SendNewSessionTicket(server_->ssl_fd());
+
+  SendReceive();  // Need to read so that we absorb the session tickets.
+  CheckKeys();
+
+  // Resume the connection.
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
+  ExpectResumption(RESUME_TICKET);
+  Connect();
+  SendReceive();
+}
+
 }  // namespace nss_test

lib/ssl/tls13con.c

@@ -87,7 +87,6 @@ static SECStatus tls13_ClientHandleFinished(sslSocket *ss,
 static SECStatus tls13_ServerHandleFinished(sslSocket *ss,
                                             SSL3Opaque *b, PRUint32 length,
                                             const TLS13CombinedHash *hashes);
-static SECStatus tls13_SendNewSessionTicket(sslSocket *ss);
 static SECStatus tls13_HandleNewSessionTicket(sslSocket *ss, SSL3Opaque *b,
                                               PRUint32 length);
 static void
@@ -3354,10 +3353,7 @@ tls13_ServerHandleFinished(sslSocket *ss, SSL3Opaque *b, PRUint32 length,
             return SECFailure; /* Error code and alerts handled below */
         }
         ssl_GetXmitBufLock(ss);
-        if (ss->opt.enableSessionTickets &&
-            ss->ssl3.hs.kea_def->authKeyType != ssl_auth_psk) {
-            /* TODO(ekr@rtfm.com): Add support for new tickets in PSK
-             * (bug 1281034).*/
+        if (ss->opt.enableSessionTickets) {
             rv = tls13_SendNewSessionTicket(ss);
             if (rv != SECSuccess) {
                 ssl_ReleaseXmitBufLock(ss);
@@ -3541,7 +3537,7 @@ tls13_SendClientSecondRound(sslSocket *ss)
  *       TicketExtension extensions<0..2^16-2>;
  *   } NewSessionTicket;
  */
-static SECStatus
+SECStatus
 tls13_SendNewSessionTicket(sslSocket *ss)
 {
     PRUint16 message_length;
@@ -3726,10 +3722,6 @@ tls13_HandleNewSessionTicket(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
         return SECFailure;
     }
 
-    /* TODO(ekr@rtfm.com): Re-enable new tickets when PSK mode is
-     * in use. I believe this works, but I can't test it until the
-     * server side supports it. Bug 1257047.
-     */
     if (!ss->opt.noCache) {
         PORT_Assert(ss->sec.ci.sid);
 
@@ -3754,13 +3746,24 @@ tls13_HandleNewSessionTicket(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
         /* Replace a previous session ticket when
          * we receive a second NewSessionTicket message. */
         if (ss->sec.ci.sid->cached == in_client_cache) {
-            /* Uncache first. */
-            ss->sec.uncache(ss->sec.ci.sid);
+            /* Create a new session ID. */
+            sslSessionID *sid = ssl3_NewSessionID(ss, PR_FALSE);
+            if (!sid) {
+                return SECFailure;
+            }
 
-            /* Then destroy and rebuild the SID. */
+            /* Copy over the peerCert. */
+            PORT_Assert(ss->sec.ci.sid->peerCert);
+            sid->peerCert = CERT_DupCertificate(ss->sec.ci.sid->peerCert);
+            if (!sid->peerCert) {
+                ssl_FreeSID(sid);
+                return SECFailure;
+            }
+
+            /* Destroy the old SID. */
+            ss->sec.uncache(ss->sec.ci.sid);
             ssl_FreeSID(ss->sec.ci.sid);
-            ss->sec.ci.sid = ssl3_NewSessionID(ss, PR_FALSE);
-            ss->sec.ci.sid->cached = never_cached;
+            ss->sec.ci.sid = sid;
         }
 
         ssl3_SetSIDSessionTicket(ss->sec.ci.sid, &ticket);

lib/ssl/tls13con.h

@@ -74,5 +74,6 @@ PRUint16 tls13_EncodeDraftVersion(SSL3ProtocolVersion version);
 PRUint16 tls13_DecodeDraftVersion(PRUint16 version);
 SECStatus tls13_NegotiateVersion(sslSocket *ss,
                                  const TLSExtension *supported_versions);
+SECStatus tls13_SendNewSessionTicket(sslSocket *ss);
 
 #endif /* __tls13con_h_ */