From e533ed60d6cf690149b2cf46fd15255ba9ecad25 Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Fri, 12 Oct 2018 18:10:36 +1100 Subject: [PATCH] Bug 1493769 - Set session_id for external resumption tokens, r=franziskus This also includes some cleanup that I performed when looking into this. It turns out that the hacks that we were using for managing the reference count on sids was unnecessary. Daiki added a much neater solution in D7493 that I stole. The error handling in SSLExp_SetResumptionToken looks nicer after a spring-clean too. --HG-- extra : rebase_source : a4aeff32ce0cee61743d98234a21d7726a8dc496 extra : amend_source : 9b3492f78154ebad6216e9a0dacd7e498d906927 --- gtests/ssl_gtest/ssl_resumption_unittest.cc | 28 ++++++++++++++ gtests/ssl_gtest/tls_agent.cc | 1 + gtests/ssl_gtest/tls_connect.cc | 6 ++- lib/ssl/ssl3con.c | 6 +-- lib/ssl/sslimpl.h | 3 +- lib/ssl/sslnonce.c | 16 ++++++-- lib/ssl/sslsock.c | 41 ++++++++++++--------- 7 files changed, 73 insertions(+), 28 deletions(-) diff --git a/gtests/ssl_gtest/ssl_resumption_unittest.cc b/gtests/ssl_gtest/ssl_resumption_unittest.cc index 6c52b1f57e..30d74acf7c 100644 --- a/gtests/ssl_gtest/ssl_resumption_unittest.cc +++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc @@ -1126,6 +1126,34 @@ void CheckGetInfoResult(uint32_t alpnSize, uint32_t earlyDataSize, ASSERT_EQ(earlyDataSize, token->maxEarlyDataSize); } +// The client should generate a new, randomized session_id +// when resuming using an external token. +TEST_P(TlsConnectGenericResumptionToken, CheckSessionId) { + ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH); + auto original_sid = MakeTlsFilter(client_); + Connect(); + SendReceive(); + + Reset(); + ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH); + ExpectResumption(RESUME_TICKET); + + StartConnect(); + ASSERT_TRUE(client_->MaybeSetResumptionToken()); + auto resumed_sid = MakeTlsFilter(client_); + + Handshake(); + CheckConnected(); + SendReceive(); + + if (version_ < SSL_LIBRARY_VERSION_TLS_1_3) { + EXPECT_NE(resumed_sid->sid(), original_sid->sid()); + EXPECT_EQ(32U, resumed_sid->sid().len()); + } else { + EXPECT_EQ(0U, resumed_sid->sid().len()); + } +} + TEST_P(TlsConnectGenericResumptionToken, ConnectResumeGetInfo) { ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH); Connect(); diff --git a/gtests/ssl_gtest/tls_agent.cc b/gtests/ssl_gtest/tls_agent.cc index 488842a88b..792d6dd89d 100644 --- a/gtests/ssl_gtest/tls_agent.cc +++ b/gtests/ssl_gtest/tls_agent.cc @@ -229,6 +229,7 @@ bool TlsAgent::EnsureTlsSetup(PRFileDesc* modelSocket) { bool TlsAgent::MaybeSetResumptionToken() { if (!resumption_token_.empty()) { + LOG("setting external resumption token"); SECStatus rv = SSL_SetResumptionToken(ssl_fd(), resumption_token_.data(), resumption_token_.size()); diff --git a/gtests/ssl_gtest/tls_connect.cc b/gtests/ssl_gtest/tls_connect.cc index e65fea782b..c48ae38ecf 100644 --- a/gtests/ssl_gtest/tls_connect.cc +++ b/gtests/ssl_gtest/tls_connect.cc @@ -559,13 +559,15 @@ void TlsConnectTestBase::CheckResumption(SessionResumptionMode expected) { EXPECT_EQ(stateless_count, stats->hsh_sid_stateless_resumes); if (expected != RESUME_NONE) { - if (client_->version() < SSL_LIBRARY_VERSION_TLS_1_3) { + if (client_->version() < SSL_LIBRARY_VERSION_TLS_1_3 && + client_->GetResumptionToken().size() == 0) { // Check that the last two session ids match. ASSERT_EQ(1U + expected_resumptions_, session_ids_.size()); EXPECT_EQ(session_ids_[session_ids_.size() - 1], session_ids_[session_ids_.size() - 2]); } else { - // TLS 1.3 only uses tickets. + // We've either chosen TLS 1.3 or are using an external resumption token, + // both of which only use tickets. EXPECT_TRUE(expected & RESUME_TICKET); } } diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c index a44c846afd..6c8c88b365 100644 --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -4771,7 +4771,7 @@ ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type) * If we have an sid and it comes from an external cache, we use it. */ if (ss->sec.ci.sid && ss->sec.ci.sid->cached == in_external_cache) { PORT_Assert(!ss->sec.isServer); - sid = ss->sec.ci.sid; + sid = ssl_ReferenceSID(ss->sec.ci.sid); SSL_TRC(3, ("%d: SSL3[%d]: using external resumption token in ClientHello", SSL_GETPID(), ss->fd)); } else if (!ss->opt.noCache) { @@ -4919,9 +4919,7 @@ ssl3_SendClientHello(sslSocket *ss, sslClientHelloType type) } ssl_ReleaseSpecWriteLock(ss); - if (ss->sec.ci.sid != NULL) { - ssl_FreeSID(ss->sec.ci.sid); /* decrement ref count, free if zero */ - } + ssl_FreeSID(ss->sec.ci.sid); /* release the old sid */ ss->sec.ci.sid = sid; /* HACK for SCSV in SSL 3.0. On initial handshake, prepend SCSV, diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h index 59d2c3cb19..35240d2fba 100644 --- a/lib/ssl/sslimpl.h +++ b/lib/ssl/sslimpl.h @@ -1184,6 +1184,7 @@ extern sslSessionID *ssl_LookupSID(const PRIPv6Addr *addr, PRUint16 port, const char *peerID, const char *urlSvrName); extern void ssl_FreeSID(sslSessionID *sid); extern void ssl_DestroySID(sslSessionID *sid, PRBool freeIt); +extern sslSessionID *ssl_ReferenceSID(sslSessionID *sid); extern int ssl3_SendApplicationData(sslSocket *ss, const PRUint8 *in, int len, int flags); @@ -1726,7 +1727,7 @@ void ssl_Trace(const char *format, ...); void ssl_CacheExternalToken(sslSocket *ss); SECStatus ssl_DecodeResumptionToken(sslSessionID *sid, const PRUint8 *encodedTicket, PRUint32 encodedTicketLen); -PRBool ssl_IsResumptionTokenValid(sslSocket *ss); +PRBool ssl_IsResumptionTokenUsable(sslSocket *ss, sslSessionID *sid); /* Remove when stable. */ diff --git a/lib/ssl/sslnonce.c b/lib/ssl/sslnonce.c index 415b6ad933..f8fb5d50f1 100644 --- a/lib/ssl/sslnonce.c +++ b/lib/ssl/sslnonce.c @@ -233,10 +233,21 @@ ssl_FreeLockedSID(sslSessionID *sid) */ void ssl_FreeSID(sslSessionID *sid) +{ + if (sid) { + LOCK_CACHE; + ssl_FreeLockedSID(sid); + UNLOCK_CACHE; + } +} + +sslSessionID * +ssl_ReferenceSID(sslSessionID *sid) { LOCK_CACHE; - ssl_FreeLockedSID(sid); + sid->references++; UNLOCK_CACHE; + return sid; } /************************************************************************/ @@ -704,10 +715,9 @@ ssl_DecodeResumptionToken(sslSessionID *sid, const PRUint8 *encodedToken, } PRBool -ssl_IsResumptionTokenValid(sslSocket *ss) +ssl_IsResumptionTokenUsable(sslSocket *ss, sslSessionID *sid) { PORT_Assert(ss); - sslSessionID *sid = ss->sec.ci.sid; PORT_Assert(sid); // Check that the ticket didn't expire. diff --git a/lib/ssl/sslsock.c b/lib/ssl/sslsock.c index 9caae0ec2c..e51da197f3 100644 --- a/lib/ssl/sslsock.c +++ b/lib/ssl/sslsock.c @@ -18,6 +18,7 @@ #include "private/pprio.h" #include "nss.h" #include "pk11pqg.h" +#include "pk11pub.h" #include "tls13esni.h" static const sslSocketOps ssl_default_ops = { /* No SSL. */ @@ -4102,6 +4103,7 @@ SSLExp_SetResumptionToken(PRFileDesc *fd, const PRUint8 *token, unsigned int len) { sslSocket *ss = ssl_FindSocket(fd); + sslSessionID *sid = NULL; if (!ss) { SSL_DBG(("%d: SSL[%d]: bad socket in SSL_SetResumptionToken", @@ -4115,7 +4117,7 @@ SSLExp_SetResumptionToken(PRFileDesc *fd, const PRUint8 *token, if (ss->firstHsDone || ss->ssl3.hs.ws != idle_handshake || ss->sec.isServer || len == 0 || !token) { PORT_SetError(SEC_ERROR_INVALID_ARGS); - goto done; + goto loser; } // We override any previously set session. @@ -4126,41 +4128,44 @@ SSLExp_SetResumptionToken(PRFileDesc *fd, const PRUint8 *token, PRINT_BUF(50, (ss, "incoming resumption token", token, len)); - ss->sec.ci.sid = ssl3_NewSessionID(ss, PR_FALSE); - if (!ss->sec.ci.sid) { - goto done; + sid = ssl3_NewSessionID(ss, PR_FALSE); + if (!sid) { + goto loser; } /* Populate NewSessionTicket values */ - SECStatus rv = ssl_DecodeResumptionToken(ss->sec.ci.sid, token, len); + SECStatus rv = ssl_DecodeResumptionToken(sid, token, len); if (rv != SECSuccess) { // If decoding fails, we assume the token is bad. PORT_SetError(SSL_ERROR_BAD_RESUMPTION_TOKEN_ERROR); - ssl_FreeSID(ss->sec.ci.sid); - ss->sec.ci.sid = NULL; - goto done; + goto loser; } - // Make sure that the token is valid. - if (!ssl_IsResumptionTokenValid(ss)) { - ssl_FreeSID(ss->sec.ci.sid); - ss->sec.ci.sid = NULL; + // Make sure that the token is currently usable. + if (!ssl_IsResumptionTokenUsable(ss, sid)) { PORT_SetError(SSL_ERROR_BAD_RESUMPTION_TOKEN_ERROR); - goto done; + goto loser; } + // Generate a new random session ID for this ticket. + rv = PK11_GenerateRandom(sid->u.ssl3.sessionID, SSL3_SESSIONID_BYTES); + if (rv != SECSuccess) { + goto loser; // Code set by PK11_GenerateRandom. + } + sid->u.ssl3.sessionIDLength = SSL3_SESSIONID_BYTES; /* Use the sid->cached as marker that this is from an external cache and * we don't have to look up anything in the NSS internal cache. */ - ss->sec.ci.sid->cached = in_external_cache; - // This has to be 2 to not free this in sendClientHello. - ss->sec.ci.sid->references = 2; - ss->sec.ci.sid->lastAccessTime = ssl_TimeSec(); + sid->cached = in_external_cache; + sid->lastAccessTime = ssl_TimeSec(); + + ss->sec.ci.sid = sid; ssl_ReleaseSSL3HandshakeLock(ss); ssl_Release1stHandshakeLock(ss); return SECSuccess; -done: +loser: + ssl_FreeSID(sid); ssl_ReleaseSSL3HandshakeLock(ss); ssl_Release1stHandshakeLock(ss);