use-after-free in Curl_ssl_addsessionid()

[stefantalpalaru]

I did this

I built Transmission with -fsanitize=address and ran into this use-after-free problem inside Curl code: https://gist.github.com/stefantalpalaru/45bbfea53e051078bedabfd090934eeb

The shared handle is used from a single thread, so I don't think it's a case of https://curl.se/docs/knownbugs.html#A_shared_connection_cache_is_not

Transmission is sharing everything it can for that handle: https://github.com/transmission/transmission/blob/0af121004c0ca5321d1625e465640fa379c25d9a/libtransmission/web.cc#L727-L745

curl/libcurl version

curl 7.87.0 (x86_64-pc-linux-gnu) libcurl/7.87.0 (NSS/3.86) OpenSSL/1.1.1s zlib/1.2.13 zstd/1.5.2 c-ares/1.18.1 nghttp2/1.51.0
Release-Date: 2022-12-21
Protocols: dict file ftp ftps http https imap imaps mqtt pop3 pop3s rtsp smtp smtps tftp
Features: AsynchDNS HTTP2 HTTPS-proxy IPv6 Largefile libz MultiSSL NTLM SSL threadsafe TLS-SRP UnixSockets zstd

operating system

Linux amd2 6.0.15-gentoo #1 SMP PREEMPT_DYNAMIC Mon Dec 26 19:39:26 CET 2022 x86_64 AMD Ryzen 9 5950X 16-Core Processor AuthenticAMD GNU/Linux

[bagder]

What more can you tell us about what curl did before this situation happened? If we can't reproduce the problem, this is going to be much harder.

The stack trace implicates the shared session id cache.

[stefantalpalaru]

I tried to disable sharing by commenting out this line: https://github.com/transmission/transmission/blob/0af121004c0ca5321d1625e465640fa379c25d9a/libtransmission/web.cc#L481

Nine and a half hours later, the same use-after-free appeared, so it's not related to CURLOPT_SHARE.
(Maybe curl_share_setopt(_, CURLSHOPT_SHARE, _) overrides a missing curl_easy_setopt(_, CURLOPT_SHARE, _)?)

If we can't reproduce the problem, this is going to be much harder.

I know, but it takes a highly variable amount of time, seeding some random torrent with Transmission to see this. It can be minutes, it can be hours.

[stefantalpalaru]

I still get the failure after commenting out this line: https://github.com/transmission/transmission/blob/0af121004c0ca5321d1625e465640fa379c25d9a/libtransmission/web.cc#L170

It took a little over three hours this time, but it shows that no sharing opt-in is needed to trigger it. Maybe this documentation note is relevant:

"SSL session IDs are reused within the same easy handle by default" - https://curl.se/libcurl/c/CURLSHOPT_SHARE.html

[bagder]

Any chance you can just check of curl built from git master still reproduces this?

[stefantalpalaru]

Sure, but tomorrow. Today I stumbled upon a possible workaround, while trying to figure out what fills Curl_cfilter.ctx - which seems to be a shallow copy of a connectdata instance, for SSL cfilters.

No luck there, but I think this prevents Curl_ssl_addsessionid() from accessing a cf->ctx->hostname that was freed in reuse_conn():

diff -ur curl-7.87.0.orig/lib/vtls/vtls.c curl-7.87.0/lib/vtls/vtls.c
--- curl-7.87.0.orig/lib/vtls/vtls.c	2022-12-19 08:48:23.000000000 +0100
+++ curl-7.87.0/lib/vtls/vtls.c	2023-01-12 15:22:46.206206686 +0100
@@ -130,6 +130,8 @@
 }
 
 
+static void reinit_hostname(struct Curl_cfilter *cf);
+
 bool
 Curl_ssl_config_matches(struct ssl_primary_config *data,
                         struct ssl_primary_config *needle)
@@ -514,6 +516,7 @@
   (void)ssl_config;
   DEBUGASSERT(ssl_config->primary.sessionid);
 
