Change the default number of NewSessionTickets we send to 2

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5227)
This commit is contained in:
Matt Caswell 2018-03-14 19:22:48 +00:00
parent 394159da60
commit 36ff232cf2
7 changed files with 185 additions and 54 deletions

View File

@ -591,6 +591,7 @@ int SSL_clear(SSL *s)
s->psksession_id = NULL;
s->psksession_id_len = 0;
s->hello_retry_request = 0;
s->sent_tickets = 0;
s->error = 0;
s->hit = 0;
@ -3034,8 +3035,8 @@ SSL_CTX *SSL_CTX_new(const SSL_METHOD *meth)
*/
ret->max_early_data = 0;
/* By default we send one session ticket automatically in TLSv1.3 */
ret->num_tickets = 1;
/* By default we send two session tickets automatically in TLSv1.3 */
ret->num_tickets = 2;
ssl_ctx_system_config(ret);

View File

@ -2590,7 +2590,6 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt)
* cache.
*/
if (SSL_IS_TLS13(s) || s->session->session_id_length > 0) {
int i = s->session_ctx->session_cache_mode;
SSL_SESSION *new_sess;
/*
* We reused an existing session, so we need to replace it with a new
@ -2603,13 +2602,6 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt)
goto err;
}
if (i & SSL_SESS_CACHE_CLIENT) {
/*
* Remove the old session from the cache. We carry on if this fails
*/
SSL_CTX_remove_session(s->session_ctx, s->session);
}
SSL_SESSION_free(s->session);
s->session = new_sess;
}

View File

@ -507,6 +507,9 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s)
/* Fall through */
case TLS_ST_SW_KEY_UPDATE:
st->hand_state = TLS_ST_OK;
return WRITE_TRAN_CONTINUE;
case TLS_ST_SW_SESSION_TICKET:
/* In a resumption we only ever send a maximum of one new ticket.
* Following an initial handshake we send the number of tickets we have
@ -708,7 +711,7 @@ WORK_STATE ossl_statem_server_pre_work(SSL *s, WORK_STATE wst)
return WORK_FINISHED_CONTINUE;
case TLS_ST_SW_SESSION_TICKET:
if (SSL_IS_TLS13(s)) {
if (SSL_IS_TLS13(s) && s->sent_tickets == 0) {
/*
* Actually this is the end of the handshake, but we're going
* straight into writing the session ticket out. So we finish off
@ -3687,12 +3690,16 @@ MSG_PROCESS_RETURN tls_process_client_certificate(SSL *s, PACKET *pkt)
sk = NULL;
/* Save the current hash state for when we receive the CertificateVerify */
if (SSL_IS_TLS13(s)
&& !ssl_handshake_hash(s, s->cert_verify_hash,
sizeof(s->cert_verify_hash),
&s->cert_verify_hash_len)) {
/* SSLfatal() already called */
goto err;
if (SSL_IS_TLS13(s)) {
if (!ssl_handshake_hash(s, s->cert_verify_hash,
sizeof(s->cert_verify_hash),
&s->cert_verify_hash_len)) {
/* SSLfatal() already called */
goto err;
}
/* Resend session tickets */
s->sent_tickets = 0;
}
ret = MSG_PROCESS_CONTINUE_READING;
@ -3989,7 +3996,6 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt)
goto err;
}
if (SSL_IS_TLS13(s)) {
ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
if (!tls_construct_extensions(s, pkt,
SSL_EXT_TLS1_3_NEW_SESSION_TICKET,
NULL, 0)) {
@ -3997,6 +4003,7 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt)
goto err;
}
s->sent_tickets++;
ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
}
EVP_CIPHER_CTX_free(ctx);
HMAC_CTX_free(hctx);

View File

