Remove all the logic around custom session IDs and retrying on collisions.
A random 32-byte (so 256-bit) session ID is never going to collide with
an existing one. (And, if it does, SSL_CTX_add_session does account for
this, so the server won't explode. Just attempting to resume some
session will fail.)
That logic didn't completely work anyway as it didn't account for
external session caches or multiple connections picking the same ID in
parallel (generation and insertion happen at different times) or
multiple servers sharing one cache. In theory one could fix this by
passing in a sufficiently clever generate_session_id, but no one does
that.
I found no callers of these functions, so just remove them altogether.
Change-Id: I8500c592cf4676de6d7194d611b99e9e76f150a7
Reviewed-on: https://boringssl-review.googlesource.com/6318
Reviewed-by: Adam Langley <alangley@gmail.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index c2dd336..0b98b11 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1657,33 +1657,6 @@
* when the lookup has completed. */
OPENSSL_EXPORT SSL_SESSION *SSL_magic_pending_session_ptr(void);
-/* GEN_SESSION_CB is a callback to generate session IDs for |ssl|. It returns
- * one on success and zero on error. On success, the generated ID is written to
- * |id| and |*id_len| set to the length. On entry, |*id_len| is the maximum
- * length of the ID, but the callback may shorten it if desired. It is an error
- * for the callback to set the size to zero.
- *
- * Callbacks may use |SSL_has_matching_session_id| to check that the generated
- * ID is unique. */
-typedef int (*GEN_SESSION_CB)(const SSL *ssl, uint8_t *id, unsigned *id_len);
-
-/* SSL_CTX_set_generate_session_id sets the session ID callback of |ctx| to
- * |cb| and returns one. It will be called on the server when establishing a new
- * session. */
-OPENSSL_EXPORT int SSL_CTX_set_generate_session_id(SSL_CTX *ctx,
- GEN_SESSION_CB cb);
-
-/* SSL_set_generate_session_id sets the session ID callback of |ssl| to |cb| and
- * returns one. It will be called on the server when establishing a new
- * session. */
-OPENSSL_EXPORT int SSL_set_generate_session_id(SSL *ssl, GEN_SESSION_CB cb);
-
-/* SSL_has_matching_session_id returns one if |ssl|'s session cache has a
- * session of value |id| and zero otherwise. */
-OPENSSL_EXPORT int SSL_has_matching_session_id(const SSL *ssl,
- const uint8_t *id,
- unsigned id_len);
-
/* Session tickets.
*
@@ -3571,9 +3544,6 @@
int (*default_verify_callback)(
int ok, X509_STORE_CTX *ctx); /* called 'verify_callback' in the SSL */
- /* Default generate session ID callback. */
- GEN_SESSION_CB generate_session_id;
-
X509_VERIFY_PARAM *param;
/* select_certificate_cb is called before most ClientHello processing and
@@ -3790,9 +3760,6 @@
/* This can also be in the session once a session is established */
SSL_SESSION *session;
- /* Default generate session ID callback. */
- GEN_SESSION_CB generate_session_id;
-
/* Used in SSL2 and SSL3 */
int verify_mode; /* 0 don't care about verify failure.
* 1 fail if verify fails */
diff --git a/ssl/internal.h b/ssl/internal.h
index 21fed62..a4e2689 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -957,7 +957,7 @@
CERT *ssl_cert_dup(CERT *cert);
void ssl_cert_clear_certs(CERT *c);
void ssl_cert_free(CERT *c);
-int ssl_get_new_session(SSL *s, int session);
+int ssl_get_new_session(SSL *ssl, int is_server);
enum ssl_session_result_t {
ssl_session_success,
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 242ac26..1c9f105 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -811,7 +811,7 @@
/* The session wasn't resumed. Create a fresh SSL_SESSION to
* fill out. */
s->hit = 0;
- if (!ssl_get_new_session(s, 0)) {
+ if (!ssl_get_new_session(s, 0 /* client */)) {
goto f_err;
}
/* Note: session_id could be empty. */
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 637e6ed..2c915e6 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -945,7 +945,7 @@
s->verify_result = s->session->verify_result;
} else {
- if (!ssl_get_new_session(s, 1)) {
+ if (!ssl_get_new_session(s, 1 /* server */)) {
goto err;
}
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 6c4a847..59a3a50 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -385,7 +385,6 @@
assert(s->sid_ctx_length <= sizeof s->sid_ctx);
memcpy(&s->sid_ctx, &ctx->sid_ctx, sizeof(s->sid_ctx));
s->verify_callback = ctx->default_verify_callback;
- s->generate_session_id = ctx->generate_session_id;
s->param = X509_VERIFY_PARAM_new();
if (!s->param) {
@@ -943,39 +942,6 @@
return 1;
}
-int SSL_CTX_set_generate_session_id(SSL_CTX *ctx, GEN_SESSION_CB cb) {
- ctx->generate_session_id = cb;
- return 1;
-}
-
-int SSL_set_generate_session_id(SSL *ssl, GEN_SESSION_CB cb) {
- ssl->generate_session_id = cb;
- return 1;
-}
-
-int SSL_has_matching_session_id(const SSL *ssl, const uint8_t *id,
- unsigned id_len) {
- /* A quick examination of SSL_SESSION_hash and SSL_SESSION_cmp shows how we
- * can "construct" a session to give us the desired check - ie. to find if
- * there's a session in the hash table that would conflict with any new
- * session built out of this id/id_len and the ssl_version in use by this
- * SSL. */
- SSL_SESSION r, *p;
-
- if (id_len > sizeof r.session_id) {
- return 0;
- }
-
- r.ssl_version = ssl->version;
- r.session_id_length = id_len;
- memcpy(r.session_id, id, id_len);
-
- CRYPTO_MUTEX_lock_read(&ssl->ctx->lock);
- p = lh_SSL_SESSION_retrieve(ssl->ctx->sessions, &r);
- CRYPTO_MUTEX_unlock(&ssl->ctx->lock);
- return p != NULL;
-}
-
int SSL_CTX_set_purpose(SSL_CTX *ctx, int purpose) {
return X509_VERIFY_PARAM_set_purpose(ctx->param, purpose);
}
diff --git a/ssl/ssl_session.c b/ssl/ssl_session.c
index 4c2e464..6b17eed 100644
--- a/ssl/ssl_session.c
+++ b/ssl/ssl_session.c
@@ -297,142 +297,63 @@
return CRYPTO_get_ex_data(&session->ex_data, idx);
}
-/* Even with SSLv2, we have 16 bytes (128 bits) of session ID space.
- * SSLv3/TLSv1 has 32 bytes (256 bits). As such, filling the ID with random
- * gunk repeatedly until we have no conflict is going to complete in one
- * iteration pretty much "most" of the time (btw: understatement). So, if it
- * takes us 10 iterations and we still can't avoid a conflict - well that's a
- * reasonable point to call it quits. Either the RAND code is broken or someone
- * is trying to open roughly very close to 2^128 (or 2^256) SSL sessions to our
- * server. How you might store that many sessions is perhaps a more interesting
- * question ... */
-static int def_generate_session_id(const SSL *ssl, uint8_t *id,
- unsigned *id_len) {
- static const unsigned kMaxAttempts = 10;
- unsigned retry = 0;
- do {
- if (!RAND_bytes(id, *id_len)) {
- return 0;
- }
- } while (SSL_has_matching_session_id(ssl, id, *id_len) &&
- (++retry < kMaxAttempts));
-
- if (retry < kMaxAttempts) {
- return 1;
- }
-
- /* else - woops a session_id match */
- /* XXX We should also check the external cache -- but the probability of a
- * collision is negligible, and we could not prevent the concurrent creation
- * of sessions with identical IDs since we currently don't have means to
- * atomically check whether a session ID already exists and make a
- * reservation for it if it does not (this problem applies to the internal
- * cache as well). */
- return 0;
-}
-
-int ssl_get_new_session(SSL *s, int session) {
- /* This gets used by clients and servers. */
-
- unsigned int tmp;
- SSL_SESSION *ss = NULL;
- GEN_SESSION_CB cb = def_generate_session_id;
-
- if (s->mode & SSL_MODE_NO_SESSION_CREATION) {
+int ssl_get_new_session(SSL *ssl, int is_server) {
+ if (ssl->mode & SSL_MODE_NO_SESSION_CREATION) {
OPENSSL_PUT_ERROR(SSL, SSL_R_SESSION_MAY_NOT_BE_CREATED);
return 0;
}
- ss = SSL_SESSION_new();
- if (ss == NULL) {
+ SSL_SESSION *session = SSL_SESSION_new();
+ if (session == NULL) {
return 0;
}
/* If the context has a default timeout, use it over the default. */
- if (s->initial_ctx->session_timeout != 0) {
- ss->timeout = s->initial_ctx->session_timeout;
+ if (ssl->initial_ctx->session_timeout != 0) {
+ session->timeout = ssl->initial_ctx->session_timeout;
}
- SSL_SESSION_free(s->session);
- s->session = NULL;
+ session->ssl_version = ssl->version;
- if (session) {
- if (s->version == SSL3_VERSION || s->version == TLS1_VERSION ||
- s->version == TLS1_1_VERSION || s->version == TLS1_2_VERSION ||
- s->version == DTLS1_VERSION || s->version == DTLS1_2_VERSION) {
- ss->ssl_version = s->version;
- ss->session_id_length = SSL3_SSL_SESSION_ID_LENGTH;
+ if (is_server) {
+ if (ssl->tlsext_ticket_expected) {
+ /* 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 {
- OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_SSL_VERSION);
- SSL_SESSION_free(ss);
- return 0;
+ session->session_id_length = SSL3_SSL_SESSION_ID_LENGTH;
+ if (!RAND_bytes(session->session_id, session->session_id_length)) {
+ goto err;
+ }
}
- /* If RFC4507 ticket use empty session ID */
- if (s->tlsext_ticket_expected) {
- ss->session_id_length = 0;
- goto sess_id_done;
- }
-
- /* Choose which callback will set the session ID */
- if (s->generate_session_id) {
- cb = s->generate_session_id;
- } else if (s->initial_ctx->generate_session_id) {
- cb = s->initial_ctx->generate_session_id;
- }
-
- /* Choose a session ID */
- tmp = ss->session_id_length;
- if (!cb(s, ss->session_id, &tmp)) {
- /* The callback failed */
- OPENSSL_PUT_ERROR(SSL, SSL_R_SSL_SESSION_ID_CALLBACK_FAILED);
- SSL_SESSION_free(ss);
- return 0;
- }
-
- /* Don't allow the callback to set the session length to zero. nor set it
- * higher than it was. */
- if (!tmp || tmp > ss->session_id_length) {
- /* The callback set an illegal length */
- OPENSSL_PUT_ERROR(SSL, SSL_R_SSL_SESSION_ID_HAS_BAD_LENGTH);
- SSL_SESSION_free(ss);
- return 0;
- }
-
- ss->session_id_length = tmp;
- /* Finally, check for a conflict */
- if (SSL_has_matching_session_id(s, ss->session_id, ss->session_id_length)) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_SSL_SESSION_ID_CONFLICT);
- SSL_SESSION_free(ss);
- return 0;
- }
-
- sess_id_done:
- if (s->tlsext_hostname) {
- ss->tlsext_hostname = BUF_strdup(s->tlsext_hostname);
- if (ss->tlsext_hostname == NULL) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- SSL_SESSION_free(ss);
- return 0;
+ if (ssl->tlsext_hostname != NULL) {
+ session->tlsext_hostname = BUF_strdup(ssl->tlsext_hostname);
+ if (session->tlsext_hostname == NULL) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
+ goto err;
}
}
} else {
- ss->session_id_length = 0;
+ session->session_id_length = 0;
}
- if (s->sid_ctx_length > sizeof(ss->sid_ctx)) {
+ if (ssl->sid_ctx_length > sizeof(session->sid_ctx)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- SSL_SESSION_free(ss);
- return 0;
+ goto err;
}
+ memcpy(session->sid_ctx, ssl->sid_ctx, ssl->sid_ctx_length);
+ session->sid_ctx_length = ssl->sid_ctx_length;
- memcpy(ss->sid_ctx, s->sid_ctx, s->sid_ctx_length);
- ss->sid_ctx_length = s->sid_ctx_length;
- s->session = ss;
- ss->ssl_version = s->version;
- ss->verify_result = X509_V_OK;
+ session->verify_result = X509_V_OK;
+ SSL_SESSION_free(ssl->session);
+ ssl->session = session;
return 1;
+
+err:
+ SSL_SESSION_free(session);
+ return 0;
}
/* ssl_lookup_session looks up |session_id| in the session cache and sets