+  reinit_hostname(cf);
   clone_host = strdup(connssl->hostname);
   if(!clone_host)
     return CURLE_OUT_OF_MEMORY; /* bail out */

---

I'll run Transmission for a while with this, before I declare it working, however it is an ugly workaround because it covers a single symptom, not the root cause. That Curl_cfilter.ctx needs to either be a deep copy, or be invalidated inside reuse_conn().

[bagder]

reuse_conn() closes and deletes a connection and it at least looks like that killed connection is what is being pointed to and used by Curl_ssl_addsessionid()! That's some serious mess-up. Doing a deep copy would indeed avoid the crash, but it does not change the fact that curl seems to have two very different views of the same connection in this moment and that is not solved by a deep copy.

[stefantalpalaru]

My workaround works. I have been using Transmission without problems for the last 19 hours.

It looks like reinit_hostname() is the only place where Curl_cfilter.ctx.hostname is set (making it also an "init", not just a "reinit"), but having it be a pointer inside Curl_cfilter.conn becomes a problem when reuse_conn() is called after ssl_cf_connect() - the only function that calls reinit_hostname().

Ideally, we would have reuse_conn() trigger reinit_hostname(), but I don't know how to connect those parts of the code.

[bagder]

I believe this bug might be fixed in master...

[stefantalpalaru]

Curl HEAD failed in a few seconds: https://gist.github.com/stefantalpalaru/47936d53bd07aa459df02da3dbb8e2ca

[stefantalpalaru]

Found a more elegant solution: since connection reusing depends on an existing connection with the same host and port, reuse_conn() does not need to copy those at all.

I noticed that hashkey() for struct connectdata * uses the host and port to create a key for the connection cache and handles everything related to those, except conn->conn_to_port. I added it and clarified the logic a little:

