Move session ID assignment out of ssl_get_new_session.
It's kind of weird that we assign a session ID, based on whether we
detect the handshake wants stateful resumption, and then erase it
afterwards.
Also remove the is_server parameter, which we can get from hs.
Change-Id: I94ac817c63abb08a457e0e0c29f5c2d2b60aa498
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47444
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 4fbb48a..c7cadec 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -663,7 +663,7 @@
// The session wasn't resumed. Create a fresh SSL_SESSION to
// fill out.
ssl_set_session(ssl, NULL);
- if (!ssl_get_new_session(hs, 0 /* client */)) {
+ if (!ssl_get_new_session(hs)) {
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
return ssl_hs_error;
}
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index 02603a1..eaf3a5e 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -892,14 +892,17 @@
hs->can_release_private_key = true;
} else {
hs->ticket_expected = tickets_supported;
- ssl_set_session(ssl, NULL);
- if (!ssl_get_new_session(hs, 1 /* server */)) {
+ ssl_set_session(ssl, nullptr);
+ if (!ssl_get_new_session(hs)) {
return ssl_hs_error;
}
- // Clear the session ID if we want the session to be single-use.
- if (!(ssl->ctx->session_cache_mode & SSL_SESS_CACHE_SERVER)) {
- hs->new_session->session_id_length = 0;
+ // Assign a session ID if not using session tickets.
+ if (!hs->ticket_expected &&
+ (ssl->ctx->session_cache_mode & SSL_SESS_CACHE_SERVER)) {
+ hs->new_session->session_id_length = SSL3_SSL_SESSION_ID_LENGTH;
+ RAND_bytes(hs->new_session->session_id,
+ hs->new_session->session_id_length);
}
}
diff --git a/ssl/internal.h b/ssl/internal.h
index 084e9c3..a753329 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -2966,7 +2966,7 @@
bool ssl_compare_public_and_private_key(const EVP_PKEY *pubkey,
const EVP_PKEY *privkey);
bool ssl_cert_check_private_key(const CERT *cert, const EVP_PKEY *privkey);
-int ssl_get_new_session(SSL_HANDSHAKE *hs, int is_server);
+bool ssl_get_new_session(SSL_HANDSHAKE *hs);
int ssl_encrypt_ticket(SSL_HANDSHAKE *hs, CBB *out, const SSL_SESSION *session);
int ssl_ctx_rotate_ticket_encryption_key(SSL_CTX *ctx);
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index 91b2fff..41df63f 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -350,19 +350,19 @@
session->cipher);
}
-int ssl_get_new_session(SSL_HANDSHAKE *hs, int is_server) {
+bool ssl_get_new_session(SSL_HANDSHAKE *hs) {
SSL *const ssl = hs->ssl;
if (ssl->mode & SSL_MODE_NO_SESSION_CREATION) {
OPENSSL_PUT_ERROR(SSL, SSL_R_SESSION_MAY_NOT_BE_CREATED);
- return 0;
+ return false;
}
UniquePtr<SSL_SESSION> session = ssl_session_new(ssl->ctx->x509_method);
if (session == NULL) {
- return 0;
+ return false;
}
- session->is_server = is_server;
+ session->is_server = ssl->server;
session->ssl_version = ssl->version;
session->is_quic = ssl->quic_method != nullptr;
@@ -384,24 +384,9 @@
session->auth_timeout = ssl->session_ctx->session_timeout;
}
- if (is_server) {
- if (hs->ticket_expected || version >= TLS1_3_VERSION) {
- // Don't set session IDs for sessions resumed with tickets. This will keep
- // them out of the session cache.
- session->session_id_length = 0;
- } else {
- session->session_id_length = SSL3_SSL_SESSION_ID_LENGTH;
- if (!RAND_bytes(session->session_id, session->session_id_length)) {
- return 0;
- }
- }
- } else {
- session->session_id_length = 0;
- }
-
if (hs->config->cert->sid_ctx_length > sizeof(session->sid_ctx)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- return 0;
+ return false;
}
OPENSSL_memcpy(session->sid_ctx, hs->config->cert->sid_ctx,
hs->config->cert->sid_ctx_length);
@@ -413,7 +398,7 @@
hs->new_session = std::move(session);
ssl_set_session(ssl, NULL);
- return 1;
+ return true;
}
int ssl_ctx_rotate_ticket_encryption_key(SSL_CTX *ctx) {
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index a4a16fe..dae4bef 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -401,7 +401,7 @@
// Resumption incorporates fresh key material, so refresh the timeout.
ssl_session_renew_timeout(ssl, hs->new_session.get(),
ssl->session_ctx->session_psk_dhe_timeout);
- } else if (!ssl_get_new_session(hs, 0)) {
+ } else if (!ssl_get_new_session(hs)) {
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
return ssl_hs_error;
}
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index 8a24d6f..ba7b4cf 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -377,7 +377,7 @@
&offered_ticket, msg, &client_hello)) {
case ssl_ticket_aead_ignore_ticket:
assert(!session);
- if (!ssl_get_new_session(hs, 1 /* server */)) {
+ if (!ssl_get_new_session(hs)) {
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
return ssl_hs_error;
}
