Don't put sessions from renegotiations in the cache.
Rather than rely on Chromium to query SSL_initial_handshake_complete in the
callback (which didn't work anyway because the callback is called afterwards),
move the logic into BoringSSL. BoringSSL already enforces that clients never
offer resumptions on renegotiation (it wouldn't work well anyway as client
session cache lookup is external), so it's reasonable to also implement
in-library that sessions established on a renegotiation are not cached.
Add a bunch of tests that new_session_cb is called when expected.
BUG=501418
Change-Id: I42d44c82b043af72b60a0f8fdb57799e20f13ed5
Reviewed-on: https://boringssl-review.googlesource.com/5171
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index a7fee64..6571c2e 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -531,11 +531,16 @@
/* Remove write buffering now. */
ssl_free_wbio_buffer(s);
+ const int is_initial_handshake = !s->s3->initial_handshake_complete;
+
s->init_num = 0;
s->s3->tmp.in_false_start = 0;
s->s3->initial_handshake_complete = 1;
- ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
+ if (is_initial_handshake) {
+ /* Renegotiations do not participate in session resumption. */
+ ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
+ }
ret = 1;
/* s->server=0; */
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 856a599..b7b094f 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2945,10 +2945,6 @@
return 0;
}
-int SSL_initial_handshake_complete(const SSL *ssl) {
- return ssl->s3->initial_handshake_complete;
-}
-
int SSL_CTX_sess_connect(const SSL_CTX *ctx) { return 0; }
int SSL_CTX_sess_connect_good(const SSL_CTX *ctx) { return 0; }
int SSL_CTX_sess_connect_renegotiate(const SSL_CTX *ctx) { return 0; }
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 2dc7320..8242426 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -96,6 +96,7 @@
// signature_retries is the number of times an asynchronous sign operation has
// been retried.
unsigned signature_retries = 0;
+ bool got_new_session = false;
};
static void TestStateExFree(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
@@ -435,6 +436,13 @@
}
}
+static int NewSessionCallback(SSL *ssl, SSL_SESSION *session) {
+ GetTestState(ssl)->got_new_session = true;
+ // BoringSSL passes a reference to |session|.
+ SSL_SESSION_free(session);
+ return 1;
+}
+
// Connect returns a new socket connected to localhost on |port| or -1 on
// error.
static int Connect(uint16_t port) {
@@ -538,6 +546,7 @@
ssl_ctx->current_time_cb = CurrentTimeCallback;
SSL_CTX_set_info_callback(ssl_ctx.get(), InfoCallback);
+ SSL_CTX_sess_set_new_cb(ssl_ctx.get(), NewSessionCallback);
return ssl_ctx;
}
@@ -653,6 +662,18 @@
return false;
}
+ if (expect_handshake_done && !config->is_server) {
+ bool expect_new_session =
+ !config->expect_no_session &&
+ (!SSL_session_reused(ssl) || config->expect_ticket_renewal);
+ if (expect_new_session != GetTestState(ssl)->got_new_session) {
+ fprintf(stderr,
+ "new session was%s established, but we expected the opposite\n",
+ GetTestState(ssl)->got_new_session ? "" : " not");
+ return false;
+ }
+ }
+
if (config->is_server && !GetTestState(ssl)->early_callback_called) {
fprintf(stderr, "early callback not called\n");
return false;
@@ -959,6 +980,9 @@
return false;
}
+ // Reset the state to assert later that the callback isn't called in
+ // renegotations.
+ GetTestState(ssl.get())->got_new_session = false;
}
if (config->export_keying_material > 0) {
@@ -1068,6 +1092,13 @@
}
}
+ if (!config->is_server && !config->false_start &&
+ !config->implicit_handshake &&
+ GetTestState(ssl.get())->got_new_session) {
+ fprintf(stderr, "new session was established after the handshake\n");
+ return false;
+ }
+
if (out_session) {
out_session->reset(SSL_get1_session(ssl.get()));
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 2e2c132..3506e05 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -178,6 +178,9 @@
// newSessionsOnResume, if true, will cause resumeConfig to
// use a different session resumption context.
newSessionsOnResume bool
+ // noSessionCache, if true, will cause the server to run without a
+ // session cache.
+ noSessionCache bool
// sendPrefix sends a prefix on the socket before actually performing a
// handshake.
sendPrefix string
@@ -574,8 +577,10 @@
go func() { waitChan <- shim.Wait() }()
config := test.config
- config.ClientSessionCache = NewLRUClientSessionCache(1)
- config.ServerSessionCache = NewLRUServerSessionCache(1)
+ if !test.noSessionCache {
+ config.ClientSessionCache = NewLRUClientSessionCache(1)
+ config.ServerSessionCache = NewLRUServerSessionCache(1)
+ }
if test.testType == clientTest {
if len(config.Certificates) == 0 {
config.Certificates = []Certificate{getRSACertificate()}
@@ -604,7 +609,12 @@
if len(resumeConfig.Certificates) == 0 {
resumeConfig.Certificates = []Certificate{getRSACertificate()}
}
- if !test.newSessionsOnResume {
+ if test.newSessionsOnResume {
+ if !test.noSessionCache {
+ resumeConfig.ClientSessionCache = NewLRUClientSessionCache(1)
+ resumeConfig.ServerSessionCache = NewLRUServerSessionCache(1)
+ }
+ } else {
resumeConfig.SessionTicketKey = config.SessionTicketKey
resumeConfig.ClientSessionCache = config.ClientSessionCache
resumeConfig.ServerSessionCache = config.ServerSessionCache
@@ -1725,6 +1735,14 @@
shouldFail: true,
expectedError: ":TOO_MANY_WARNING_ALERTS:",
},
+ {
+ name: "EmptySessionID",
+ config: Config{
+ SessionTicketsDisabled: true,
+ },
+ noSessionCache: true,
+ flags: []string{"-expect-no-session"},
+ },
}
testCases = append(testCases, basicTests...)
@@ -2150,6 +2168,7 @@
RenewTicketOnResume: true,
},
},
+ flags: []string{"-expect-ticket-renewal"},
resumeSession: true,
})
tests = append(tests, testCase{
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index 031ad93..00dec32 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -83,6 +83,8 @@
{ "-no-legacy-server-connect", &TestConfig::no_legacy_server_connect },
{ "-tls-unique", &TestConfig::tls_unique },
{ "-use-async-private-key", &TestConfig::use_async_private_key },
+ { "-expect-ticket-renewal", &TestConfig::expect_ticket_renewal },
+ { "-expect-no-session", &TestConfig::expect_no_session },
};
const Flag<std::string> kStringFlags[] = {
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index e9af0de..e7230a7 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -80,6 +80,8 @@
bool no_legacy_server_connect = false;
bool tls_unique = false;
bool use_async_private_key = false;
+ bool expect_ticket_renewal = false;
+ bool expect_no_session = false;
};
bool ParseConfig(int argc, char **argv, TestConfig *out_config);
