Skip to content

Commit

Permalink
Bug 1309054 - The server shouldn't resume if it doesn't pick the same…
Browse files Browse the repository at this point in the history 
… cipher suite, r=ekr

--HG--
extra : rebase_source : 7995b4bad3d620af372e8a202d4fcc927b286e45
  • Loading branch information
@martinthomson
martinthomson committed Oct 11, 2016
1 parent 4d1939b commit 93d191f
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 50 deletions.
48 changes: 27 additions & 21 deletions external_tests/ssl_gtest/ssl_resumption_unittest.cc
View file
Expand Up @@ -322,34 +322,50 @@ TEST_P(TlsConnectTls13, TestTls13ResumeDifferentGroup) {
CheckKeys(ssl_kea_dh, ssl_grp_ffdhe_2048, ssl_auth_rsa_sign, ssl_sig_none);
}

// We need to enable different cipher suites at different times in the following
// tests. Those cipher suites need to be suited to the version.
static uint16_t ChooseOneCipher(uint16_t version) {
if (version >= SSL_LIBRARY_VERSION_TLS_1_3) {
return TLS_AES_128_GCM_SHA256;
}
return TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA;
}

static uint16_t ChooseAnotherCipher(uint16_t version) {
if (version >= SSL_LIBRARY_VERSION_TLS_1_3) {
return TLS_CHACHA20_POLY1305_SHA256;
}
return TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA;
}

// Test that we don't resume when we can't negotiate the same cipher.
TEST_P(TlsConnectTls13, TestTls13ResumeClientDifferentCipher) {
TEST_P(TlsConnectGeneric, TestResumeClientDifferentCipher) {
ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
client_->EnableSingleCipher(TLS_AES_128_GCM_SHA256);
client_->EnableSingleCipher(ChooseOneCipher(version_));
Connect();
SendReceive(); // Need to read so that we absorb the session ticket.
CheckKeys();
SendReceive();
CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign);

Reset();
ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
ExpectResumption(RESUME_NONE);
client_->EnableSingleCipher(TLS_AES_256_GCM_SHA384);
client_->EnableSingleCipher(ChooseAnotherCipher(version_));
Connect();
CheckKeys();
CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign);
}

// Test that we don't resume when we can't negotiate the same cipher.
TEST_P(TlsConnectTls13, TestTls13ResumeServerDifferentCipher) {
TEST_P(TlsConnectGeneric, TestResumeServerDifferentCipher) {
ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
server_->EnableSingleCipher(TLS_AES_128_GCM_SHA256);
server_->EnableSingleCipher(ChooseOneCipher(version_));
Connect();
SendReceive(); // Need to read so that we absorb the session ticket.
CheckKeys();

Reset();
ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
ExpectResumption(RESUME_NONE);
server_->EnableSingleCipher(TLS_AES_256_GCM_SHA384);
server_->EnableSingleCipher(ChooseAnotherCipher(version_));
Connect();
CheckKeys();
}
Expand Down Expand Up @@ -388,24 +404,14 @@ class SelectedCipherSuiteReplacer : public TlsHandshakeFilter {
// suite for resumption.
TEST_P(TlsConnectStream, TestResumptionOverrideCipher) {
ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
server_->EnableSingleCipher(TLS_AES_128_GCM_SHA256);
} else {
server_->EnableSingleCipher(TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA);
}
server_->EnableSingleCipher(ChooseOneCipher(version_));
Connect();
SendReceive();
CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign);

Reset();
ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
uint16_t overrideCipher;
if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
overrideCipher = TLS_CHACHA20_POLY1305_SHA256;
} else {
overrideCipher = TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA;
}
server_->SetPacketFilter(new SelectedCipherSuiteReplacer(overrideCipher));
server_->SetPacketFilter(new SelectedCipherSuiteReplacer(ChooseAnotherCipher(version_)));

