Summary:	ASTERISK-29085: func_curl: Segmentation fault when using CURL after setting httpheader CURLOPT
Reporter:	Péter Juhász (peter.juhasz)	Labels:	patch
Date Opened:	2020-09-17 07:50:46	Date Closed:	2020-09-23 08:08:39
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Functions/func_curl
Versions:	16.8.0	Frequency of Occurrence	Constant
Related Issues:	is related to ASTERISK-28613 func_curl: CURLOPT cannot set Content-Type header
Environment:	Fedora 32 Linux x86_64	Attachments:	( 0) 0001-func_curl-Clear-HTTP-headers-form-shared-cURL-instan.patch ( 1) gdb.txt
Description:	The capability to set HTTP headers was recently added to Asterisk (in issue ASTERISK-28613), but it turns out that this functionality is unsafe in its current implementation, because it is possible to induce a segmentation fault with some combinations of CURLOPT calls. The steps to reproduce: - Set CURLOPT(httpheader)=Content-Type: application/json - use CURL to send POST JSON data to some HTTPS service - Set some other CURLOPT that is not httpheader (e.g. userpwd, httptimeout) - use CURL again With such a dialplan Asterisk crashes consistently. We have a coredump, but it contains potentially sensitive data, so I don't want to upload it to the public tracker. Analyzing the coredump, it appears that curl->set.headers in acf_curl_helper contains garbage, or more precisel, the data and next pointers in that structure became stale since the first call to CURL.
Comments:	By: Asterisk Team (asteriskteam) 2020-09-17 07:50:48.328-0500 Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution. Please note that log messages and other files should not be sent to the Sangoma Asterisk Team unless explicitly asked for. All files should be placed on this issue in a sanitized fashion as needed. A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report. Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process]. Please note that once your issue enters an open state it has been accepted. As Asterisk is an open source project there is no guarantee or timeframe on when your issue will be looked into. If you need expedient resolution you will need to find and pay a suitable developer. Asking for an update on your issue will not yield any progress on it and will not result in a response. All updates are posted to the issue when they occur. Please note that by submitting data, code, or documentation to Sangoma through JIRA, you accept the Terms of Use present at [https://www.asterisk.org/terms-of-use/|https://www.asterisk.org/terms-of-use/]. By: Benjamin Keith Ford (bford) 2020-09-17 10:17:13.436-0500 Tried to replicate this, but did not get a crash. Since you have sensitive information, would it be possible to attach just thread 1 from the backtrace where the crash is occurring? You can sanitize any private info (if any). By: Péter Juhász (peter.juhasz) 2020-09-17 10:35:36.318-0500 Backtrace attached. By: Péter Juhász (peter.juhasz) 2020-09-17 10:43:18.693-0500 Looking at func_curl.c, I now think that the CURLOPT calls before the second CURL call might not even be relevant. There is a call to curl_slist_free_all(headers) in acf_curl_helper when CURL is used for the first time in the dialplan, which frees the actual string that contained the HTTP header, but the pointer to that freed string remains inside the structure that is kept around and passed again to libcurl. If there is no CURLOPT(httpheader) before the second or any subsequent CURL call, that part of the structure is not modified. So I think CURLOPT(httpheader) is not safe to use in its current implementation. By: Sean Bright (seanbright) 2020-09-17 12:51:23.122-0500 Can you try [the attached patch|^0001-func_curl-Clear-HTTP-headers-form-shared-cURL-instan.patch] and let us know if it resolves the crash you are experiencing? By: Péter Juhász (peter.juhasz) 2020-09-17 13:05:00.271-0500 I will try (tomorrow morning CEST)! I expect that it will resolve the crash, but it also changes the behavior of the option from the intended and documented way, which states that headers persist and subsequent calls to CURLOPT(httpheader) add to the list (as opposed to replacing it). (I do think that this interface is suboptimal and that it should be replaced with something more flexible and more in line with the rest of the function.) By: Sean Bright (seanbright) 2020-09-17 13:10:16.241-0500 bq. it also changes the behavior of the option from the intended and documented way, which states that headers persist and subsequent calls to CURLOPT(httpheader) add to the list (as opposed to replacing it). Which documentation are you referring to? By: Péter Juhász (peter.juhasz) 2020-09-17 13:15:02.884-0500 The one embedded in the file func_curl.c: {code} <enum name="httpheader"> <para>Add HTTP header. Multiple calls add multiple headers. Setting of any header will remove the default "Content-Type application/x-www-form-urlencoded"</para> </enum> {code} And there is the comment at line 431: {code} /* Remove any existing entry, only http headers are left */ {code} This was the stated intent, it's a different question that apparently it never worked properly. I'm just saying that if the proposed fix changes the behavior of the module, the documentation will have to be adjusted too (in which case the fix would be fine). By: Sean Bright (seanbright) 2020-09-17 13:17:35.378-0500 I do not believe that the patch I have provided affects that behavior. Let us know after your testing. By: Péter Juhász (peter.juhasz) 2020-09-18 06:40:52.195-0500 After testing: There were no crashes, and you were right, the headers were not cleared between calls. (If I understand the code correctly, the settings changed by CURLOPT calls are stored in the channel's datastore, and the necessary libcurl data structure is assembled/updated every time before libcurl actually performs the request, and I was worried because in the coredump not just the libcurl data structure but the channel data store appeared to contain corrupted data.) By: Sean Bright (seanbright) 2020-09-18 08:18:01.282-0500 Thank you for following up. I've put up a (slightly different) patch for review on Gerrit. By: Friendly Automation (friendly-automation) 2020-09-23 08:08:40.524-0500 Change 14930 merged by Friendly Automation: func_curl.c: Prevent crash when using CURLOPT(httpheader) [https://gerrit.asterisk.org/c/asterisk/+/14930|https://gerrit.asterisk.org/c/asterisk/+/14930] By: Friendly Automation (friendly-automation) 2020-09-23 10:05:52.400-0500 Change 14973 merged by Joshua Colp: func_curl.c: Prevent crash when using CURLOPT(httpheader) [https://gerrit.asterisk.org/c/asterisk/+/14973|https://gerrit.asterisk.org/c/asterisk/+/14973] By: Friendly Automation (friendly-automation) 2020-09-23 10:06:13.321-0500 Change 14972 merged by Joshua Colp: func_curl.c: Prevent crash when using CURLOPT(httpheader) [https://gerrit.asterisk.org/c/asterisk/+/14972|https://gerrit.asterisk.org/c/asterisk/+/14972] By: Friendly Automation (friendly-automation) 2020-09-23 10:06:31.790-0500 Change 14971 merged by Joshua Colp: func_curl.c: Prevent crash when using CURLOPT(httpheader) [https://gerrit.asterisk.org/c/asterisk/+/14971|https://gerrit.asterisk.org/c/asterisk/+/14971] By: Friendly Automation (friendly-automation) 2020-09-23 10:06:47.982-0500 Change 14970 merged by Joshua Colp: func_curl.c: Prevent crash when using CURLOPT(httpheader) [https://gerrit.asterisk.org/c/asterisk/+/14970|https://gerrit.asterisk.org/c/asterisk/+/14970]