Commit 31c521b0 authored by Ivan Avdeev's avatar Ivan Avdeev Committed by Daniel Stenberg
Browse files

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
parent 6cabd785

lib/urldata.h

+0 −1
Original line number Diff line number Diff line
@@ -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

+7 −1
Original line number Diff line number Diff line
@@ -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

+6 −0
Original line number Diff line number Diff line
@@ -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

+4 −0
Original line number Diff line number Diff line
@@ -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

+4 −0
Original line number Diff line number Diff line
@@ -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;