Bug 1340841 - Ticket lifetime duration is in seconds, r=ekr

9 files changed, 76 insertions, 16 deletions

diff --git a/gtests/ssl_gtest/libssl_internals.c b/gtests/ssl_gtest/libssl_internals.c
index 08af9af0e..3778d809a 100644
--- a/gtests/ssl_gtest/libssl_internals.c
+++ b/gtests/ssl_gtest/libssl_internals.c
@@ -370,3 +370,7 @@ SECStatus SSLInt_UsingShortHeaders(PRFileDesc *fd, PRBool *result) {
 
   return SECSuccess;
 }
+
+void SSLInt_SetTicketLifetime(uint32_t lifetime) {
+  ssl_ticket_lifetime = lifetime;
+}
diff --git a/gtests/ssl_gtest/libssl_internals.h b/gtests/ssl_gtest/libssl_internals.h
index 342c3040a..9058f322f 100644
--- a/gtests/ssl_gtest/libssl_internals.h
+++ b/gtests/ssl_gtest/libssl_internals.h
@@ -49,5 +49,6 @@ SSLCipherAlgorithm SSLInt_CipherSpecToAlgorithm(PRBool isServer,
 unsigned char *SSLInt_CipherSpecToIv(PRBool isServer, ssl3CipherSpec *spec);
 SECStatus SSLInt_EnableShortHeaders(PRFileDesc *fd);
 SECStatus SSLInt_UsingShortHeaders(PRFileDesc *fd, PRBool *result);
+void SSLInt_SetTicketLifetime(uint32_t lifetime);
 
 #endif  // ndef libssl_internals_h_
diff --git a/gtests/ssl_gtest/ssl_resumption_unittest.cc b/gtests/ssl_gtest/ssl_resumption_unittest.cc
index 34699b1d6..0fca3aa80 100644
--- a/gtests/ssl_gtest/ssl_resumption_unittest.cc
+++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc
@@ -201,6 +201,63 @@ TEST_P(TlsConnectGeneric, ConnectResumeClientBothTicketServerTicketForget) {
   SendReceive();
 }
 
+TEST_P(TlsConnectGeneric, ConnectWithExpiredTicketAtClient) {
+  SSLInt_SetTicketLifetime(1);  // one second
+  // This causes a ticket resumption.
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  Connect();
+  SendReceive();
+
+  WAIT_(false, 1000);
+
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ExpectResumption(RESUME_NONE);
+
+  // TLS 1.3 uses the pre-shared key extension instead.
+  SSLExtensionType xtn = (version_ >= SSL_LIBRARY_VERSION_TLS_1_3)
+                             ? ssl_tls13_pre_shared_key_xtn
+                             : ssl_session_ticket_xtn;
+  auto capture = new TlsExtensionCapture(xtn);
+  client_->SetPacketFilter(capture);
+  Connect();
+
+  if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
+    EXPECT_FALSE(capture->captured());
+  } else {
+    EXPECT_TRUE(capture->captured());
+    EXPECT_EQ(0U, capture->extension().len());
+  }
+}
+
+TEST_P(TlsConnectGeneric, ConnectWithExpiredTicketAtServer) {
+  SSLInt_SetTicketLifetime(1);  // one second
+  // This causes a ticket resumption.
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  Connect();
+  SendReceive();
+
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ExpectResumption(RESUME_NONE);
+
+  SSLExtensionType xtn = (version_ >= SSL_LIBRARY_VERSION_TLS_1_3)
+                             ? ssl_tls13_pre_shared_key_xtn
+                             : ssl_session_ticket_xtn;
+  auto capture = new TlsExtensionCapture(xtn);
+  client_->SetPacketFilter(capture);
+  client_->StartConnect();
+  server_->StartConnect();
+  client_->Handshake();
+  EXPECT_TRUE(capture->captured());
+  EXPECT_LT(0U, capture->extension().len());
+
+  WAIT_(false, 1000);  // Let the ticket expire on the server.
+
+  Handshake();
+  CheckConnected();
+}
+
 // This callback switches out the "server" cert used on the server with
 // the "client" certificate, which should be the same type.
 static int32_t SwitchCertificates(TlsAgent* agent, const SECItem* srvNameArr,
diff --git a/gtests/ssl_gtest/tls_connect.cc b/gtests/ssl_gtest/tls_connect.cc
index 2b122e5cb..52ee94ffc 100644
--- a/gtests/ssl_gtest/tls_connect.cc
+++ b/gtests/ssl_gtest/tls_connect.cc
@@ -173,6 +173,7 @@ void TlsConnectTestBase::ClearServerCache() {
 void TlsConnectTestBase::SetUp() {
   SSL_ConfigServerSessionIDCache(1024, 0, 0, g_working_dir_path.c_str());
   SSLInt_ClearSessionTicketKey();
+  SSLInt_SetTicketLifetime(10);
   ClearStats();
   Init();
 }
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index cf86ff81c..bbcd110c6 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -10256,7 +10256,7 @@ ssl3_SendNewSessionTicket(sslSocket *ss)
         goto loser;
 
     /* This is a fixed value. */
-    rv = ssl3_AppendHandshakeNumber(ss, TLS_EX_SESS_TICKET_LIFETIME_HINT, 4);
+    rv = ssl3_AppendHandshakeNumber(ss, ssl_ticket_lifetime, 4);
     if (rv != SECSuccess)
         goto loser;
 
diff --git a/lib/ssl/ssl3exthandle.c b/lib/ssl/ssl3exthandle.c
index 1cb3dcb86..8607ad848 100644
--- a/lib/ssl/ssl3exthandle.c
+++ b/lib/ssl/ssl3exthandle.c
@@ -935,6 +935,9 @@ ssl3_ClientHandleStatusRequestXtn(const sslSocket *ss, TLSExtensionData *xtnData
     return SECSuccess;
 }
 
+PRUint32 ssl_ticket_lifetime = 2 * 24 * 60 * 60; /* 2 days in seconds */
+#define TLS_EX_SESS_TICKET_VERSION (0x0103)
+
 /*
  * Called from ssl3_SendNewSessionTicket, tls13_SendNewSessionTicket
  */
@@ -1664,8 +1667,7 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, SECItem *data)
      * memory since the ticket is of no use.
      */
     if (parsed_session_ticket->timestamp != 0 &&
-        parsed_session_ticket->timestamp +
-                TLS_EX_SESS_TICKET_LIFETIME_HINT >
+        parsed_session_ticket->timestamp + ssl_ticket_lifetime >
             ssl_Time()) {
 
         sid = ssl3_NewSessionID(ss, PR_TRUE);
@@ -1721,6 +1723,7 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, SECItem *data)
                 SECITEM_FreeItem(&sid->u.ssl3.srvName, PR_FALSE);
             }
             sid->u.ssl3.srvName = parsed_session_ticket->srvName;