diff --git a/lib/conncache.c b/lib/conncache.c
index c21b96ca8..47cb878cb 100644
--- a/lib/conncache.c
+++ b/lib/conncache.c
@@ -136,20 +136,21 @@ void Curl_conncache_destroy(struct conncache *connc)
 /* creates a key to find a bundle for this connection */
 static void hashkey(struct connectdata *conn, char *buf, size_t len)
 {
-  const char *hostname;
+  const char *hostname = conn->host.name;
   long port = conn->remote_port;
   DEBUGASSERT(len >= HASHKEY_SIZE);
+
+  if(conn->bits.conn_to_host)
+    hostname = conn->conn_to_host.name;
+  if(conn->bits.conn_to_port)
+    port = conn->conn_to_port;
+
 #ifndef CURL_DISABLE_PROXY
   if(conn->bits.httpproxy && !conn->bits.tunnel_proxy) {
     hostname = conn->http_proxy.host.name;
     port = conn->port;
   }
-  else
 #endif
-    if(conn->bits.conn_to_host)
-      hostname = conn->conn_to_host.name;
-  else
-    hostname = conn->host.name;
 
   /* put the numbers first so that the hostname gets cut off if too long */
 #ifdef ENABLE_IPV6
diff --git a/lib/url.c b/lib/url.c
index f90427f9b..62f3ac370 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -3369,22 +3369,6 @@ static void reuse_conn(struct Curl_easy *data,
   }
 #endif
 
-  Curl_free_idnconverted_hostname(&existing->host);
-  Curl_free_idnconverted_hostname(&existing->conn_to_host);
-  Curl_safefree(existing->host.rawalloc);
-  Curl_safefree(existing->conn_to_host.rawalloc);
-  existing->host = temp->host;
-  temp->host.rawalloc = NULL;
-  temp->host.encalloc = NULL;
-  existing->conn_to_host = temp->conn_to_host;
-  temp->conn_to_host.rawalloc = NULL;
-  existing->conn_to_port = temp->conn_to_port;
-  existing->remote_port = temp->remote_port;
-  Curl_safefree(existing->hostname_resolve);
-
-  existing->hostname_resolve = temp->hostname_resolve;
-  temp->hostname_resolve = NULL;
-
   conn_reset_all_postponed_data(temp); /* free buffers */
 
   /* re-use init */

[bagder]

I seems that when the ossl_new_session_cb is called, and it calls SSL_get_ex_data (line 2952) it extracts the wrong data.

Or perhaps, connssl->call_data (line 2954) contains the wrong data.

Your stack trace from the crash indicates that the new session callback comes when SSL_read() is called.

How about a little test to set the ex_data just before SSL_read?

diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index 715515fcf..f10624b17 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -4603,17 +4603,19 @@ static ssize_t ossl_recv(struct Curl_cfilter *cf,
   ssize_t nread;
   int buffsize;
   struct connectdata *conn = cf->conn;
   struct ssl_connect_data *connssl = cf->ctx;
   struct ssl_backend_data *backend = connssl->backend;
+  int cf_idx = ossl_get_ssl_cf_index();
 
   (void)data;
   DEBUGASSERT(backend);
 
   ERR_clear_error();
 
   buffsize = (buffersize > (size_t)INT_MAX) ? INT_MAX : (int)buffersize;
+  SSL_set_ex_data(backend->handle, cf_idx, cf);
   nread = (ssize_t)SSL_read(backend->handle, buf, buffsize);
 
   if(nread <= 0) {
     /* failed SSL_read */
     int err = SSL_get_error(backend->handle, (int)nread);

[stefantalpalaru]

It still fails, with your patch.

Here's a failure output without that patch, but with CURLOPT_VERBOSE enabled: https://gist.github.com/stefantalpalaru/0d52e7472d90647666976aba206b66c3

Connection history for the relevant tracker:

* Hostname 'tracker.jiesen.life' was found in DNS cache
* Connected to tracker.jiesen.life (240e:38f:a803:3812:ee60:7311:105e:7bec) port 8443 (#806)
* Connected to tracker.jiesen.life (240e:38f:a803:3812:ee60:7311:105e:7bec) port 8443 (#326)
Host: tracker.jiesen.life:8443
*  subjectAltName: host "tracker.jiesen.life" matched cert's "*.jiesen.life"
* h2h3 [:authority: tracker.jiesen.life:8443]
Host: tracker.jiesen.life:8443
* Connection #806 to host tracker.jiesen.life left intact
* Re-using existing connection #806 with host tracker.jiesen.life
* h2h3 [:authority: tracker.jiesen.life:8443]
Host: tracker.jiesen.life:8443
* Re-using existing connection #806 with host tracker.jiesen.life
* h2h3 [:authority: tracker.jiesen.life:8443]
Host: tracker.jiesen.life:8443
* Connection #806 to host tracker.jiesen.life left intact
* Re-using existing connection #806 with host tracker.jiesen.life
* h2h3 [:authority: tracker.jiesen.life:8443]
Host: tracker.jiesen.life:8443
* Connection #806 to host tracker.jiesen.life left intact
* Hostname tracker.jiesen.life was found in DNS cache
* Hostname tracker.jiesen.life was found in DNS cache
* Connected to tracker.jiesen.life (240e:38f:a803:3812:ee60:7311:105e:7bec) port 8443 (#1313)

There are 46 instances of "old SSL session ID is stale, removing", which explains why we add a reused SSL connection's session ID to the corresponding cache - the old one was "stale".

[bagder]

Are you sure this is the patched version? It still says SSL_read() on line 4615 while in my version with my patch that invocation is on line 4617...

[stefantalpalaru]

Here's a failure output without that patch

[bagder]

Ah, missed that. So yes, that explains why the stack traces look exactly the same...

[bagder]

So with the patch, did the stack trace look identical as well (with updated line numbers)?

[stefantalpalaru]

Output with your patch: https://gist.github.com/stefantalpalaru/15687f1d91323d7b6cca2fa8600f4d6a

[icing]

Please see #10310 as a fix for this.