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

lib/urldata.h

@@ -242,7 +242,6 @@ struct curl_schannel_cred {
   CredHandle cred_handle;
   TimeStamp time_stamp;
   int refcount;
-  bool cached;
 };
 
 struct curl_schannel_ctxt {

lib/vtls/axtls.c

@@ -259,14 +259,18 @@ static CURLcode connect_prep(struct connectdata *conn, int sockindex)
    */
 
   /* In axTLS, handshaking happens inside ssl_client_new. */
+  Curl_ssl_sessionid_lock(conn);
   if(!Curl_ssl_getsessionid(conn, (void **) &ssl_sessionid, &ssl_idsize)) {
     /* we got a session id, use it! */
     infof (data, "SSL re-using session ID\n");
     ssl = ssl_client_new(ssl_ctx, conn->sock[sockindex],
                          ssl_sessionid, (uint8_t)ssl_idsize);
+    Curl_ssl_sessionid_unlock();
   }
-  else
+  else {
+    Curl_ssl_sessionid_unlock();
     ssl = ssl_client_new(ssl_ctx, conn->sock[sockindex], NULL, 0);
+  }
 
   conn->ssl[sockindex].ssl = ssl;
   return CURLE_OK;
@@ -381,9 +385,11 @@ static CURLcode connect_finish(struct connectdata *conn, int sockindex)
   /* Put our freshly minted SSL session in cache */
   ssl_idsize = ssl_get_session_id_size(ssl);
   ssl_sessionid = ssl_get_session_id(ssl);
+  Curl_ssl_sessionid_lock(conn);
   if(Curl_ssl_addsessionid(conn, (void *) ssl_sessionid, ssl_idsize)
      != CURLE_OK)
     infof (data, "failed to add session to cache\n");
+  Curl_ssl_sessionid_unlock(conn);
 
   return CURLE_OK;
 }

lib/vtls/cyassl.c

@@ -378,9 +378,11 @@ cyassl_connect_step1(struct connectdata *conn,
 #endif /* HAVE_ALPN */
 
   /* 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(conssl->handle, ssl_sessionid)) {
+      Curl_ssl_sessionid_unlock(conn);
       failf(data, "SSL: SSL_set_session failed: %s",
             ERR_error_string(SSL_get_error(conssl->handle, 0), error_buffer));
       return CURLE_SSL_CONNECT_ERROR;
@@ -388,6 +390,7 @@ cyassl_connect_step1(struct connectdata *conn,
     /* Informational message */
     infof (data, "SSL re-using session ID\n");
   }
