From 8b943813f6c61af57c3f39a61dba67552d06f992 Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Mon, 20 Feb 2017 10:02:05 +1100 Subject: [PATCH] Bug 1340841 - Ticket lifetime duration is in seconds, r=ekr --HG-- extra : source : 16f9b6c70772563079ee310ffbab91c5bcf16980 extra : amend_source : 57fc0d2054ab4061a670e05ca861d7c35bd56888 --- gtests/ssl_gtest/libssl_internals.c | 4 ++ gtests/ssl_gtest/libssl_internals.h | 1 + gtests/ssl_gtest/ssl_resumption_unittest.cc | 57 +++++++++++++++++++++ gtests/ssl_gtest/tls_connect.cc | 1 + lib/ssl/ssl3con.c | 2 +- lib/ssl/ssl3exthandle.c | 10 +++- lib/ssl/sslimpl.h | 10 +--- lib/ssl/sslnonce.c | 3 +- lib/ssl/tls13con.c | 4 +- 9 files changed, 76 insertions(+), 16 deletions(-) diff --git a/gtests/ssl_gtest/libssl_internals.c b/gtests/ssl_gtest/libssl_internals.c index 14b5257f8a..dd168d2cda 100644 --- a/gtests/ssl_gtest/libssl_internals.c +++ b/gtests/ssl_gtest/libssl_internals.c @@ -367,3 +367,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 342c3040a8..9058f322ff 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 8c96e09939..9b8d9fa46f 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 = std::make_shared(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 = std::make_shared(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 f6269a0af1..b4317e0f76 100644 --- a/gtests/ssl_gtest/tls_connect.cc +++ b/gtests/ssl_gtest/tls_connect.cc @@ -174,6 +174,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 60cca68b1b..11150a1dfc 100644 --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -10281,7 +10281,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 bdb0cbea62..04c8182cc2 100644 --- a/lib/ssl/ssl3exthandle.c +++ b/lib/ssl/ssl3exthandle.c @@ -850,6 +850,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 */ @@ -1567,8 +1570,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); @@ -1623,6 +1625,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; @@ -1662,6 +1665,9 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, SECItem *data) 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 0cea957d53..e2997eff4c 100644 --- a/lib/ssl/sslimpl.h +++ b/lib/ssl/sslimpl.h @@ -1221,16 +1221,12 @@ 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 PRUint32 ssl_sid_timeout; extern PRUint32 ssl3_sid_timeout; +extern PRUint32 ssl_ticket_lifetime; extern const char *const ssl3_cipherName[]; @@ -1698,10 +1694,6 @@ SECStatus ssl_GetSessionTicketKeys(sslSocket *ss, unsigned char *keyName, PK11SymKey **encKey, PK11SymKey **macKey); void ssl_ResetSessionTicketKeys(); -/* 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 616901300c..7ad1c6bc7a 100644 --- a/lib/ssl/sslnonce.c +++ b/lib/ssl/sslnonce.c @@ -19,7 +19,6 @@ #include #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 a565ff82a2..edbb29fb0a 100644 --- a/lib/ssl/tls13con.c +++ b/lib/ssl/tls13con.c @@ -3808,7 +3808,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) @@ -3827,7 +3827,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;