From 83e3671d87f31e855e24fc29089fcfa6ba3a8f91 Mon Sep 17 00:00:00 2001
From: Hanno Becker <hanno.becker@arm.com>
Date: Thu, 15 Apr 2021 08:19:40 +0100
Subject: [PATCH] Don't check ciphersuite and compression in SSL session cache
 lookup

Session-ID based session resumption requires that the resumed session
is consistent with the client's ClientHello in terms of choice of
ciphersuite and choice of compression.

This check was previously assumed to be performed in the session cache
implementation, which seems wrong: The session cache should be an id-based
lookup only, and protocol specific checks should be left to Mbed TLS.

This commit
- adds an explicit ciphersuite and compression consistency check after
  the SSL session cache has been queried
- removes the ciphersuite and compression consistency check from
  Mbed TLS' session cache reference implementation.

Don't use ssl_check_xxx() for functions with void return

Signed-off-by: Hanno Becker <hanno.becker@arm.com>
---
 library/ssl_cache.c | 10 +++----
 library/ssl_srv.c   | 66 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/library/ssl_cache.c b/library/ssl_cache.c
index 7e9d4da05..32188cf3f 100644
--- a/library/ssl_cache.c
+++ b/library/ssl_cache.c
@@ -78,14 +78,12 @@ int mbedtls_ssl_cache_get( void *data, mbedtls_ssl_session *session )
             continue;
 #endif
 
-        if( session->ciphersuite != entry->session.ciphersuite ||
-            session->compression != entry->session.compression ||
-            session->id_len != entry->session.id_len )
-            continue;
-
-        if( memcmp( session->id, entry->session.id,
+        if( session->id_len != entry->session.id_len ||
+            memcmp( session->id, entry->session.id,
                     entry->session.id_len ) != 0 )
+        {
             continue;
+        }
 
         ret = mbedtls_ssl_session_copy( session, &entry->session );
         if( ret != 0 )
diff --git a/library/ssl_srv.c b/library/ssl_srv.c
index 581b3f7bd..aca871eca 100644
--- a/library/ssl_srv.c
+++ b/library/ssl_srv.c
@@ -2765,6 +2765,55 @@ static int ssl_write_hello_verify_request( mbedtls_ssl_context *ssl )
 }
 #endif /* MBEDTLS_SSL_DTLS_HELLO_VERIFY */
 
+static void ssl_handle_id_based_session_resumption( mbedtls_ssl_context *ssl )
+{
+    int ret;
+    mbedtls_ssl_session session_tmp;
+    mbedtls_ssl_session * const session = ssl->session_negotiate;
+
+    /* Resume is 0  by default, see ssl_handshake_init().
+     * It may be already set to 1 by ssl_parse_session_ticket_ext(). */
+    if( ssl->handshake->resume == 1 )
+        return;
+    if( session->id_len == 0 )
+        return;
+    if( ssl->conf->f_get_cache == NULL )
+        return;
+#if defined(MBEDTLS_SSL_RENEGOTIATION)
+    if( ssl->renego_status != MBEDTLS_SSL_INITIAL_HANDSHAKE )
+        return;
+#endif
+
+    mbedtls_ssl_session_init( &session_tmp );
+
+    session_tmp.id_len = session->id_len;
+    memcpy( session_tmp.id, session->id, session->id_len );
+
+    ret = ssl->conf->f_get_cache( ssl->conf->p_cache,
+                                  &session_tmp );
+    if( ret != 0 )
+        goto exit;
+
+    if( session->ciphersuite != session_tmp.ciphersuite ||
+        session->compression != session_tmp.compression )
+    {
+        /* Mismatch between cached and negotiated session */
+        goto exit;
+    }
+
+    /* Move semantics */
+    mbedtls_ssl_session_free( session );
+    *session = session_tmp;
+    memset( &session_tmp, 0, sizeof( session_tmp ) );
+
+    MBEDTLS_SSL_DEBUG_MSG( 3, ( "session successfully restored from cache" ) );
+    ssl->handshake->resume = 1;
+
+exit:
+
+    mbedtls_ssl_session_free( &session_tmp );
+}
+
 static int ssl_write_server_hello( mbedtls_ssl_context *ssl )
 {
 #if defined(MBEDTLS_HAVE_TIME)
@@ -2835,22 +2884,7 @@ static int ssl_write_server_hello( mbedtls_ssl_context *ssl )
 
     MBEDTLS_SSL_DEBUG_BUF( 3, "server hello, random bytes", buf + 6, 32 );
 
-    /*
-     * Resume is 0  by default, see ssl_handshake_init().
-     * It may be already set to 1 by ssl_parse_session_ticket_ext().
-     * If not, try looking up session ID in our cache.
-     */
-    if( ssl->handshake->resume == 0 &&
-#if defined(MBEDTLS_SSL_RENEGOTIATION)
-        ssl->renego_status == MBEDTLS_SSL_INITIAL_HANDSHAKE &&
-#endif
-        ssl->session_negotiate->id_len != 0 &&
-        ssl->conf->f_get_cache != NULL &&
-        ssl->conf->f_get_cache( ssl->conf->p_cache, ssl->session_negotiate ) == 0 )
-    {
-        MBEDTLS_SSL_DEBUG_MSG( 3, ( "session successfully restored from cache" ) );
-        ssl->handshake->resume = 1;
-    }
+    ssl_handle_id_based_session_resumption( ssl );
 
     if( ssl->handshake->resume == 0 )
     {