ConnectExpectFail();
client_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_SERVER_HELLO);
Expand Down
48 changes: 19 additions & 29 deletions lib/ssl/ssl3con.c
View file
Expand Up @@ -59,8 +59,7 @@ static SECStatus ssl3_SendServerKeyExchange(sslSocket *ss);
static SECStatus ssl3_HandleClientHelloPart2(sslSocket *ss,
SECItem *suites,
SECItem *comps,
sslSessionID *sid,
PRBool canOfferSessionTicket);
sslSessionID *sid);
static SECStatus ssl3_HandleServerHelloPart2(sslSocket *ss,
const SECItem *sidBytes,
int *retErrCode);
Expand Down Expand Up @@ -8202,7 +8201,6 @@ ssl3_HandleClientHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
SECItem cookieBytes = { siBuffer, NULL, 0 };
SECItem suites = { siBuffer, NULL, 0 };
SECItem comps = { siBuffer, NULL, 0 };
PRBool canOfferSessionTicket = PR_FALSE;
PRBool isTLS13;

SSL_TRC(3, ("%d: SSL3[%d]: handle client_hello handshake",
Expand Down Expand Up @@ -8515,19 +8513,6 @@ ssl3_HandleClientHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
ss->sec.ci.sid = NULL;
}

/* We only send a session ticket extension if the client supports
* the extension and we are unable to do either a stateful or
* stateless resume.
*
* TODO: send a session ticket if performing a stateful
* resumption. (As per RFC4507, a server may issue a session
* ticket while doing a (stateless or stateful) session resume,
* but OpenSSL-0.9.8g does not accept session tickets while
* resuming.)
*/
if (ssl3_ExtensionNegotiated(ss, ssl_session_ticket_xtn) && sid == NULL) {
canOfferSessionTicket = PR_TRUE;
}

if (sid != NULL) {
/* We've found a session cache entry for this client.
Expand Down Expand Up @@ -8565,8 +8550,7 @@ ssl3_HandleClientHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) {
rv = tls13_HandleClientHelloPart2(ss, &suites, sid);
} else {
rv = ssl3_HandleClientHelloPart2(ss, &suites, &comps, sid,
canOfferSessionTicket);
rv = ssl3_HandleClientHelloPart2(ss, &suites, &comps, sid);
}
if (rv != SECSuccess) {
errCode = PORT_GetError();
Expand All @@ -8586,8 +8570,7 @@ static SECStatus
ssl3_HandleClientHelloPart2(sslSocket *ss,
SECItem *suites,
SECItem *comps,
sslSessionID *sid,
PRBool canOfferSessionTicket)
sslSessionID *sid)
{
PRBool haveSpecWriteLock = PR_FALSE;
PRBool haveXmitBufLock = PR_FALSE;
Expand Down Expand Up @@ -8679,15 +8662,6 @@ ssl3_HandleClientHelloPart2(sslSocket *ss,
goto alert_loser;
}

if (canOfferSessionTicket)
canOfferSessionTicket =
ssl3_KEASupportsTickets(ss->ssl3.hs.kea_def);

if (canOfferSessionTicket) {
ssl3_RegisterServerHelloExtensionSender(ss,
ssl_session_ticket_xtn, ssl3_SendSessionTicketXtn);
}

/* Select a compression algorithm. */
for (i = 0; i < comps->len; i++) {
SSLCompressionMethod method = (SSLCompressionMethod)comps->data[i];
Expand Down Expand Up @@ -8916,6 +8890,22 @@ ssl3_HandleClientHelloPart2(sslSocket *ss,
}
SSL_AtomicIncrementLong(&ssl3stats.hch_sid_cache_misses);

/* We only send a session ticket extension if the client supports
* the extension and we are unable to resume.
*
* TODO: send a session ticket if performing a stateful
* resumption. (As per RFC4507, a server may issue a session
* ticket while doing a (stateless or stateful) session resume,
* but OpenSSL-0.9.8g does not accept session tickets while
* resuming.)
*/
if (ssl3_ExtensionNegotiated(ss, ssl_session_ticket_xtn) &&
ssl3_KEASupportsTickets(ss->ssl3.hs.kea_def)) {
ssl3_RegisterServerHelloExtensionSender(ss,
ssl_session_ticket_xtn,
ssl3_SendSessionTicketXtn);
}

rv = ssl3_ServerCallSNICallback(ss);
if (rv != SECSuccess) {
/* The alert has already been sent. */
Expand Down

0 comments on commit 93d191f

Please sign in to comment.