vtls: fix ssl session cache race condition

Sessionid cache management is inseparable from managing individual
session lifetimes. E.g. for reference-counted sessions (like those in
SChannel and OpenSSL engines) every session addition and removal
should be accompanied with refcount increment and decrement
respectively. Failing to do so synchronously leads to a race condition
that causes symptoms like use-after-free and memory corruption.
This commit:
 - makes existing session cache locking explicit, thus allowing
   individual engines to manage lock's scope.
 - fixes OpenSSL and SChannel engines by putting refcount management
   inside this lock's scope in relevant places.
 - adds these explicit locking calls to other engines that use
   sessionid cache to accommodate for this change. Note, however,
   that it is unknown whether any of these engines could also have
   this race.

Bug: https://github.com/curl/curl/issues/815
Fixes #815
Closes #847

1 files changed, 6 insertions, 0 deletions

diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index 776ebe5de..f702653cd 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -2082,9 +2082,11 @@ static CURLcode ossl_connect_step1(struct connectdata *conn, int sockindex)
 #endif
 
   /* Check if there's a cached ID we can/should use here! */
+  Curl_ssl_sessionid_lock(conn);
   if(!Curl_ssl_getsessionid(conn, &ssl_sessionid, NULL)) {
     /* we got a session id, use it! */
     if(!SSL_set_session(connssl->handle, ssl_sessionid)) {
+      Curl_ssl_sessionid_unlock(conn);
       failf(data, "SSL: SSL_set_session failed: %s",
             ERR_error_string(ERR_get_error(), NULL));
       return CURLE_SSL_CONNECT_ERROR;
@@ -2092,6 +2094,7 @@ static CURLcode ossl_connect_step1(struct connectdata *conn, int sockindex)
     /* Informational message */
     infof (data, "SSL re-using session ID\n");
   }
+  Curl_ssl_sessionid_unlock(conn);
 
   /* pass the raw socket into the SSL layers */
   if(!SSL_set_fd(connssl->handle, (int)sockfd)) {
@@ -2818,6 +2821,7 @@ static CURLcode ossl_connect_step3(struct connectdata *conn, int sockindex)
      will stay in memory until explicitly freed with SSL_SESSION_free(3),
      regardless of its state. */
 
+  Curl_ssl_sessionid_lock(conn);
   incache = !(Curl_ssl_getsessionid(conn, &old_ssl_sessionid, NULL));
   if(incache) {
     if(old_ssl_sessionid != our_ssl_sessionid) {
@@ -2831,6 +2835,7 @@ static CURLcode ossl_connect_step3(struct connectdata *conn, int sockindex)
     result = Curl_ssl_addsessionid(conn, our_ssl_sessionid,
                                    0 /* unknown size */);
     if(result) {
+      Curl_ssl_sessionid_unlock(conn);
       failf(data, "failed to store ssl session");
       return result;
     }
@@ -2842,6 +2847,7 @@ static CURLcode ossl_connect_step3(struct connectdata *conn, int sockindex)
      */
     SSL_SESSION_free(our_ssl_sessionid);
   }
+  Curl_ssl_sessionid_unlock(conn);
 
   /*
    * We check certificates to authenticate the server; otherwise we risk