The branch master has been updated
via c0537ebdf1e85e52923d05aa5d83496e97d121a3 (commit)
via 5d61491c8816ebad172340aafce713e9654dea94 (commit)
from c290853878ed2a6988521c01a8f675ed959ab5cc (commit)
- Log -----------------------------------------------------------------
commit c0537ebdf1e85e52923d05aa5d83496e97d121a3
Author: Matt Caswell <m...@openssl.org>
Date: Tue Aug 1 11:57:51 2017 +0100
Add a test to check we get a new session even if s->hit is true in TLSv1.3
In TLSv1.3 we can resume, but still get a new session. This adds a test to
make sure that is happening.
Reviewed-by: Rich Salz <rs...@openssl.org>
Reviewed-by: Ben Kaduk <ka...@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/4068)
commit 5d61491c8816ebad172340aafce713e9654dea94
Author: Matt Caswell <m...@openssl.org>
Date: Tue Aug 1 10:49:47 2017 +0100
Fix new_session_cb calls in TLSv1.3
If a new_session_cb is set then it was only ever getting invoked if !s->hit
is true. This is sensible for <=TLSv1.2 but does not work for TLSv1.3.
Fixes #4045
Reviewed-by: Rich Salz <rs...@openssl.org>
Reviewed-by: Ben Kaduk <ka...@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/4068)
-----------------------------------------------------------------------
Summary of changes:
ssl/ssl_lib.c | 7 +++---
ssl/statem/statem_clnt.c | 12 +++++-----
ssl/statem/statem_lib.c | 7 +++++-
test/sslapitest.c | 59 ++++++++++++++++++++++++++++++++----------------
4 files changed, 55 insertions(+), 30 deletions(-)
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index ab8e443..491023f 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -3156,10 +3156,11 @@ void ssl_update_cache(SSL *s, int mode)
return;
i = s->session_ctx->session_cache_mode;
- if ((i & mode) && (!s->hit)
- && ((i & SSL_SESS_CACHE_NO_INTERNAL_STORE)
+ if ((i & mode) != 0
+ && (!s->hit || SSL_IS_TLS13(s))
+ && ((i & SSL_SESS_CACHE_NO_INTERNAL_STORE) != 0
|| SSL_CTX_add_session(s->session_ctx, s->session))
- && (s->session_ctx->new_session_cb != NULL)) {
+ && s->session_ctx->new_session_cb != NULL) {
SSL_SESSION_up_ref(s->session);
if (!s->session_ctx->new_session_cb(s, s->session))
SSL_SESSION_free(s->session);
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 1e96022..5f6c6b0 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -2462,6 +2462,12 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL
*s, PACKET *pkt)
* We reused an existing session, so we need to replace it with a new
* one
*/
+ if ((new_sess = ssl_session_dup(s->session, 0)) == 0) {
+ al = SSL_AD_INTERNAL_ERROR;
+ SSLerr(SSL_F_TLS_PROCESS_NEW_SESSION_TICKET, ERR_R_MALLOC_FAILURE);
+ goto f_err;
+ }
+
if (i & SSL_SESS_CACHE_CLIENT) {
/*
* Remove the old session from the cache. We carry on if this fails
@@ -2469,12 +2475,6 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL
*s, PACKET *pkt)
SSL_CTX_remove_session(s->session_ctx, s->session);
}
- if ((new_sess = ssl_session_dup(s->session, 0)) == 0) {
- al = SSL_AD_INTERNAL_ERROR;
- SSLerr(SSL_F_TLS_PROCESS_NEW_SESSION_TICKET, ERR_R_MALLOC_FAILURE);
- goto f_err;
- }
-
SSL_SESSION_free(s->session);
s->session = new_sess;
}
diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index a6baf2a..08d5f41 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -1028,7 +1028,12 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst,
int clearbufs)
s->ctx->stats.sess_accept_good++;
s->handshake_func = ossl_statem_accept;
} else {
- ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
+ /*
+ * In TLSv1.3 we update the cache as part of processing the
+ * NewSessionTicket
+ */
+ if (!SSL_IS_TLS13(s))
+ ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
if (s->hit)
s->ctx->stats.sess_hit++;
diff --git a/test/sslapitest.c b/test/sslapitest.c
index d8324f8..a2917da 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -786,6 +786,11 @@ static void ssl_session_tear_down(SSL_SESSION_TEST_FIXTURE
fixture)
static int new_session_cb(SSL *ssl, SSL_SESSION *sess)
{
new_called++;
+ /*
+ * sess has been up-refed for us, but we don't actually need it so free it
+ * immediately.
+ */
+ SSL_SESSION_free(sess);
return 1;
}
@@ -842,6 +847,32 @@ static int execute_test_session(SSL_SESSION_TEST_FIXTURE
fix)
if (fix.use_ext_cache && (new_called != 1 || remove_called != 0))
goto end;
+#if !defined(OPENSSL_NO_TLS1_3)
+ new_called = remove_called = 0;
+ if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl2,
+ &clientssl2, NULL, NULL))
+ || !TEST_true(SSL_set_session(clientssl2, sess1))
+ || !TEST_true(create_ssl_connection(serverssl2, clientssl2,
+ SSL_ERROR_NONE))
+ || !TEST_true(SSL_session_reused(clientssl2)))
+ goto end;
+
+ /*
+ * In TLSv1.3 we should have created a new session even though we have
+ * resumed. The original session should also have been removed.
+ */
+ if (fix.use_ext_cache && !TEST_true((new_called == 1
+ && remove_called == 1)))
+ goto end;
+
+ SSL_SESSION_free(sess1);
+ if (!TEST_ptr(sess1 = SSL_get1_session(clientssl2)))
+ goto end;
+ shutdown_ssl_connection(serverssl2, clientssl2);
+ serverssl2 = clientssl2 = NULL;
+#endif
+
+ new_called = remove_called = 0;
if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl2,
&clientssl2, NULL, NULL))
|| !TEST_true(create_ssl_connection(serverssl2, clientssl2,
@@ -851,16 +882,17 @@ static int execute_test_session(SSL_SESSION_TEST_FIXTURE
fix)
if (!TEST_ptr(sess2 = SSL_get1_session(clientssl2)))
goto end;
- if (fix.use_ext_cache && (new_called != 2 || remove_called != 0))
+ if (fix.use_ext_cache && !TEST_true(new_called == 1 && remove_called == 0))
goto end;
+ new_called = remove_called = 0;
/*
* This should clear sess2 from the cache because it is a "bad" session.
* See SSL_set_session() documentation.
*/
if (!TEST_true(SSL_set_session(clientssl2, sess1)))
goto end;
- if (fix.use_ext_cache && (new_called != 2 || remove_called != 1))
+ if (fix.use_ext_cache && !TEST_true(new_called == 0 && remove_called == 1))
goto end;
if (!TEST_ptr_eq(SSL_get_session(clientssl2), sess1))
goto end;
@@ -870,35 +902,30 @@ static int execute_test_session(SSL_SESSION_TEST_FIXTURE
fix)
if (!TEST_true(SSL_CTX_add_session(cctx, sess2))
|| !TEST_true(SSL_CTX_remove_session(cctx, sess2)))
goto end;
-
- /*
- * This is for the purposes of internal cache testing...ignore the
- * counter for external cache
- */
- if (fix.use_ext_cache)
- remove_called--;
}
+ new_called = remove_called = 0;
/* This shouldn't be in the cache so should fail */
if (!TEST_false(SSL_CTX_remove_session(cctx, sess2)))
goto end;
- if (fix.use_ext_cache && (new_called != 2 || remove_called != 2))
+ if (fix.use_ext_cache && !TEST_true(new_called == 0 && remove_called == 1))
goto end;
#if !defined(OPENSSL_NO_TLS1_1) && !defined(OPENSSL_NO_TLS1_2)
+ new_called = remove_called = 0;
/* Force a connection failure */
SSL_CTX_set_max_proto_version(sctx, TLS1_1_VERSION);
if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl3,
&clientssl3, NULL, NULL))
|| !TEST_true(SSL_set_session(clientssl3, sess1))
- /* This should fail because of the mismatched protocol versions */
+ /* This should fail because of the mismatched protocol versions */
|| !TEST_false(create_ssl_connection(serverssl3, clientssl3,
SSL_ERROR_NONE)))
goto end;
/* We should have automatically removed the session from the cache */
- if (fix.use_ext_cache && (new_called != 2 || remove_called != 3))
+ if (fix.use_ext_cache && !TEST_true(new_called == 0 && remove_called == 1))
goto end;
/* Should succeed because it should not already be in the cache */
@@ -919,14 +946,6 @@ static int execute_test_session(SSL_SESSION_TEST_FIXTURE
fix)
#endif
SSL_SESSION_free(sess1);
SSL_SESSION_free(sess2);
-
- /*
- * Check if we need to remove any sessions up-refed for the external cache
- */
- if (new_called >= 1)
- SSL_SESSION_free(sess1);
- if (new_called >= 2)
- SSL_SESSION_free(sess2);
SSL_CTX_free(sctx);
SSL_CTX_free(cctx);
_____
openssl-commits mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-commits
