From c0a58e034d3eff68ca5e0d36d7b4d147425b0599 Mon Sep 17 00:00:00 2001 From: Marcus Huewe Date: Fri, 11 May 2018 12:24:56 +0200 Subject: [PATCH] Do not free a session before calling the remove_session_cb If the remove_session_cb accesses the session's data (for instance, via SSL_SESSION_get_protocol_version), a potential use after free can occur. For this, consider the following scenario when adding a new session via SSL_CTX_add_session: - The session cache is full (SSL_CTX_sess_number(ctx) > SSL_CTX_sess_get_cache_size(ctx)) - Only the session cache has a reference to ctx->session_cache_tail (that is, ctx->session_cache_tail->references == 1) Since the cache is full, remove_session_lock is called to remove ctx->session_cache_tail from the cache. That is, it SSL_SESSION_free()s the session, which free()s the data. Afterwards, the free()d session is passed to the remove_session_cb. If the callback accesses the session's data, we have a use after free. The free before calling the callback behavior was introduced in commit e4612d02c53cccd24fa97b08fc01250d1238cca1 ("Remove sessions from external cache, even if internal cache not used."). CLA: trivial Reviewed-by: Rich Salz Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/6222) --- ssl/ssl_sess.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index 525edb3289..0723765366 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -769,11 +769,11 @@ static int remove_session_lock(SSL_CTX *ctx, SSL_SESSION *c, int lck) if (lck) CRYPTO_THREAD_unlock(ctx->lock); - if (ret) - SSL_SESSION_free(r); - if (ctx->remove_session_cb != NULL) ctx->remove_session_cb(ctx, c); + + if (ret) + SSL_SESSION_free(r); } else ret = 0; return ret; -- GitLab