Code to thread-safety in ChangeCipherState

The server-side ChangeCipherState processing stores the new cipher
in the SSL_SESSION object, so that the new state can be used if
this session gets resumed.  However, writing to the session is only
thread-safe for initial handshakes, as at other times the session
object may be in a shared cache and in use by another thread at the
same time.  Reflect this invariant in the code by only writing to
s->session->cipher when it is currently NULL (we do not cache sessions
with no cipher).  The code prior to this change would never actually
change the (non-NULL) cipher value in a session object, since our
server enforces that (pre-TLS-1.3) resumptions use the exact same
cipher as the initial connection, and non-abbreviated renegotiations
have produced a new session object before we get to this point.
Regardless, include logic to detect such a condition and abort the
handshake if it occurs, to avoid any risk of inadvertently using
the wrong cipher on a connection.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/10943)

(cherry picked from commit 2e3ec2e1578977fca830a47fd7f521e290540e6d)

diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt
index 10444a17f90c6e6985531e5873d7ad6c688325f9..f5324c6819d8e25bb231160b8f6f14638db234d6 100644 (file)
--- a/crypto/err/openssl.txt
+++ b/crypto/err/openssl.txt
@@ -1180,6 +1180,7 @@ SSL_F_OSSL_STATEM_SERVER_CONSTRUCT_MESSAGE:431:*
 SSL_F_OSSL_STATEM_SERVER_POST_PROCESS_MESSAGE:601:\
        ossl_statem_server_post_process_message
 SSL_F_OSSL_STATEM_SERVER_POST_WORK:602:ossl_statem_server_post_work
+SSL_F_OSSL_STATEM_SERVER_PRE_WORK:640:
 SSL_F_OSSL_STATEM_SERVER_PROCESS_MESSAGE:603:ossl_statem_server_process_message
 SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION:418:ossl_statem_server_read_transition
 SSL_F_OSSL_STATEM_SERVER_WRITE_TRANSITION:604:\

diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h
index b6cac4f5f55b18b7309a9f6b9864f8990d0bebb7..0ef684f3c131925b6a6ff27e64e45888a5ac418f 100644 (file)
--- a/include/openssl/sslerr.h
+++ b/include/openssl/sslerr.h
@@ -88,6 +88,7 @@ int ERR_load_SSL_strings(void);
 # define SSL_F_OSSL_STATEM_SERVER_CONSTRUCT_MESSAGE       431
 # define SSL_F_OSSL_STATEM_SERVER_POST_PROCESS_MESSAGE    601
 # define SSL_F_OSSL_STATEM_SERVER_POST_WORK               602
+# define SSL_F_OSSL_STATEM_SERVER_PRE_WORK                640
 # define SSL_F_OSSL_STATEM_SERVER_PROCESS_MESSAGE         603
 # define SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION         418
 # define SSL_F_OSSL_STATEM_SERVER_WRITE_TRANSITION        604

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index e055cc2f3ab338468bd365b89826f39baddc9f98..75619d9bca8a32cedfabbc07e1a2f9e4bbcd4e01 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -743,7 +743,15 @@ WORK_STATE ossl_statem_server_pre_work(SSL *s, WORK_STATE wst)
     case TLS_ST_SW_CHANGE:
         if (SSL_IS_TLS13(s))
             break;
-        s->session->cipher = s->s3->tmp.new_cipher;
+        /* Writes to s->session are only safe for initial handshakes */
+        if (s->session->cipher == NULL) {
+            s->session->cipher = s->s3->tmp.new_cipher;
+        } else if (s->session->cipher != s->s3->tmp.new_cipher) {
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR,
+                     SSL_F_OSSL_STATEM_SERVER_PRE_WORK,
+                     ERR_R_INTERNAL_ERROR);
+            return WORK_ERROR;
+        }
         if (!s->method->ssl3_enc->setup_key_block(s)) {
             /* SSLfatal() already called */
             return WORK_ERROR;