Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Bug 1656429 - Correct RTT estimate used in anti-replay, r=kjacobs
This was never a security problem, but the more time that passes between the
handshake and sending a ticket, the more likely we are to reject 0-RTT.
Eventually, 0-RTT only works if it is delayed in the network by a surprising
amount.

Differential Revision: https://phabricator.services.mozilla.com/D85540

--HG--
extra : moz-landing-system : lando
  • Loading branch information
martinthomson committed Aug 5, 2020
1 parent 6ebd04c commit 9d44f17
Show file tree
Hide file tree
Showing 6 changed files with 126 additions and 8 deletions.
100 changes: 100 additions & 0 deletions gtests/ssl_gtest/ssl_0rtt_unittest.cc
Expand Up @@ -211,6 +211,106 @@ TEST_P(TlsConnectTls13, ZeroRttOptionsSetLate) {
SendReceive();
}

// Make sure that a session ticket sent well after the original handshake
// can be used for 0-RTT.
// Stream because DTLS doesn't support SSL_SendSessionTicket.
TEST_F(TlsConnectStreamTls13, ZeroRttUsingLateTicket) {
// Use a small-ish anti-replay window.
ResetAntiReplay(100 * PR_USEC_PER_MSEC);
RolloverAntiReplay();

ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
server_->Set0RttEnabled(true);
Connect();
CheckKeys();

// Now move time forward 30s and send a ticket.
AdvanceTime(30 * PR_USEC_PER_SEC);
EXPECT_EQ(SECSuccess, SSL_SendSessionTicket(server_->ssl_fd(), NULL, 0));
SendReceive();
Reset();
StartConnect();

client_->Set0RttEnabled(true);
server_->Set0RttEnabled(true);
ExpectResumption(RESUME_TICKET);
ZeroRttSendReceive(true, true);
Handshake();
ExpectEarlyDataAccepted(true);
CheckConnected();
SendReceive();
}

// Check that post-handshake authentication with a long RTT doesn't
// make things worse.
TEST_F(TlsConnectStreamTls13, ZeroRttUsingLateTicketPha) {
// Use a small-ish anti-replay window.
ResetAntiReplay(100 * PR_USEC_PER_MSEC);
RolloverAntiReplay();

ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
server_->Set0RttEnabled(true);
client_->SetupClientAuth();
client_->SetOption(SSL_ENABLE_POST_HANDSHAKE_AUTH, PR_TRUE);
Connect();
CheckKeys();

// Add post-handshake authentication, with some added delays.
AdvanceTime(10 * PR_USEC_PER_SEC);
EXPECT_EQ(SECSuccess, SSL_SendCertificateRequest(server_->ssl_fd()));
AdvanceTime(10 * PR_USEC_PER_SEC);
server_->SendData(50);
client_->ReadBytes(50);
client_->SendData(50);
server_->ReadBytes(50);

AdvanceTime(10 * PR_USEC_PER_SEC);
EXPECT_EQ(SECSuccess, SSL_SendSessionTicket(server_->ssl_fd(), NULL, 0));
server_->SendData(100);
client_->ReadBytes(100);
Reset();
StartConnect();

client_->Set0RttEnabled(true);
server_->Set0RttEnabled(true);
ExpectResumption(RESUME_TICKET);
ZeroRttSendReceive(true, true);
Handshake();
ExpectEarlyDataAccepted(true);
CheckConnected();
SendReceive();
}

// Same, but with client authentication on the first connection.
TEST_F(TlsConnectStreamTls13, ZeroRttUsingLateTicketClientAuth) {
// Use a small-ish anti-replay window.
ResetAntiReplay(100 * PR_USEC_PER_MSEC);
RolloverAntiReplay();

ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
client_->SetupClientAuth();
server_->RequestClientAuth(true);
server_->Set0RttEnabled(true);
Connect();
CheckKeys();

// Now move time forward 30s and send a ticket.
AdvanceTime(30 * PR_USEC_PER_SEC);
EXPECT_EQ(SECSuccess, SSL_SendSessionTicket(server_->ssl_fd(), NULL, 0));
SendReceive();
Reset();
StartConnect();

client_->Set0RttEnabled(true);
server_->Set0RttEnabled(true);
ExpectResumption(RESUME_TICKET);
ZeroRttSendReceive(true, true);
Handshake();
ExpectEarlyDataAccepted(true);
CheckConnected();
SendReceive();
}