@ -1403,7 +1403,7 @@ static HANDSHAKE_RESULT *do_handshake_internal(
HANDSHAKE_EX_DATA server_ex_data, client_ex_data;
CTX_DATA client_ctx_data, server_ctx_data, server2_ctx_data;
HANDSHAKE_RESULT *ret = HANDSHAKE_RESULT_new();
int client_turn = 1, client_turn_count = 0;
int client_turn = 1, client_turn_count = 0, client_wait_count = 0;
connect_phase_t phase = HANDSHAKE;
handshake_status_t status = HANDSHAKE_RETRY;
const unsigned char* tick = NULL;
@ -1586,9 +1586,19 @@ static HANDSHAKE_RESULT *do_handshake_internal(
ret->result = SSL_TEST_INTERNAL_ERROR;
goto err;
}
/* Continue. */
client_turn ^= 1;
if (client_turn && server.status == PEER_SUCCESS) {
/*
* The server may finish before the client because the
* client spends some turns processing NewSessionTickets.
*/
if (client_wait_count++ >= 2) {
ret->result = SSL_TEST_INTERNAL_ERROR;
goto err;
}
} else {
/* Continue. */
client_turn ^= 1;
}
}
break;
}

View File

@ -882,10 +882,14 @@ static int execute_test_session(int maxprot, int use_int_cache,
SSL *serverssl3 = NULL, *clientssl3 = NULL;
# endif
SSL_SESSION *sess1 = NULL, *sess2 = NULL;
int testresult = 0;
int testresult = 0, numnewsesstick = 1;
new_called = remove_called = 0;
/* TLSv1.3 sends 2 NewSessionTickets */
if (maxprot == TLS1_3_VERSION)
numnewsesstick = 2;
if (!TEST_true(create_ssl_ctx_pair(TLS_server_method(), TLS_client_method(),
TLS1_VERSION, TLS_MAX_VERSION,
&sctx, &cctx, cert, privkey)))
@ -923,7 +927,9 @@ static int execute_test_session(int maxprot, int use_int_cache,
if (use_int_cache && !TEST_false(SSL_CTX_add_session(cctx, sess1)))
goto end;
if (use_ext_cache
&& (!TEST_int_eq(new_called, 1) || !TEST_int_eq(remove_called, 0)))
&& (!TEST_int_eq(new_called, numnewsesstick)
|| !TEST_int_eq(remove_called, 0)))
goto end;
new_called = remove_called = 0;
@ -938,11 +944,11 @@ static int execute_test_session(int maxprot, int use_int_cache,
if (maxprot == TLS1_3_VERSION) {
/*
* In TLSv1.3 we should have created a new session even though we have
* resumed. The original session should also have been removed.
* resumed.
*/
if (use_ext_cache
&& (!TEST_int_eq(new_called, 1)
|| !TEST_int_eq(remove_called, 1)))
|| !TEST_int_eq(remove_called, 0)))
goto end;
} else {
/*
@ -972,7 +978,8 @@ static int execute_test_session(int maxprot, int use_int_cache,
goto end;
if (use_ext_cache
&& (!TEST_int_eq(new_called, 1) || !TEST_int_eq(remove_called, 0)))
&& (!TEST_int_eq(new_called, numnewsesstick)
|| !TEST_int_eq(remove_called, 0)))
goto end;
new_called = remove_called = 0;
@ -1072,7 +1079,7 @@ static int execute_test_session(int maxprot, int use_int_cache,
if (use_ext_cache) {
SSL_SESSION *tmp = sess2;
if (!TEST_int_eq(new_called, 1)
if (!TEST_int_eq(new_called, numnewsesstick)
|| !TEST_int_eq(remove_called, 0)
|| !TEST_int_eq(get_called, 0))
goto end;
@ -1105,10 +1112,6 @@ static int execute_test_session(int maxprot, int use_int_cache,
goto end;
if (maxprot == TLS1_3_VERSION) {
/*
* Every time we issue a NewSessionTicket we are creating a new
* session for next time in TLSv1.3
*/
if (!TEST_int_eq(new_called, 1)
|| !TEST_int_eq(get_called, 0))
goto end;
@ -1181,6 +1184,101 @@ static int test_session_with_both_cache(void)
#endif
}
SSL_SESSION *sesscache[9];
static int new_cachesession_cb(SSL *ssl, SSL_SESSION *sess)
{
sesscache[new_called++] = sess;
return 1;
}
static int test_tickets(int idx)
{
SSL_CTX *sctx = NULL, *cctx = NULL;
SSL *serverssl = NULL, *clientssl = NULL;
int testresult = 0, i;
size_t j;
/* idx is the test number, but also the number of tickets we want */
new_called = 0;
if (!TEST_true(create_ssl_ctx_pair(TLS_server_method(), TLS_client_method(),
TLS1_VERSION, TLS_MAX_VERSION, &sctx,
&cctx, cert, privkey))
|| !TEST_true(SSL_CTX_set_num_tickets(sctx, idx)))
goto end;
SSL_CTX_set_session_cache_mode(cctx, SSL_SESS_CACHE_CLIENT
| SSL_SESS_CACHE_NO_INTERNAL_STORE);
SSL_CTX_sess_set_new_cb(cctx, new_cachesession_cb);
if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl,
&clientssl, NULL, NULL)))
goto end;
SSL_force_post_handshake_auth(clientssl);
if (!TEST_true(create_ssl_connection(serverssl, clientssl,
SSL_ERROR_NONE))
/* Check we got the number of tickets we were expecting */
|| !TEST_int_eq(idx, new_called))
goto end;
/* After a post-handshake authentication we should get new tickets issued */
SSL_set_verify(serverssl, SSL_VERIFY_PEER, NULL);
if (!TEST_true(SSL_verify_client_post_handshake(serverssl)))
goto end;
/* Start handshake on the server and client */
if (!TEST_int_eq(SSL_do_handshake(serverssl), 1)
|| !TEST_int_le(SSL_read(clientssl, NULL, 0), 0)
|| !TEST_int_le(SSL_read(serverssl, NULL, 0), 0)
|| !TEST_true(create_ssl_connection(serverssl, clientssl,
SSL_ERROR_NONE))
|| !TEST_int_eq(idx * 2, new_called))
goto end;
SSL_CTX_sess_set_new_cb(cctx, NULL);
SSL_shutdown(clientssl);
SSL_shutdown(serverssl);
SSL_free(serverssl);
SSL_free(clientssl);
serverssl = clientssl = NULL;
/* Test that we can resume with all the tickets we got given */
for (i = 0; i < new_called; i++) {
if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl,
&clientssl, NULL, NULL))
|| !TEST_true(SSL_set_session(clientssl, sesscache[i]))
|| !TEST_true(create_ssl_connection(serverssl, clientssl,
SSL_ERROR_NONE))
|| !TEST_true(SSL_session_reused(clientssl)))
goto end;
SSL_shutdown(clientssl);
SSL_shutdown(serverssl);
SSL_free(serverssl);
SSL_free(clientssl);
serverssl = clientssl = NULL;
SSL_SESSION_free(sesscache[i]);
sesscache[i] = NULL;
}
testresult = 1;
end:
SSL_free(serverssl);
SSL_free(clientssl);
for (j = 0; j < OSSL_NELEM(sesscache); j++)
SSL_SESSION_free(sesscache[j]);
SSL_CTX_free(sctx);
SSL_CTX_free(cctx);
return testresult;
}
#define USE_NULL 0
#define USE_BIO_1 1
#define USE_BIO_2 2
@ -1198,7 +1296,6 @@ static int test_session_with_both_cache(void)
# define TOTAL_CONN_FAIL_SSL_SET_BIO_TESTS 0
#endif
#define TOTAL_SSL_SET_BIO_TESTS TOTAL_NO_CONN_SSL_SET_BIO_TESTS \
+ TOTAL_CONN_SUCCESS_SSL_SET_BIO_TESTS \
+ TOTAL_CONN_FAIL_SSL_SET_BIO_TESTS
@ -1933,10 +2030,13 @@ static int test_early_data_read_write(int idx)
goto end;
/*
* Make sure we process the NewSessionTicket. This arrives post-handshake.
* We attempt a read which we do not expect to return any data.
* Make sure we process the two NewSessionTickets. These arrive
* post-handshake. We attempt reads which we do not expect to return any
* data.
*/
if (!TEST_false(SSL_read_ex(clientssl, buf, sizeof(buf), &readbytes)))
if (!TEST_false(SSL_read_ex(clientssl, buf, sizeof(buf), &readbytes))
|| !TEST_false(SSL_read_ex(clientssl, buf, sizeof(buf),
&readbytes)))
goto end;
/* Server should be able to write normal data */
@ -3392,9 +3492,10 @@ static int test_custom_exts(int tst)
|| (tst == 2 && snicb != 1))
goto end;
} else {
/* In this case there 2 NewSessionTicket messages created */
if (clntaddnewcb != 1
|| clntparsenewcb != 4
|| srvaddnewcb != 4
|| clntparsenewcb != 5
|| srvaddnewcb != 5
|| srvparsenewcb != 1)
goto end;
}
@ -3438,10 +3539,13 @@ static int test_custom_exts(int tst)
|| srvparsenewcb != 2)
goto end;
} else {
/* No Certificate message extensions in the resumption handshake */
/*
* No Certificate message extensions in the resumption handshake,
* 2 NewSessionTickets in the initial handshake, 1 in the resumption
*/
if (clntaddnewcb != 2
|| clntparsenewcb != 7
|| srvaddnewcb != 7
|| clntparsenewcb != 8
|| srvaddnewcb != 8
|| srvparsenewcb != 2)
goto end;
}
@ -4205,14 +4309,16 @@ static struct info_cb_states_st {
{SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
{SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL},
{SSL_CB_EXIT, NULL}, {SSL_CB_ALERT, NULL},
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "},
{SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"}, {SSL_CB_LOOP, "TWSH"},
{SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TWFIN"},
{SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"},
{SSL_CB_LOOP, "TRFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL},
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TWST"},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
{SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
{SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"},
{SSL_CB_LOOP, "TWSH"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"},
{SSL_CB_LOOP, "TWFIN"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL},
{SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
{SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL},
{SSL_CB_EXIT, NULL}, {0, NULL},
}, {
/* TLSv1.3 client followed by resumption */
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "},
@ -4223,6 +4329,9 @@ static struct info_cb_states_st {
{SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "SSLOK "},
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
{SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
{SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TWCH"}, {SSL_CB_EXIT, NULL},
{SSL_CB_LOOP, "TWCH"}, {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"},
@ -4856,6 +4965,9 @@ int setup_tests(void)
ADD_TEST(test_session_with_only_int_cache);
ADD_TEST(test_session_with_only_ext_cache);
ADD_TEST(test_session_with_both_cache);
#ifndef OPENSSL_NO_TLS1_3
ADD_ALL_TESTS(test_tickets, 3);
#endif
ADD_ALL_TESTS(test_ssl_set_bio, TOTAL_SSL_SET_BIO_TESTS);
ADD_TEST(test_ssl_bio_pop_next_bio);
ADD_TEST(test_ssl_bio_pop_ssl_bio);

View File

@ -682,7 +682,7 @@ int create_ssl_objects(SSL_CTX *serverctx, SSL_CTX *clientctx, SSL **sssl,
int create_ssl_connection(SSL *serverssl, SSL *clientssl, int want)
{
int retc = -1, rets = -1, err, abortctr = 0;
int retc = -1, rets = -1, err, abortctr = 0, i;
int clienterr = 0, servererr = 0;
unsigned char buf;
size_t readbytes;
@ -741,13 +741,16 @@ int create_ssl_connection(SSL *serverssl, SSL *clientssl, int want)
/*
* We attempt to read some data on the client side which we expect to fail.
* This will ensure we have received the NewSessionTicket in TLSv1.3 where
* appropriate.
* appropriate. We do this twice because there are 2 NewSesionTickets.
*/
if (SSL_read_ex(clientssl, &buf, sizeof(buf), &readbytes) > 0) {
if (!TEST_ulong_eq(readbytes, 0))
for (i = 0; i < 2; i++) {
if (SSL_read_ex(clientssl, &buf, sizeof(buf), &readbytes) > 0) {
if (!TEST_ulong_eq(readbytes, 0))
return 0;
} else if (!TEST_int_eq(SSL_get_error(clientssl, 0),
SSL_ERROR_WANT_READ)) {
return 0;
} else if (!TEST_int_eq(SSL_get_error(clientssl, 0), SSL_ERROR_WANT_READ)) {
return 0;
}
}
return 1;

View File

@ -220,6 +220,12 @@ sub start
my $execcmd = $self->execute
." s_server -max_protocol TLSv1.3 -no_comp -rev -engine ossltest"
#In TLSv1.3 we issue two session tickets. The default session id
#callback gets confused because the ossltest engine causes the same
#session id to be created twice due to the changed random number
#generation. Using "-ext_cache" replaces the default callback with a
#different one that doesn't get confused.
." -ext_cache"
." -accept $self->{server_addr}:0"
." -cert ".$self->cert." -cert2 ".$self->cert
." -naccept ".$self->serverconnects;