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, 26 insertions, 29 deletions

diff --git a/lib/vtls/schannel.c b/lib/vtls/schannel.c
index 3db5c362c..38a2aa33e 100644
--- a/lib/vtls/schannel.c
+++ b/lib/vtls/schannel.c
@@ -123,11 +123,21 @@ schannel_connect_step1(struct connectdata *conn, int sockindex)
         conn->host.name, conn->remote_port);
 
   /* check for an existing re-usable credential handle */
+  Curl_ssl_sessionid_lock(conn);
   if(!Curl_ssl_getsessionid(conn, (void **)&old_cred, NULL)) {
     connssl->cred = old_cred;
     infof(data, "schannel: re-using existing credential handle\n");
+
+    /* increment the reference counter of the credential/session handle */
+    connssl->cred->refcount++;
+    infof(data, "schannel: incremented credential handle refcount = %d\n",
+          connssl->cred->refcount);
+
+    Curl_ssl_sessionid_unlock(conn);
   }
   else {
+    Curl_ssl_sessionid_unlock(conn);
+
     /* setup Schannel API options */
     memset(&schannel_cred, 0, sizeof(schannel_cred));
     schannel_cred.dwVersion = SCHANNEL_CRED_VERSION;
@@ -200,6 +210,7 @@ schannel_connect_step1(struct connectdata *conn, int sockindex)
       return CURLE_OUT_OF_MEMORY;
     }
     memset(connssl->cred, 0, sizeof(struct curl_schannel_cred));
+    connssl->cred->refcount = 1;
 
     /* https://msdn.microsoft.com/en-us/library/windows/desktop/aa374716.aspx
        */
@@ -666,18 +677,13 @@ schannel_connect_step3(struct connectdata *conn, int sockindex)
   }
 #endif
 
-  /* increment the reference counter of the credential/session handle */
-  if(connssl->cred && connssl->ctxt) {
-    connssl->cred->refcount++;
-    infof(data, "schannel: incremented credential handle refcount = %d\n",
-          connssl->cred->refcount);
-  }
-
   /* save the current session data for possible re-use */
+  Curl_ssl_sessionid_lock(conn);
   incache = !(Curl_ssl_getsessionid(conn, (void **)&old_cred, NULL));
   if(incache) {
     if(old_cred != connssl->cred) {
       infof(data, "schannel: old credential handle is stale, removing\n");
+      /* we're not taking old_cred ownership here, no refcount++ is needed */
       Curl_ssl_delsessionid(conn, (void *)old_cred);
       incache = FALSE;
     }
@@ -687,14 +693,17 @@ schannel_connect_step3(struct connectdata *conn, int sockindex)
     result = Curl_ssl_addsessionid(conn, (void *)connssl->cred,
                                    sizeof(struct curl_schannel_cred));
     if(result) {
+      Curl_ssl_sessionid_unlock(conn);
       failf(data, "schannel: failed to store credential handle");
       return result;
     }
     else {
-      connssl->cred->cached = TRUE;
+      /* this cred session is now also referenced by sessionid cache */
+      connssl->cred->refcount++;
       infof(data, "schannel: stored credential handle in session cache\n");
     }
   }
+  Curl_ssl_sessionid_unlock(conn);
 
   if(data->set.ssl.certinfo) {
     sspi_status = s_pSecFn->QueryContextAttributes(&connssl->ctxt->ctxt_handle,
@@ -1442,19 +1451,10 @@ int Curl_schannel_shutdown(struct connectdata *conn, int sockindex)
 
   /* free SSPI Schannel API credential handle */
   if(connssl->cred) {
-    /* decrement the reference counter of the credential/session handle */
-    if(connssl->cred->refcount > 0) {
-      connssl->cred->refcount--;
-      infof(data, "schannel: decremented credential handle refcount = %d\n",
-            connssl->cred->refcount);
-    }
-
-    /* if the handle was not cached and the refcount is zero */
-    if(!connssl->cred->cached && connssl->cred->refcount == 0) {
-      infof(data, "schannel: clear credential handle\n");
-      s_pSecFn->FreeCredentialsHandle(&connssl->cred->cred_handle);
-      Curl_safefree(connssl->cred);
-    }
+    Curl_ssl_sessionid_lock(conn);
+    Curl_schannel_session_free(connssl->cred);
+    Curl_ssl_sessionid_unlock(conn);
+    connssl->cred = NULL;
   }
 
   /* free internal buffer for received encrypted data */
@@ -1476,16 +1476,13 @@ int Curl_schannel_shutdown(struct connectdata *conn, int sockindex)
 
 void Curl_schannel_session_free(void *ptr)
 {
+  /* this is expected to be called under sessionid lock */
   struct curl_schannel_cred *cred = ptr;
 
-  if(cred && cred->cached) {
-    if(cred->refcount == 0) {
-      s_pSecFn->FreeCredentialsHandle(&cred->cred_handle);
-      Curl_safefree(cred);
-    }
-    else {
-      cred->cached = FALSE;
-    }
+  cred->refcount--;
+  if(cred->refcount == 0) {
+    s_pSecFn->FreeCredentialsHandle(&cred->cred_handle);
+    Curl_safefree(cred);
   }
 }