From 36ff232cf2bf5dfcaf9e60a8c492439428a243bb Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 14 Mar 2018 19:22:48 +0000 Subject: [PATCH] Change the default number of NewSessionTickets we send to 2 Reviewed-by: Viktor Dukhovni Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/5227) --- ssl/ssl_lib.c | 5 +- ssl/statem/statem_clnt.c | 8 -- ssl/statem/statem_srvr.c | 23 +++-- test/handshake_helper.c | 18 +++- test/sslapitest.c | 164 ++++++++++++++++++++++++++++++------ test/ssltestlib.c | 15 ++-- util/perl/TLSProxy/Proxy.pm | 6 ++ 7 files changed, 185 insertions(+), 54 deletions(-) diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 2c29d7f61c..c38fc58a5d 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -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); diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 60e987adb1..6c0f8be564 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -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; } diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index dfeba173a7..ce8cec185a 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -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); diff --git a/test/handshake_helper.c b/test/handshake_helper.c index b3d94bb1ee..3ebf64dfe3 100644 --- a/test/handshake_helper.c +++ b/test/handshake_helper.c @@ -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; } diff --git a/test/sslapitest.c b/test/sslapitest.c index 06d6cb2b68..626e26f52b 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c @@ -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); diff --git a/test/ssltestlib.c b/test/ssltestlib.c index c7689631f1..2ef4b5d432 100644 --- a/test/ssltestlib.c +++ b/test/ssltestlib.c @@ -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; diff --git a/util/perl/TLSProxy/Proxy.pm b/util/perl/TLSProxy/Proxy.pm index 8df0153d24..8c13520ec6 100644 --- a/util/perl/TLSProxy/Proxy.pm +++ b/util/perl/TLSProxy/Proxy.pm @@ -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; -- 2.25.1