Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use-after-free in Curl_ssl_addsessionid() #10273

Closed
stefantalpalaru opened this issue Jan 10, 2023 · 18 comments
Closed

use-after-free in Curl_ssl_addsessionid() #10273

stefantalpalaru opened this issue Jan 10, 2023 · 18 comments
Assignees

Comments

@stefantalpalaru
Copy link

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
Copy link
Member

bagder commented Jan 11, 2023

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
Copy link
Author

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
Copy link
Author

stefantalpalaru commented Jan 11, 2023

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
Copy link
Member

bagder commented Jan 12, 2023

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

@stefantalpalaru
Copy link
Author

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
Copy link
Member

bagder commented Jan 13, 2023

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
Copy link
Author

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
Copy link
Member

bagder commented Jan 13, 2023

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

@stefantalpalaru
Copy link
Author

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

@stefantalpalaru
Copy link
Author

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
Copy link
Member

bagder commented Jan 13, 2023

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
Copy link
Author

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
Copy link
Member

bagder commented Jan 14, 2023

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
Copy link
Author

Here's a failure output without that patch

@bagder
Copy link
Member

bagder commented Jan 14, 2023

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

@bagder
Copy link
Member

bagder commented Jan 14, 2023

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

@stefantalpalaru
Copy link
Author

icing added a commit to icing/curl that referenced this issue Jan 17, 2023
…ng the `connectdata` instance

since this may get free'ed on connection reuse. Refs curl#10309, curl#10273.
@icing
Copy link
Contributor

icing commented Jan 17, 2023

Please see #10310 as a fix for this.

@icing icing self-assigned this Jan 17, 2023
@jay jay closed this as completed in f8da4f2 Jan 20, 2023
bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
- Copy the hostname and dispname to ssl_connect_data.

Use a copy instead of referencing the `connectdata` instance since this
may get free'ed on connection reuse.

Reported-by: Stefan Talpalaru
Reported-by: sergio-nsk@users.noreply.github.com

Fixes curl#10273
Fixes curl#10309

Closes curl#10310
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants