From 0c07534eed920f32f072866aa350b1ee7666900b Mon Sep 17 00:00:00 2001 From: Kamil Dudka Date: Mon, 1 Jul 2019 10:19:45 +0200 Subject: [PATCH] prevent multi from crashing with many parallel transfers Resolves: #1697566 Resolves: #1723242 --- 0018-curl-7.64.0-multi-sigsegv.patch | 221 +++++++++++++++++++++++++++ curl.spec | 9 +- 2 files changed, 229 insertions(+), 1 deletion(-) create mode 100644 0018-curl-7.64.0-multi-sigsegv.patch diff --git a/0018-curl-7.64.0-multi-sigsegv.patch b/0018-curl-7.64.0-multi-sigsegv.patch new file mode 100644 index 0000000..c402824 --- /dev/null +++ b/0018-curl-7.64.0-multi-sigsegv.patch @@ -0,0 +1,221 @@ +From 42f31adb0386c168682d14bf1e52fbbdfc5fc36d Mon Sep 17 00:00:00 2001 +From: Even Rouault +Date: Sun, 7 Apr 2019 14:07:35 +0200 +Subject: [PATCH 1/2] multi_runsingle(): fix use-after-free + +Fixes #3745 +Closes #3746 + +The following snippet +``` + +int main() +{ + CURL* hCurlHandle = curl_easy_init(); + curl_easy_setopt(hCurlHandle, CURLOPT_URL, "http://example.com"); + curl_easy_setopt(hCurlHandle, CURLOPT_PROXY, "1"); + curl_easy_perform(hCurlHandle); + curl_easy_cleanup(hCurlHandle); + return 0; +} +``` +triggers the following Valgrind warning + +``` +==4125== Invalid read of size 8 +==4125== at 0x4E7D1EE: Curl_llist_remove (llist.c:97) +==4125== by 0x4E7EF5C: detach_connnection (multi.c:798) +==4125== by 0x4E80545: multi_runsingle (multi.c:1451) +==4125== by 0x4E8197C: curl_multi_perform (multi.c:2072) +==4125== by 0x4E766A0: easy_transfer (easy.c:625) +==4125== by 0x4E76915: easy_perform (easy.c:719) +==4125== by 0x4E7697C: curl_easy_perform (easy.c:738) +==4125== by 0x4008BE: main (in /home/even/curl/test) +==4125== Address 0x9b3d1d0 is 1,120 bytes inside a block of size 1,600 free'd +==4125== at 0x4C2ECF0: free (vg_replace_malloc.c:530) +==4125== by 0x4E62C36: conn_free (url.c:756) +==4125== by 0x4E62D34: Curl_disconnect (url.c:818) +==4125== by 0x4E48DF9: Curl_once_resolved (hostip.c:1097) +==4125== by 0x4E8052D: multi_runsingle (multi.c:1446) +==4125== by 0x4E8197C: curl_multi_perform (multi.c:2072) +==4125== by 0x4E766A0: easy_transfer (easy.c:625) +==4125== by 0x4E76915: easy_perform (easy.c:719) +==4125== by 0x4E7697C: curl_easy_perform (easy.c:738) +==4125== by 0x4008BE: main (in /home/even/curl/test) +==4125== Block was alloc'd at +==4125== at 0x4C2F988: calloc (vg_replace_malloc.c:711) +==4125== by 0x4E6438E: allocate_conn (url.c:1654) +==4125== by 0x4E685B4: create_conn (url.c:3496) +==4125== by 0x4E6968F: Curl_connect (url.c:4023) +==4125== by 0x4E802E7: multi_runsingle (multi.c:1368) +==4125== by 0x4E8197C: curl_multi_perform (multi.c:2072) +==4125== by 0x4E766A0: easy_transfer (easy.c:625) +==4125== by 0x4E76915: easy_perform (easy.c:719) +==4125== by 0x4E7697C: curl_easy_perform (easy.c:738) +==4125== by 0x4008BE: main (in /home/even/curl/test) +``` + +This has been bisected to commit 2f44e94 + +Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=14109 +Credit to OSS Fuzz + +Upstream-commit: 64cbae31078b2b64818a1d793516fbe73a7e4c45 +Signed-off-by: Kamil Dudka +--- + lib/multi.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/lib/multi.c b/lib/multi.c +index 856cc22..cad76c1 100644 +--- a/lib/multi.c ++++ b/lib/multi.c +@@ -1549,7 +1549,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, + if(result) + /* if Curl_once_resolved() returns failure, the connection struct + is already freed and gone */ +- Curl_detach_connnection(data); /* no more connection */ ++ data->conn = NULL; /* no more connection */ + else { + /* call again please so that we get the next socket setup */ + rc = CURLM_CALL_MULTI_PERFORM; +-- +2.20.1 + + +From 2d79804c0fab1efa594b1739a3e408c4ab031704 Mon Sep 17 00:00:00 2001 +From: Daniel Stenberg +Date: Tue, 28 May 2019 08:23:43 +0200 +Subject: [PATCH 2/2] multi: track users of a socket better + +They need to be removed from the socket hash linked list with more care. + +When sh_delentry() is called to remove a sockethash entry, remove all +individual transfers from the list first. To enable this, each Curl_easy struct +now stores a pointer to the sockethash entry to know how to remove itself. + +Reported-by: Tom van der Woerdt and Kunal Ekawde + +Fixes #3952 +Fixes #3904 +Closes #3953 + +Upstream-commit: 8581e1928ea8e125021408ec071fcedb0276c7fe +Signed-off-by: Kamil Dudka +--- + lib/multi.c | 38 ++++++++++++++++++++++++++------------ + lib/urldata.h | 1 + + 2 files changed, 27 insertions(+), 12 deletions(-) + +diff --git a/lib/multi.c b/lib/multi.c +index cad76c1..467cad8 100644 +--- a/lib/multi.c ++++ b/lib/multi.c +@@ -245,8 +245,17 @@ static struct Curl_sh_entry *sh_addentry(struct curl_hash *sh, + + + /* delete the given socket + handle from the hash */ +-static void sh_delentry(struct curl_hash *sh, curl_socket_t s) ++static void sh_delentry(struct Curl_sh_entry *entry, ++ struct curl_hash *sh, curl_socket_t s) + { ++ struct curl_llist *list = &entry->list; ++ struct curl_llist_element *e; ++ /* clear the list of transfers first */ ++ for(e = list->head; e; e = list->head) { ++ struct Curl_easy *dta = e->ptr; ++ Curl_llist_remove(&entry->list, e, NULL); ++ dta->sh_entry = NULL; ++ } + /* We remove the hash entry. This will end up in a call to + sh_freeentry(). */ + Curl_hash_delete(sh, (char *)&s, sizeof(curl_socket_t)); +@@ -806,6 +815,11 @@ bool Curl_pipeline_wanted(const struct Curl_multi *multi, int bits) + occasionally be called with the pointer already cleared. */ + void Curl_detach_connnection(struct Curl_easy *data) + { ++ if(data->sh_entry) { ++ /* still listed as a user of a socket hash entry, remove it */ ++ Curl_llist_remove(&data->sh_entry->list, &data->sh_queue, NULL); ++ data->sh_entry = NULL; ++ } + data->conn = NULL; + } + +@@ -2432,6 +2446,7 @@ static CURLMcode singlesocket(struct Curl_multi *multi, + /* add 'data' to the list of handles using this socket! */ + Curl_llist_insert_next(&entry->list, entry->list.tail, + data, &data->sh_queue); ++ data->sh_entry = entry; + } + + comboaction = (entry->writers? CURL_POLL_OUT : 0) | +@@ -2491,11 +2506,7 @@ static CURLMcode singlesocket(struct Curl_multi *multi, + multi->socket_cb(data, s, CURL_POLL_REMOVE, + multi->socket_userp, + entry->socketp); +- sh_delentry(&multi->sockhash, s); +- } +- else { +- /* remove this transfer as a user of this socket */ +- Curl_llist_remove(&entry->list, &data->sh_queue, NULL); ++ sh_delentry(entry, &multi->sockhash, s); + } + } + } /* for loop over numsocks */ +@@ -2539,7 +2550,7 @@ void Curl_multi_closed(struct Curl_easy *data, curl_socket_t s) + entry->socketp); + + /* now remove it from the socket hash */ +- sh_delentry(&multi->sockhash, s); ++ sh_delentry(entry, &multi->sockhash, s); + } + } + } +@@ -2630,7 +2641,6 @@ static CURLMcode multi_socket(struct Curl_multi *multi, + return result; + } + if(s != CURL_SOCKET_TIMEOUT) { +- + struct Curl_sh_entry *entry = sh_getentry(&multi->sockhash, s); + + if(!entry) +@@ -2643,15 +2653,19 @@ static CURLMcode multi_socket(struct Curl_multi *multi, + else { + struct curl_llist *list = &entry->list; + struct curl_llist_element *e; ++ struct curl_llist_element *enext; + SIGPIPE_VARIABLE(pipe_st); + + /* the socket can be shared by many transfers, iterate */ +- for(e = list->head; e; e = e->next) { ++ for(e = list->head; e; e = enext) { + data = (struct Curl_easy *)e->ptr; + +- if(data->magic != CURLEASY_MAGIC_NUMBER) +- /* bad bad bad bad bad bad bad */ +- return CURLM_INTERNAL_ERROR; ++ /* assign 'enext' here since the 'e' struct might be cleared ++ further down in the singlesocket() call */ ++ enext = e->next; ++ ++ DEBUGASSERT(data); ++ DEBUGASSERT(data->magic == CURLEASY_MAGIC_NUMBER); + + /* If the pipeline is enabled, take the handle which is in the head of + the pipeline. If we should write into the socket, take the +diff --git a/lib/urldata.h b/lib/urldata.h +index d4a5ad8..c793a6b 100644 +--- a/lib/urldata.h ++++ b/lib/urldata.h +@@ -1797,6 +1797,7 @@ struct Curl_easy { + struct curl_llist_element connect_queue; + struct curl_llist_element pipeline_queue; + struct curl_llist_element sh_queue; /* list per Curl_sh_entry */ ++ struct Curl_sh_entry *sh_entry; /* the socket hash this was added to */ + + CURLMstate mstate; /* the handle's state */ + CURLcode result; /* previous result */ +-- +2.20.1 + diff --git a/curl.spec b/curl.spec index cd05c49..08606d2 100644 --- a/curl.spec +++ b/curl.spec @@ -1,7 +1,7 @@ Summary: A utility for getting files from remote servers (FTP, HTTP, and others) Name: curl Version: 7.64.0 -Release: 7%{?dist} +Release: 8%{?dist} License: MIT Source: https://curl.haxx.se/download/%{name}-%{version}.tar.xz @@ -26,6 +26,9 @@ Patch16: 0016-curl-7.64.0-CVE-2019-5435.patch # fix TFTP receive buffer overflow (CVE-2019-5436) Patch17: 0017-curl-7.64.0-CVE-2019-5436.patch +# prevent multi from crashing with many parallel transfers (#1697566, #1723242) +Patch18: 0018-curl-7.64.0-multi-sigsegv.patch + # patch making libcurl multilib ready Patch101: 0101-curl-7.32.0-multilib.patch @@ -203,6 +206,7 @@ be installed. # upstream patches %patch16 -p1 %patch17 -p1 +%patch18 -p1 # make tests/*.py use Python 3 sed -e '1 s|^#!/.*python|#!%{__python3}|' -i tests/*.py @@ -363,6 +367,9 @@ rm -f ${RPM_BUILD_ROOT}%{_libdir}/libcurl.la %{_libdir}/libcurl.so.4.[0-9].[0-9].minimal %changelog +* Mon Jul 01 2019 Kamil Dudka - 7.64.0-8 +- prevent multi from crashing with many parallel transfers (#1697566, #1723242) + * Wed May 22 2019 Kamil Dudka - 7.64.0-7 - fix TFTP receive buffer overflow (CVE-2019-5436) - fix integer overflows in curl_url_set() (CVE-2019-5435)