+  Curl_ssl_sessionid_unlock(conn);
 
   /* pass the raw socket into the SSL layer */
   if(!SSL_set_fd(conssl->handle, (int)sockfd)) {
@@ -581,6 +584,7 @@ cyassl_connect_step3(struct connectdata *conn,
 
   our_ssl_sessionid = SSL_get_session(connssl->handle);
 
+  Curl_ssl_sessionid_lock(conn);
   incache = !(Curl_ssl_getsessionid(conn, &old_ssl_sessionid, NULL));
   if(incache) {
     if(old_ssl_sessionid != our_ssl_sessionid) {
@@ -594,10 +598,12 @@ cyassl_connect_step3(struct connectdata *conn,
     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;
     }
   }
+  Curl_ssl_sessionid_unlock(conn);
 
   connssl->connecting_state = ssl_connect_done;
 

lib/vtls/darwinssl.c

@@ -1474,10 +1474,12 @@ static CURLcode darwinssl_connect_step1(struct connectdata *conn,
 #endif /* CURL_BUILD_MAC_10_9 || CURL_BUILD_IOS_7 */
 
   /* Check if there's a cached ID we can/should use here! */
+  Curl_ssl_sessionid_lock(conn);
   if(!Curl_ssl_getsessionid(conn, (void **)&ssl_sessionid,
                             &ssl_sessionid_len)) {
     /* we got a session id, use it! */
     err = SSLSetPeerID(connssl->ssl_ctx, ssl_sessionid, ssl_sessionid_len);
+    Curl_ssl_sessionid_unlock(conn);
     if(err != noErr) {
       failf(data, "SSL: SSLSetPeerID() failed: OSStatus %d", err);
       return CURLE_SSL_CONNECT_ERROR;
@@ -1497,11 +1499,13 @@ static CURLcode darwinssl_connect_step1(struct connectdata *conn,
 
     err = SSLSetPeerID(connssl->ssl_ctx, ssl_sessionid, ssl_sessionid_len);
     if(err != noErr) {
+      Curl_ssl_sessionid_unlock(conn);
       failf(data, "SSL: SSLSetPeerID() failed: OSStatus %d", err);
       return CURLE_SSL_CONNECT_ERROR;
     }
 
     result = Curl_ssl_addsessionid(conn, ssl_sessionid, ssl_sessionid_len);
+    Curl_ssl_sessionid_unlock(conn);
     if(result) {
       failf(data, "failed to store ssl session");
       return result;

lib/vtls/gtls.c

@@ -750,6 +750,7 @@ gtls_connect_step1(struct connectdata *conn,
   /* This might be a reconnect, so we check for a session ID in the cache
      to speed up things */
 
+  Curl_ssl_sessionid_lock(conn);
   if(!Curl_ssl_getsessionid(conn, &ssl_sessionid, &ssl_idsize)) {
     /* we got a session id, use it! */
     gnutls_session_set_data(session, ssl_sessionid, ssl_idsize);
@@ -757,6 +758,7 @@ gtls_connect_step1(struct connectdata *conn,
     /* Informational message */
     infof (data, "SSL re-using session ID\n");
   }
+  Curl_ssl_sessionid_unlock(conn);
 
   return CURLE_OK;
 }
@@ -1284,6 +1286,7 @@ gtls_connect_step3(struct connectdata *conn,
       /* extract session ID to the allocated buffer */
       gnutls_session_get_data(session, connect_sessionid, &connect_idsize);
 
+      Curl_ssl_sessionid_lock(conn);
       incache = !(Curl_ssl_getsessionid(conn, &ssl_sessionid, NULL));
       if(incache) {
         /* there was one before in the cache, so instead of risking that the
@@ -1293,6 +1296,7 @@ gtls_connect_step3(struct connectdata *conn,
 
       /* store this session id */
       result = Curl_ssl_addsessionid(conn, connect_sessionid, connect_idsize);
+      Curl_ssl_sessionid_unlock(conn);
       if(result) {
         free(connect_sessionid);
         result = CURLE_OUT_OF_MEMORY;

lib/vtls/mbedtls.c

@@ -365,14 +365,17 @@ mbed_connect_step1(struct connectdata *conn,
 
   mbedtls_ssl_conf_ciphersuites(&connssl->config,
                                 mbedtls_ssl_list_ciphersuites());
+  Curl_ssl_sessionid_lock(conn);
   if(!Curl_ssl_getsessionid(conn, &old_session, NULL)) {
     ret = mbedtls_ssl_set_session(&connssl->ssl, old_session);
     if(ret) {
+      Curl_ssl_sessionid_unlock(conn);
       failf(data, "mbedtls_ssl_set_session returned -0x%x", -ret);
       return CURLE_SSL_CONNECT_ERROR;
     }
     infof(data, "mbedTLS re-using session\n");
   }
+  Curl_ssl_sessionid_unlock(conn);
 
   mbedtls_ssl_conf_ca_chain(&connssl->config,
                             &connssl->cacert,
@@ -607,10 +610,12 @@ mbed_connect_step3(struct connectdata *conn,
   }
 
   /* If there's already a matching session in the cache, delete it */
+  Curl_ssl_sessionid_lock(conn);
   if(!Curl_ssl_getsessionid(conn, &old_ssl_sessionid, NULL))
     Curl_ssl_delsessionid(conn, old_ssl_sessionid);
 
   retcode = Curl_ssl_addsessionid(conn, our_ssl_sessionid, 0);
+  Curl_ssl_sessionid_unlock(conn);
   if(retcode) {
     free(our_ssl_sessionid);
     failf(data, "failed to store ssl session");

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

lib/vtls/polarssl.c

@@ -337,8 +337,10 @@ polarssl_connect_step1(struct connectdata *conn,
               net_send, &conn->sock[sockindex]);
 
   ssl_set_ciphersuites(&connssl->ssl, ssl_list_ciphersuites());
+  Curl_ssl_sessionid_lock(conn);
   if(!Curl_ssl_getsessionid(conn, &old_session, NULL)) {
     ret = ssl_set_session(&connssl->ssl, old_session);
+    Curl_ssl_sessionid_unlock(conn);
     if(ret) {
       failf(data, "ssl_set_session returned -0x%x", -ret);
       return CURLE_SSL_CONNECT_ERROR;
@@ -572,10 +574,12 @@ polarssl_connect_step3(struct connectdata *conn,
   }
 
   /* If there's already a matching session in the cache, delete it */
+  Curl_ssl_sessionid_lock(conn);
   if(!Curl_ssl_getsessionid(conn, &old_ssl_sessionid, NULL))
     Curl_ssl_delsessionid(conn, old_ssl_sessionid);
 
   retcode = Curl_ssl_addsessionid(conn, our_ssl_sessionid, 0);
+  Curl_ssl_sessionid_unlock(conn);
   if(retcode) {
     free(our_ssl_sessionid);
     failf(data, "failed to store ssl session");

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 */
+    Curl_ssl_sessionid_lock(conn);
-    if(connssl->cred->refcount > 0) {
+    Curl_schannel_session_free(connssl->cred);
-      connssl->cred->refcount--;
+    Curl_ssl_sessionid_unlock(conn);
-      infof(data, "schannel: decremented credential handle refcount = %d\n",
+    connssl->cred = NULL;
-            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);
-    }
   }
 
   /* 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) {
+  cred->refcount--;
-    if(cred->refcount == 0) {
+  if(cred->refcount == 0) {
-      s_pSecFn->FreeCredentialsHandle(&cred->cred_handle);
+    s_pSecFn->FreeCredentialsHandle(&cred->cred_handle);
-      Curl_safefree(cred);
+    Curl_safefree(cred);
-    }
-    else {
-      cred->cached = FALSE;
-    }
   }
 }
 

lib/vtls/vtls.c

@@ -329,6 +329,25 @@ Curl_ssl_connect_nonblocking(struct connectdata *conn, int sockindex,
   return result;
 }
 
+/*
+ * Lock shared SSL session data
+ */
+void Curl_ssl_sessionid_lock(struct connectdata *conn)
+{
+  if(SSLSESSION_SHARED(conn->data))
+    Curl_share_lock(conn->data,
+                    CURL_LOCK_DATA_SSL_SESSION, CURL_LOCK_ACCESS_SINGLE);
+}
+
+/*
+ * Unlock shared SSL session data
+ */
+void Curl_ssl_sessionid_unlock(struct connectdata *conn)
+{
+  if(SSLSESSION_SHARED(conn->data))
+    Curl_share_unlock(conn->data, CURL_LOCK_DATA_SSL_SESSION);
+}
+
 /*
  * Check if there's a session ID for the given connection in the cache, and if
  * there's one suitable, it is provided. Returns TRUE when no entry matched.
@@ -350,10 +369,8 @@ bool Curl_ssl_getsessionid(struct connectdata *conn,
     return TRUE;
 
   /* Lock if shared */
-  if(SSLSESSION_SHARED(data)) {
+  if(SSLSESSION_SHARED(data))
-    Curl_share_lock(data, CURL_LOCK_DATA_SSL_SESSION, CURL_LOCK_ACCESS_SINGLE);
     general_age = &data->share->sessionage;
-  }
   else
     general_age = &data->state.sessionage;
 
@@ -382,10 +399,6 @@ bool Curl_ssl_getsessionid(struct connectdata *conn,
     }
   }
 
-  /* Unlock */
-  if(SSLSESSION_SHARED(data))
-    Curl_share_unlock(data, CURL_LOCK_DATA_SSL_SESSION);
-
   return no_match;
 }
 
@@ -418,9 +431,6 @@ void Curl_ssl_delsessionid(struct connectdata *conn, void *ssl_sessionid)
   size_t i;
   struct SessionHandle *data=conn->data;
 
-  if(SSLSESSION_SHARED(data))
-    Curl_share_lock(data, CURL_LOCK_DATA_SSL_SESSION, CURL_LOCK_ACCESS_SINGLE);
-
   for(i = 0; i < data->set.ssl.max_ssl_sessions; i++) {
     struct curl_ssl_session *check = &data->state.session[i];
 
@@ -429,9 +439,6 @@ void Curl_ssl_delsessionid(struct connectdata *conn, void *ssl_sessionid)
       break;
     }
   }
-
-  if(SSLSESSION_SHARED(data))
-    Curl_share_unlock(data, CURL_LOCK_DATA_SSL_SESSION);
 }
 
 /*
@@ -481,7 +488,6 @@ CURLcode Curl_ssl_addsessionid(struct connectdata *conn,
 
   /* If using shared SSL session, lock! */
   if(SSLSESSION_SHARED(data)) {
-    Curl_share_lock(data, CURL_LOCK_DATA_SSL_SESSION, CURL_LOCK_ACCESS_SINGLE);
     general_age = &data->share->sessionage;
   }
   else {
@@ -514,10 +520,6 @@ CURLcode Curl_ssl_addsessionid(struct connectdata *conn,
   store->conn_to_port = conn_to_port; /* connect to port number */
   store->remote_port = conn->remote_port; /* port number */
 
-  /* Unlock */
-  if(SSLSESSION_SHARED(data))
-    Curl_share_unlock(data, CURL_LOCK_DATA_SSL_SESSION);
-
   if(!Curl_clone_ssl_config(&conn->ssl_config, &store->ssl_config)) {
     store->sessionid = NULL; /* let caller free sessionid */
     free(clone_host);

lib/vtls/vtls.h

@@ -96,17 +96,48 @@ CURLcode Curl_ssl_push_certinfo(struct SessionHandle * data, int certnum,
 
 /* Functions to be used by SSL library adaptation functions */
 
-/* extract a session ID */
+/* Lock session cache mutex.
+ * Call this before calling other Curl_ssl_*session* functions
+ * Caller should unlock this mutex as soon as possible, as it may block
+ * other SSL connection from making progress.
+ * The purpose of explicitly locking SSL session cache data is to allow
+ * individual SSL engines to manage session lifetime in their specific way.
+ */
+void Curl_ssl_sessionid_lock(struct connectdata *conn);
+
+/* Unlock session cache mutex */
+void Curl_ssl_sessionid_unlock(struct connectdata *conn);
+
+/* extract a session ID
+ * Sessionid mutex must be locked (see Curl_ssl_sessionid_lock).
+ * Caller must make sure that the ownership of returned sessionid object
+ * is properly taken (e.g. its refcount is incremented
+ * under sessionid mutex).
+ */
 bool Curl_ssl_getsessionid(struct connectdata *conn,
                            void **ssl_sessionid,
                            size_t *idsize); /* set 0 if unknown */
-/* add a new session ID */
+/* add a new session ID
+ * Sessionid mutex must be locked (see Curl_ssl_sessionid_lock).
+ * Caller must ensure that it has properly shared ownership of this sessionid
+ * object with cache (e.g. incrementing refcount on success)
+ */
 CURLcode Curl_ssl_addsessionid(struct connectdata *conn,
                                void *ssl_sessionid,
                                size_t idsize);
-/* Kill a single session ID entry in the cache */
+/* Kill a single session ID entry in the cache
+ * Sessionid mutex must be locked (see Curl_ssl_sessionid_lock).
+ * This will call engine-specific curlssl_session_free function, which must
+ * take sessionid object ownership from sessionid cache
+ * (e.g. decrement refcount).
+ */
 void Curl_ssl_kill_session(struct curl_ssl_session *session);
-/* delete a session from the cache */
+/* delete a session from the cache
+ * Sessionid mutex must be locked (see Curl_ssl_sessionid_lock).
+ * This will call engine-specific curlssl_session_free function, which must
+ * take sessionid object ownership from sessionid cache
+ * (e.g. decrement refcount).
+ */
 void Curl_ssl_delsessionid(struct connectdata *conn, void *ssl_sessionid);
 
 /* get N random bytes into the buffer, return 0 if a find random is filled