From 5ef40c60f67e9060b33f02f49b366f508c0d09fa Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 23 Aug 2017 21:28:29 -0700 Subject: [PATCH] Mark renego-established sessions not resumable. We do not call the new_session callback on renego, but a consumer using SSL_get_session may still attempt to resume such a session. Leave the not_resumable flag unset. Also document this renegotiation restriction. Change-Id: I5361f522700b02edf5272ba5089c0777e5dafb09 Reviewed-on: https://boringssl-review.googlesource.com/19664 Commit-Queue: Adam Langley Reviewed-by: Adam Langley CQ-Verified: CQ bot account: commit-bot@chromium.org --- PORTING.md | 4 ++++ ssl/handshake_client.cc | 11 +++++------ ssl/ssl_lib.cc | 1 + ssl/test/bssl_shim.cc | 7 +++++++ 4 files changed, 17 insertions(+), 6 deletions(-) diff --git a/PORTING.md b/PORTING.md index ca9f6a444..eca7194e3 100644 --- a/PORTING.md +++ b/PORTING.md @@ -130,6 +130,10 @@ Things which do not work: * If a HelloRequest is received while `SSL_write` has unsent application data, the renegotiation is rejected. +* Renegotiation does not participate in session resumption. The client will + not offer a session on renegotiation or resume any session established by a + renegotiation handshake. + ### Lowercase hexadecimal BoringSSL's `BN_bn2hex` function uses lowercase hexadecimal digits instead of diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index e3c464180..6afd00b1b 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -508,7 +508,10 @@ int ssl3_connect(SSL_HANDSHAKE *hs) { ret = -1; goto end; } - ssl->s3->established_session->not_resumable = 0; + /* Renegotiations do not participate in session resumption. */ + if (!ssl->s3->initial_handshake_complete) { + ssl->s3->established_session->not_resumable = 0; + } hs->new_session.reset(); } @@ -517,12 +520,8 @@ int ssl3_connect(SSL_HANDSHAKE *hs) { break; case SSL_ST_OK: { - const int is_initial_handshake = !ssl->s3->initial_handshake_complete; ssl->s3->initial_handshake_complete = 1; - if (is_initial_handshake) { - /* Renegotiations do not participate in session resumption. */ - ssl_update_cache(hs, SSL_SESS_CACHE_CLIENT); - } + ssl_update_cache(hs, SSL_SESS_CACHE_CLIENT); ret = 1; ssl_do_info_callback(ssl, SSL_CB_HANDSHAKE_DONE, 1); diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 32ec272ea..9ecd7df64 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -218,6 +218,7 @@ void ssl_update_cache(SSL_HANDSHAKE *hs, int mode) { SSL_CTX *ctx = ssl->session_ctx; /* Never cache sessions with empty session IDs. */ if (ssl->s3->established_session->session_id_length == 0 || + ssl->s3->established_session->not_resumable || (ctx->session_cache_mode & mode) != mode) { return; } diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 8f4126e69..d7d2eface 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -2372,6 +2372,13 @@ static bool DoExchange(bssl::UniquePtr *out_session, SSL *ssl, return false; } + if (SSL_total_renegotiations(ssl) > 0 && + !SSL_get_session(ssl)->not_resumable) { + fprintf(stderr, + "Renegotiations should never produce resumable sessions.\n"); + return false; + } + if (SSL_total_renegotiations(ssl) != config->expect_total_renegotiations) { fprintf(stderr, "Expected %d renegotiations, got %d\n", config->expect_total_renegotiations, SSL_total_renegotiations(ssl));