Fix SSL_clear's interaction with session resumption.
Prior to 87eab4902d1003983079881d3ee8a66ec2eaccc2, due to some
confusions between configuration and connection state, SSL_clear had the
side effect of offering the previously established session on the new
connection.
wpa_supplicant relies on this behavior, so restore it for TLS 1.2 and
below and add a test. (This behavior is largely incompatible with TLS
1.3's post-handshake tickets, so it won't work in 1.3. It'll act as if
we configured an unresumable session instead.)
Change-Id: Iaee8c0afc1cb65c0ab7397435602732b901b1c2d
Reviewed-on: https://boringssl-review.googlesource.com/12632
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 47ecd48..eed82b0 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2877,6 +2877,15 @@
}
int SSL_clear(SSL *ssl) {
+ /* In OpenSSL, reusing a client |SSL| with |SSL_clear| causes the previously
+ * established session to be offered the next time around. wpa_supplicant
+ * depends on this behavior, so emulate it. */
+ SSL_SESSION *session = NULL;
+ if (!ssl->server && ssl->s3->established_session != NULL) {
+ session = ssl->s3->established_session;
+ SSL_SESSION_up_ref(session);
+ }
+
/* TODO(davidben): Some state on |ssl| is reset both in |SSL_new| and
* |SSL_clear| because it is per-connection state rather than configuration
* state. Per-connection state should be on |ssl->s3| and |ssl->d1| so it is
@@ -2902,6 +2911,7 @@
ssl->method->ssl_free(ssl);
if (!ssl->method->ssl_new(ssl)) {
+ SSL_SESSION_free(session);
return 0;
}
@@ -2909,6 +2919,11 @@
ssl->d1->mtu = mtu;
}
+ if (session != NULL) {
+ SSL_set_session(ssl, session);
+ SSL_SESSION_free(session);
+ }
+
return 1;
}
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index c2bb990..9bdaad2 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -1246,7 +1246,37 @@
PEM_read_bio_PrivateKey(bio.get(), nullptr, nullptr, nullptr));
}
-static bool ConnectClientAndServer(bssl::UniquePtr<SSL> *out_client, bssl::UniquePtr<SSL> *out_server,
+static bool CompleteHandshakes(SSL *client, SSL *server) {
+ // Drive both their handshakes to completion.
+ for (;;) {
+ int client_ret = SSL_do_handshake(client);
+ int client_err = SSL_get_error(client, client_ret);
+ if (client_err != SSL_ERROR_NONE &&
+ client_err != SSL_ERROR_WANT_READ &&
+ client_err != SSL_ERROR_WANT_WRITE) {
+ fprintf(stderr, "Client error: %d\n", client_err);
+ return false;
+ }
+
+ int server_ret = SSL_do_handshake(server);
+ int server_err = SSL_get_error(server, server_ret);
+ if (server_err != SSL_ERROR_NONE &&
+ server_err != SSL_ERROR_WANT_READ &&
+ server_err != SSL_ERROR_WANT_WRITE) {
+ fprintf(stderr, "Server error: %d\n", server_err);
+ return false;
+ }
+
+ if (client_ret == 1 && server_ret == 1) {
+ break;
+ }
+ }
+
+ return true;
+}
+
+static bool ConnectClientAndServer(bssl::UniquePtr<SSL> *out_client,
+ bssl::UniquePtr<SSL> *out_server,
SSL_CTX *client_ctx, SSL_CTX *server_ctx,
SSL_SESSION *session) {
bssl::UniquePtr<SSL> client(SSL_new(client_ctx)), server(SSL_new(server_ctx));
@@ -1266,29 +1296,8 @@
SSL_set_bio(client.get(), bio1, bio1);
SSL_set_bio(server.get(), bio2, bio2);
- // Drive both their handshakes to completion.
- for (;;) {
- int client_ret = SSL_do_handshake(client.get());
- int client_err = SSL_get_error(client.get(), client_ret);
- if (client_err != SSL_ERROR_NONE &&
- client_err != SSL_ERROR_WANT_READ &&
- client_err != SSL_ERROR_WANT_WRITE) {
- fprintf(stderr, "Client error: %d\n", client_err);
- return false;
- }
-
- int server_ret = SSL_do_handshake(server.get());
- int server_err = SSL_get_error(server.get(), server_ret);
- if (server_err != SSL_ERROR_NONE &&
- server_err != SSL_ERROR_WANT_READ &&
- server_err != SSL_ERROR_WANT_WRITE) {
- fprintf(stderr, "Server error: %d\n", server_err);
- return false;
- }
-
- if (client_ret == 1 && server_ret == 1) {
- break;
- }
+ if (!CompleteHandshakes(client.get(), server.get())) {
+ return false;
}
*out_client = std::move(client);
@@ -2719,6 +2728,65 @@
return true;
}
+static bool TestSSLClearSessionResumption(bool is_dtls,
+ const SSL_METHOD *method,
+ uint16_t version) {
+ // Skip this for TLS 1.3. TLS 1.3's ticket mechanism is incompatible with this
+ // API pattern.
+ if (version == TLS1_3_VERSION) {
+ return true;
+ }
+
+ bssl::UniquePtr<X509> cert = GetTestCertificate();
+ bssl::UniquePtr<EVP_PKEY> key = GetTestKey();
+ bssl::UniquePtr<SSL_CTX> server_ctx(SSL_CTX_new(method));
+ bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(method));
+ if (!cert || !key || !server_ctx || !client_ctx ||
+ !SSL_CTX_use_certificate(server_ctx.get(), cert.get()) ||
+ !SSL_CTX_use_PrivateKey(server_ctx.get(), key.get()) ||
+ !SSL_CTX_set_min_proto_version(client_ctx.get(), version) ||
+ !SSL_CTX_set_max_proto_version(client_ctx.get(), version) ||
+ !SSL_CTX_set_min_proto_version(server_ctx.get(), version) ||
+ !SSL_CTX_set_max_proto_version(server_ctx.get(), version)) {
+ return false;
+ }
+
+ // Connect a client and a server.
+ bssl::UniquePtr<SSL> client, server;
+ if (!ConnectClientAndServer(&client, &server, client_ctx.get(),
+ server_ctx.get(), nullptr /* no session */)) {
+ return false;
+ }
+
+ if (SSL_session_reused(client.get()) ||
+ SSL_session_reused(server.get())) {
+ fprintf(stderr, "Session unexpectedly reused.\n");
+ return false;
+ }
+
+ // Reset everything.
+ if (!SSL_clear(client.get()) ||
+ !SSL_clear(server.get())) {
+ fprintf(stderr, "SSL_clear failed.\n");
+ return false;
+ }
+
+ // Attempt to connect a second time.
+ if (!CompleteHandshakes(client.get(), server.get())) {
+ fprintf(stderr, "Could not reuse SSL objects.\n");
+ return false;
+ }
+
+ // |SSL_clear| should implicitly offer the previous session to the server.
+ if (!SSL_session_reused(client.get()) ||
+ !SSL_session_reused(server.get())) {
+ fprintf(stderr, "Session was not reused in second try.\n");
+ return false;
+ }
+
+ return true;
+}
+
static bool ForEachVersion(bool (*test_func)(bool is_dtls,
const SSL_METHOD *method,
uint16_t version)) {
@@ -2794,7 +2862,8 @@
!TestEarlyCallbackVersionSwitch() ||
!TestSetVersion() ||
!ForEachVersion(TestVersion) ||
- !ForEachVersion(TestALPNCipherAvailable)) {
+ !ForEachVersion(TestALPNCipherAvailable) ||
+ !ForEachVersion(TestSSLClearSessionResumption)) {
ERR_print_errors_fp(stderr);
return 1;
}