+            parsed_session_ticket->srvName.data = NULL;
         }
         if (parsed_session_ticket->alpnSelection.data != NULL) {
             sid->u.ssl3.alpnSelection = parsed_session_ticket->alpnSelection;
@@ -1760,6 +1763,9 @@ loser:
         if (parsed_session_ticket->alpnSelection.data) {
             SECITEM_FreeItem(&parsed_session_ticket->alpnSelection, PR_FALSE);
         }
+        if (parsed_session_ticket->srvName.data) {
+            SECITEM_FreeItem(&parsed_session_ticket->srvName, PR_FALSE);
+        }
         PORT_ZFree(parsed_session_ticket, sizeof(SessionTicket));
     }
 
diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h
index 69b461eb7..5c4e801d6 100644
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -1220,17 +1220,13 @@ struct sslSocketStr {
     SSLProtocolVariant protocolVariant;
 };
 
-/* All the global data items declared here should be protected using the
-** ssl_global_data_lock, which is a reader/writer lock.
-*/
-extern NSSRWLock *ssl_global_data_lock;
 extern char ssl_debug;
 extern char ssl_trace;
 extern FILE *ssl_trace_iob;
 extern FILE *ssl_keylog_iob;
 extern CERTDistNames *ssl3_server_ca_list;
-extern PRUint32 ssl_sid_timeout;
 extern PRUint32 ssl3_sid_timeout;
+extern PRUint32 ssl_ticket_lifetime;
 
 extern const char *const ssl3_cipherName[];
 
@@ -1700,10 +1696,6 @@ extern PRBool ssl_GetSessionTicketKeys(SECKEYPrivateKey *svrPrivKey,
                                        PK11SymKey **macKey);
 extern SECStatus ssl3_SessionTicketShutdown(void *appData, void *nssData);
 
-/* Tell clients to consider tickets valid for this long. */
-#define TLS_EX_SESS_TICKET_LIFETIME_HINT (2 * 24 * 60 * 60) /* 2 days */
-#define TLS_EX_SESS_TICKET_VERSION (0x0103)
-
 extern SECStatus ssl3_ValidateNextProtoNego(const unsigned char *data,
                                             unsigned int length);
 
diff --git a/lib/ssl/sslnonce.c b/lib/ssl/sslnonce.c
index 616901300..7ad1c6bc7 100644
--- a/lib/ssl/sslnonce.c
+++ b/lib/ssl/sslnonce.c
@@ -19,7 +19,6 @@
 #include <time.h>
 #endif
 
-PRUint32 ssl_sid_timeout = 100;
 PRUint32 ssl3_sid_timeout = 86400L; /* 24 hours */
 
 static sslSessionID *cache = NULL;
@@ -471,7 +470,7 @@ ssl_TicketTimeValid(const NewSessionTicket *ticket)
     }
 
     endTime = ticket->received_timestamp +
-              (PRTime)(ticket->ticket_lifetime_hint * PR_USEC_PER_MSEC);
+              (PRTime)(ticket->ticket_lifetime_hint * PR_USEC_PER_SEC);
     return endTime > PR_Now();
 }
 
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index 5670fbee5..f3c8686cb 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -3807,7 +3807,7 @@ tls13_SendNewSessionTicket(sslSocket *ss)
         ticket.flags |= ticket_allow_early_data;
         max_early_data_size_len = 8; /* type + len + value. */
     }
-    ticket.ticket_lifetime_hint = TLS_EX_SESS_TICKET_LIFETIME_HINT;
+    ticket.ticket_lifetime_hint = ssl_ticket_lifetime;
 
     rv = ssl3_EncodeSessionTicket(ss, &ticket, &ticket_data);
     if (rv != SECSuccess)
@@ -3826,7 +3826,7 @@ tls13_SendNewSessionTicket(sslSocket *ss)
         goto loser;
 
     /* This is a fixed value. */
-    rv = ssl3_AppendHandshakeNumber(ss, TLS_EX_SESS_TICKET_LIFETIME_HINT, 4);
+    rv = ssl3_AppendHandshakeNumber(ss, ssl_ticket_lifetime, 4);
     if (rv != SECSuccess)
         goto loser;