TEST_P(TlsConnectTls13, ZeroRttServerForgetTicket) {
SetupForZeroRtt();
client_->Set0RttEnabled(true);
Expand Down
2 changes: 1 addition & 1 deletion gtests/ssl_gtest/tls_connect.cc
Expand Up @@ -107,7 +107,7 @@ std::string VersionString(uint16_t version) {
}

// The default anti-replay window for tests. Tests that rely on a different
// value call SSL_InitAntiReplay directly.
// value call ResetAntiReplay directly.
static PRTime kAntiReplayWindow = 100 * PR_USEC_PER_SEC;

TlsConnectTestBase::TlsConnectTestBase(SSLProtocolVariant variant,
Expand Down
2 changes: 1 addition & 1 deletion lib/ssl/ssl3exthandle.c
Expand Up @@ -796,7 +796,7 @@ ssl3_EncodeSessionTicket(sslSocket *ss, const NewSessionTicket *ticket,
* This is compared to the expected time, which should differ only as a
* result of clock errors or errors in the RTT estimate.
*/
ticketAgeBaseline = (ssl_Time(ss) - ss->ssl3.hs.serverHelloTime) / PR_USEC_PER_MSEC;
ticketAgeBaseline = ss->ssl3.hs.rttEstimate / PR_USEC_PER_MSEC;
ticketAgeBaseline -= ticket->ticket_age_add;
rv = sslBuffer_AppendNumber(&plaintext, ticketAgeBaseline, 4);
if (rv != SECSuccess)
Expand Down
10 changes: 8 additions & 2 deletions lib/ssl/sslimpl.h
Expand Up @@ -713,17 +713,23 @@ typedef struct SSL3HandshakeStateStr {
PRBool receivedCcs; /* A server received ChangeCipherSpec
* before the handshake started. */
PRBool clientCertRequested; /* True if CertificateRequest received. */
PRBool endOfFlight; /* Processed a full flight (DTLS 1.3). */
ssl3KEADef kea_def_mutable; /* Used to hold the writable kea_def
* we use for TLS 1.3 */
PRTime serverHelloTime; /* Time the ServerHello flight was sent. */
PRUint16 ticketNonce; /* A counter we use for tickets. */
SECItem fakeSid; /* ... (server) the SID the client used. */
PRBool endOfFlight; /* Processed a full flight (DTLS 1.3). */

/* rttEstimate is used to guess the round trip time between server and client.
* When the server sends ServerHello it sets this to the current time.
* Only after it receives a message from the client's second flight does it
* set the value to something resembling an RTT estimate. */
PRTime rttEstimate;

/* The following lists contain DTLSHandshakeRecordEntry */
PRCList dtlsSentHandshake; /* Used to map records to handshake fragments. */
PRCList dtlsRcvdHandshake; /* Handshake records we have received
* used to generate ACKs. */

PRCList psks; /* A list of PSKs, resumption and/or external. */
} SSL3HandshakeState;

Expand Down
17 changes: 15 additions & 2 deletions lib/ssl/tls13con.c
Expand Up @@ -2866,7 +2866,9 @@ tls13_SendServerHelloSequence(sslSocket *ss)
}
}

ss->ssl3.hs.serverHelloTime = ssl_Time(ss);
/* Here we set a baseline value for our RTT estimation.
* This value is updated when we get a response from the client. */
ss->ssl3.hs.rttEstimate = ssl_Time(ss);
return SECSuccess;
}

Expand Down Expand Up @@ -3251,8 +3253,9 @@ tls13_HandleCertificate(sslSocket *ss, PRUint8 *b, PRUint32 length)
rv = TLS13_CHECK_HS_STATE(ss, SSL_ERROR_RX_UNEXPECTED_CERTIFICATE,
wait_cert_request, wait_server_cert);
}
if (rv != SECSuccess)
if (rv != SECSuccess) {
return SECFailure;
}

/* We can ignore any other cleartext from the client. */
if (ss->sec.isServer && IS_DTLS(ss)) {
Expand All @@ -3266,6 +3269,11 @@ tls13_HandleCertificate(sslSocket *ss, PRUint8 *b, PRUint32 length)
PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
return SECFailure;
}
} else if (ss->sec.isServer) {
/* Our first shot an getting an RTT estimate. If the client took extra
* time to fetch a certificate, this will be bad, but we can't do much
* about that. */
ss->ssl3.hs.rttEstimate = ssl_Time(ss) - ss->ssl3.hs.rttEstimate;
}

/* Process the context string */
Expand Down Expand Up @@ -4773,6 +4781,11 @@ tls13_ServerHandleFinished(sslSocket *ss, PRUint8 *b, PRUint32 length)
if (rv != SECSuccess) {
return SECFailure; /* Code already set. */
}

if (!tls13_IsPostHandshake(ss)) {
/* Finalize the RTT estimate. */
ss->ssl3.hs.rttEstimate = ssl_Time(ss) - ss->ssl3.hs.rttEstimate;
}
}

rv = tls13_CommonHandleFinished(ss,
Expand Down
3 changes: 1 addition & 2 deletions lib/ssl/tls13replay.c
Expand Up @@ -56,8 +56,7 @@ tls13_ReleaseAntiReplayContext(SSLAntiReplayContext *ctx)
PORT_Free(ctx);
}

/* Clear the current state and free any resources we allocated. The signature
* here is odd to allow this to be called during shutdown. */
/* Clear the current state and free any resources we allocated. */
SECStatus
SSLExp_ReleaseAntiReplayContext(SSLAntiReplayContext *ctx)
{
Expand Down

0 comments on commit 9d44f17

Please sign in to comment.