From 56d701de2827f429d2f14b16ce8175d83783ec68 Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Mon, 31 Jul 2017 11:25:36 +1000 Subject: [PATCH] Bug 1385746 - Add an application-selected token to tickets, r=ekr --HG-- branch : NSS_TLS13_DRAFT19_BRANCH --- gtests/ssl_gtest/libssl_internals.c | 20 --- gtests/ssl_gtest/libssl_internals.h | 1 - gtests/ssl_gtest/ssl_resumption_unittest.cc | 142 +++++++++++++++++++- gtests/ssl_gtest/tls_filter.h | 2 + lib/ssl/ssl3con.c | 4 +- lib/ssl/ssl3exthandle.c | 31 ++++- lib/ssl/sslexp.h | 10 +- lib/ssl/sslimpl.h | 5 +- lib/ssl/tls13con.c | 23 +++- lib/ssl/tls13con.h | 1 - 10 files changed, 198 insertions(+), 41 deletions(-) diff --git a/gtests/ssl_gtest/libssl_internals.c b/gtests/ssl_gtest/libssl_internals.c index 9152e58860..a254b099a0 100644 --- a/gtests/ssl_gtest/libssl_internals.c +++ b/gtests/ssl_gtest/libssl_internals.c @@ -225,26 +225,6 @@ 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; diff --git a/gtests/ssl_gtest/libssl_internals.h b/gtests/ssl_gtest/libssl_internals.h index 60a0a6179e..f1ef08b0b2 100644 --- a/gtests/ssl_gtest/libssl_internals.h +++ b/gtests/ssl_gtest/libssl_internals.h @@ -35,7 +35,6 @@ 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); diff --git a/gtests/ssl_gtest/ssl_resumption_unittest.cc b/gtests/ssl_gtest/ssl_resumption_unittest.cc index 79cb1d528d..2f14b1cea6 100644 --- a/gtests/ssl_gtest/ssl_resumption_unittest.cc +++ b/gtests/ssl_gtest/ssl_resumption_unittest.cc @@ -9,6 +9,7 @@ #include "secerr.h" #include "ssl.h" #include "sslerr.h" +#include "sslexp.h" #include "sslproto.h" extern "C" { @@ -622,7 +623,7 @@ TEST_F(TlsConnectTest, TestTls13ResumptionDuplicateNST) { // Clear the session ticket keys to invalidate the old ticket. SSLInt_ClearSelfEncryptKey(); - SSLInt_SendNewSessionTicket(server_->ssl_fd()); + SSL_SendSessionTicket(server_->ssl_fd(), NULL, 0); SendReceive(); // Need to read so that we absorb the session tickets. CheckKeys(); @@ -636,6 +637,145 @@ TEST_F(TlsConnectTest, TestTls13ResumptionDuplicateNST) { SendReceive(); } +// Check that the value captured in a NewSessionTicket message matches the value +// captured from a pre_shared_key extension. +void NstTicketMatchesPskIdentity(const DataBuffer& nst, const DataBuffer& psk) { + uint32_t len; + + size_t offset = 4 + 4; // Skip ticket_lifetime and ticket_age_add. + ASSERT_TRUE(nst.Read(offset, 1, &len)); + offset += 1 + len; // Skip ticket_nonce. + + ASSERT_TRUE(nst.Read(offset, 2, &len)); + offset += 2; // Skip the ticket length. + ASSERT_LE(offset + len, nst.len()); + DataBuffer nst_ticket(nst.data() + offset, static_cast(len)); + + offset = 2; // Skip the identities length. + ASSERT_TRUE(psk.Read(offset, 2, &len)); + offset += 2; // Skip the identity length. + ASSERT_LE(offset + len, psk.len()); + DataBuffer psk_ticket(psk.data() + offset, static_cast(len)); + + EXPECT_EQ(nst_ticket, psk_ticket); +} + +TEST_F(TlsConnectTest, TestTls13ResumptionDuplicateNSTWithToken) { + ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET); + ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); + + auto nst_capture = std::make_shared( + ssl_hs_new_session_ticket); + server_->SetTlsRecordFilter(nst_capture); + Connect(); + + // Clear the session ticket keys to invalidate the old ticket. + SSLInt_ClearSelfEncryptKey(); + nst_capture->Reset(); + uint8_t token[] = {0x20, 0x20, 0xff, 0x00}; + EXPECT_EQ(SECSuccess, + SSL_SendSessionTicket(server_->ssl_fd(), token, sizeof(token))); + + SendReceive(); // Need to read so that we absorb the session tickets. + CheckKeys(); + EXPECT_LT(0U, nst_capture->buffer().len()); + + // Resume the connection. + Reset(); + ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET); + ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); + ExpectResumption(RESUME_TICKET); + + auto psk_capture = + std::make_shared(ssl_tls13_pre_shared_key_xtn); + client_->SetPacketFilter(psk_capture); + Connect(); + SendReceive(); + + NstTicketMatchesPskIdentity(nst_capture->buffer(), psk_capture->extension()); +} + +// Disable SSL_ENABLE_SESSION_TICKETS but ensure that tickets can still be sent +// by invoking SSL_SendSessionTicket directly (and that the ticket is usable). +TEST_F(TlsConnectTest, SendSessionTicketWithTicketsDisabled) { + ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET); + ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); + + EXPECT_EQ(SECSuccess, SSL_OptionSet(server_->ssl_fd(), + SSL_ENABLE_SESSION_TICKETS, PR_FALSE)); + + auto nst_capture = std::make_shared( + ssl_hs_new_session_ticket); + server_->SetTlsRecordFilter(nst_capture); + Connect(); + + EXPECT_EQ(0U, nst_capture->buffer().len()) << "expect nothing captured yet"; + + EXPECT_EQ(SECSuccess, SSL_SendSessionTicket(server_->ssl_fd(), NULL, 0)); + EXPECT_LT(0U, nst_capture->buffer().len()) << "should capture now"; + + SendReceive(); // Ensure that the client reads the ticket. + + // Resume the connection. + Reset(); + ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET); + ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); + ExpectResumption(RESUME_TICKET); + + auto psk_capture = + std::make_shared(ssl_tls13_pre_shared_key_xtn); + client_->SetPacketFilter(psk_capture); + Connect(); + SendReceive(); + + NstTicketMatchesPskIdentity(nst_capture->buffer(), psk_capture->extension()); +} + +// Test calling SSL_SendSessionTicket in inappropriate conditions. +TEST_F(TlsConnectTest, SendSessionTicketInappropriate) { + ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET); + ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_2); + + EXPECT_EQ(SECFailure, SSL_SendSessionTicket(client_->ssl_fd(), NULL, 0)) + << "clients can't send tickets"; + EXPECT_EQ(SEC_ERROR_INVALID_ARGS, PORT_GetError()); + + server_->StartConnect(); + client_->StartConnect(); + + EXPECT_EQ(SECFailure, SSL_SendSessionTicket(server_->ssl_fd(), NULL, 0)) + << "no ticket before the handshake has started"; + EXPECT_EQ(SEC_ERROR_INVALID_ARGS, PORT_GetError()); + Handshake(); + EXPECT_EQ(SECFailure, SSL_SendSessionTicket(server_->ssl_fd(), NULL, 0)) + << "no special tickets in TLS 1.2"; + EXPECT_EQ(SEC_ERROR_INVALID_ARGS, PORT_GetError()); +} + +TEST_F(TlsConnectTest, SendSessionTicketMassiveToken) { + ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET); + ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); + Connect(); + // It should be safe to set length with a NULL token because the length should + // be checked before reading token. + EXPECT_EQ(SECFailure, SSL_SendSessionTicket(server_->ssl_fd(), NULL, 0xffff)) + << "no special tickets in TLS 1.2"; + EXPECT_EQ(SEC_ERROR_INVALID_ARGS, PORT_GetError()); + + EXPECT_EQ(SECFailure, SSL_SendSessionTicket(server_->ssl_fd(), NULL, 0x1ffff)) + << "no special tickets in TLS 1.2"; + EXPECT_EQ(SEC_ERROR_INVALID_ARGS, PORT_GetError()); +} + +TEST_F(TlsConnectDatagram13, SendSessionTicketDtls) { + ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET); + ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); + Connect(); + EXPECT_EQ(SECFailure, SSL_SendSessionTicket(server_->ssl_fd(), NULL, 0)) + << "no extra tickets in DTLS until we have Ack support"; + EXPECT_EQ(SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION, PORT_GetError()); +} + TEST_F(TlsConnectTest, TestTls13ResumptionDowngrade) { ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET); ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3); diff --git a/gtests/ssl_gtest/tls_filter.h b/gtests/ssl_gtest/tls_filter.h index afa735ccbe..cb55d8b9da 100644 --- a/gtests/ssl_gtest/tls_filter.h +++ b/gtests/ssl_gtest/tls_filter.h @@ -209,6 +209,8 @@ class TlsInspectorRecordHandshakeMessage : public TlsHandshakeFilter { const DataBuffer& input, DataBuffer* output); + void Reset() { buffer_.Truncate(0); } + const DataBuffer& buffer() const { return buffer_; } private: diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c index 48b9002e5d..1cc53356cb 100644 --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -10138,8 +10138,8 @@ ssl3_SendNewSessionTicket(sslSocket *ss) SECStatus rv; NewSessionTicket nticket = { 0 }; - rv = ssl3_EncodeSessionTicket(ss, &nticket, &ticket, - ss->ssl3.pwSpec->master_secret); + rv = ssl3_EncodeSessionTicket(ss, &nticket, NULL, 0, + ss->ssl3.pwSpec->master_secret, &ticket); if (rv != SECSuccess) goto loser; diff --git a/lib/ssl/ssl3exthandle.c b/lib/ssl/ssl3exthandle.c index 4fb1c761e3..ea9b49c30b 100644 --- a/lib/ssl/ssl3exthandle.c +++ b/lib/ssl/ssl3exthandle.c @@ -660,9 +660,9 @@ PRUint32 ssl_ticket_lifetime = 2 * 24 * 60 * 60; /* 2 days in seconds */ * Called from ssl3_SendNewSessionTicket, tls13_SendNewSessionTicket */ SECStatus -ssl3_EncodeSessionTicket(sslSocket *ss, - const NewSessionTicket *ticket, - SECItem *ticket_data, PK11SymKey *secret) +ssl3_EncodeSessionTicket(sslSocket *ss, const NewSessionTicket *ticket, + const PRUint8 *appToken, unsigned int appTokenLen, + PK11SymKey *secret, SECItem *ticket_data) { SECStatus rv; SECItem plaintext; @@ -745,7 +745,15 @@ ssl3_EncodeSessionTicket(sslSocket *ss, + sizeof(ticket->flags) /* ticket flags */ + 1 + alpnSelection->len /* alpn value + length field */ + 4 /* maxEarlyData */ - + 4; /* ticketAgeBaseline */ + + 4 /* ticketAgeBaseline */ + + 2 + appTokenLen; /* application token */ + + /* This really only happens if appTokenLen is too much, and that always + * comes from the using application. */ + if (plaintext_length > 0xffff) { + PORT_SetError(SEC_ERROR_INVALID_ARGS); + goto loser; + } if (SECITEM_AllocItem(NULL, &plaintext_item, plaintext_length) == NULL) goto loser; @@ -905,6 +913,14 @@ ssl3_EncodeSessionTicket(sslSocket *ss, if (rv != SECSuccess) goto loser; + /* Application token */ + rv = ssl3_AppendNumberToItem(&plaintext, appTokenLen, 2); + if (rv != SECSuccess) + goto loser; + rv = ssl3_AppendToItem(&plaintext, appToken, appTokenLen); + if (rv != SECSuccess) + goto loser; + /* Check that we are totally full. */ PORT_Assert(plaintext.len == 0); @@ -1178,6 +1194,13 @@ ssl_ParseSessionTicket(sslSocket *ss, const SECItem *decryptedTicket, } parsedTicket->ticketAgeBaseline = temp; + rv = ssl3_ExtConsumeHandshakeVariable(ss, &parsedTicket->applicationToken, + 2, &buffer, &len); + if (rv != SECSuccess) { + PORT_SetError(SEC_ERROR_LIBRARY_FAILURE); + return SECFailure; + } + #ifndef UNSAFE_FUZZER_MODE /* Done parsing. Check that all bytes have been consumed. */ if (len != 0) { diff --git a/lib/ssl/sslexp.h b/lib/ssl/sslexp.h index 0688d1a684..c9a28182a1 100644 --- a/lib/ssl/sslexp.h +++ b/lib/ssl/sslexp.h @@ -236,11 +236,11 @@ typedef SECStatus(PR_CALLBACK *SSLExtensionHandler)( * Earlier versions of TLS do not support the spontaneous sending of the * NewSessionTicket message. */ -#define SSL_SendSessionTicket(fd, token, tokenLen) \ - SSL_EXPERIMENTAL_API("SSL_SendSessionTicket", \ - (PRFiledesc * _fd, const PRUint8 *_token, \ - unsigned int _tokenLen), \ - (fd, token, tokenLen)) +#define SSL_SendSessionTicket(fd, appToken, appTokenLen) \ + SSL_EXPERIMENTAL_API("SSL_SendSessionTicket", \ + (PRFileDesc * _fd, const PRUint8 *_appToken, \ + unsigned int _appTokenLen), \ + (fd, appToken, appTokenLen)) SEC_END_PROTOS diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h index 28e46a16c6..6ffbaef76d 100644 --- a/lib/ssl/sslimpl.h +++ b/lib/ssl/sslimpl.h @@ -1002,6 +1002,7 @@ typedef struct SessionTicketStr { SECItem alpnSelection; PRUint32 maxEarlyData; PRUint32 ticketAgeBaseline; + SECItem applicationToken; } SessionTicket; /* @@ -1698,7 +1699,9 @@ extern void ssl3_SetSIDSessionTicket(sslSessionID *sid, /*in/out*/ NewSessionTicket *session_ticket); SECStatus ssl3_EncodeSessionTicket(sslSocket *ss, const NewSessionTicket *ticket, - SECItem *ticket_data, PK11SymKey *secret); + const PRUint8 *appToken, + unsigned int appTokenLen, + PK11SymKey *secret, SECItem *ticket_data); SECStatus SSLExp_SendSessionTicket(PRFileDesc *fd, const PRUint8 *token, unsigned int tokenLen); diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c index c8ccb91a86..4f01b02632 100644 --- a/lib/ssl/tls13con.c +++ b/lib/ssl/tls13con.c @@ -97,6 +97,9 @@ static SECStatus tls13_ClientHandleFinished(sslSocket *ss, PRUint8 *b, PRUint32 length); static SECStatus tls13_ServerHandleFinished(sslSocket *ss, PRUint8 *b, PRUint32 length); +static SECStatus tls13_SendNewSessionTicket(sslSocket *ss, + const PRUint8 *appToken, + unsigned int appTokenLen); static SECStatus tls13_HandleNewSessionTicket(sslSocket *ss, PRUint8 *b, PRUint32 length); static SECStatus tls13_ComputeHandshakeHashes(sslSocket *ss, @@ -3721,7 +3724,7 @@ tls13_ServerHandleFinished(sslSocket *ss, PRUint8 *b, PRUint32 length) } ssl_GetXmitBufLock(ss); if (ss->opt.enableSessionTickets) { - rv = tls13_SendNewSessionTicket(ss); + rv = tls13_SendNewSessionTicket(ss, NULL, 0); if (rv != SECSuccess) { ssl_ReleaseXmitBufLock(ss); return SECFailure; /* Error code and alerts handled below */ @@ -3930,8 +3933,9 @@ tls13_SendClientSecondRound(sslSocket *ss) PRUint32 ssl_max_early_data_size = (2 << 16); /* Arbitrary limit. */ -SECStatus -tls13_SendNewSessionTicket(sslSocket *ss) +static SECStatus +tls13_SendNewSessionTicket(sslSocket *ss, const PRUint8 *appToken, + unsigned int appTokenLen) { PRUint16 message_length; PK11SymKey *secret; @@ -3971,7 +3975,8 @@ tls13_SendNewSessionTicket(sslSocket *ss) goto loser; } - rv = ssl3_EncodeSessionTicket(ss, &ticket, &ticket_data, secret); + rv = ssl3_EncodeSessionTicket(ss, &ticket, appToken, appTokenLen, + secret, &ticket_data); PK11_FreeSymKey(secret); if (rv != SECSuccess) goto loser; @@ -4052,15 +4057,21 @@ SSLExp_SendSessionTicket(PRFileDesc *fd, const PRUint8 *token, return SECFailure; } + if (IS_DTLS(ss)) { + PORT_SetError(SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION); + return SECFailure; + } + if (!ss->sec.isServer || !ss->firstHsDone || - ss->version < SSL_LIBRARY_VERSION_TLS_1_3) { + ss->version < SSL_LIBRARY_VERSION_TLS_1_3 || + tokenLen > 0xffff) { PORT_SetError(SEC_ERROR_INVALID_ARGS); return SECFailure; } ssl_GetSSL3HandshakeLock(ss); ssl_GetXmitBufLock(ss); - rv = tls13_SendNewSessionTicket(ss); + rv = tls13_SendNewSessionTicket(ss, token, tokenLen); if (rv == SECSuccess) { rv = ssl3_FlushHandshake(ss, 0); } diff --git a/lib/ssl/tls13con.h b/lib/ssl/tls13con.h index f4645ff69c..66e4580dd1 100644 --- a/lib/ssl/tls13con.h +++ b/lib/ssl/tls13con.h @@ -92,7 +92,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); PRBool tls13_IsReplay(const sslSocket *ss, const sslSessionID *sid); void tls13_AntiReplayRollover(PRTime now);