Summary:	ASTERISK-28159: SIGABRT caused by stack corruption in hashkeys_read when no matching keys present
Reporter:	Michael Walton (mike@farsouthnet.com)	Labels:	patch
Date Opened:	2018-11-11 20:22:39.000-0600	Date Closed:	2018-11-26 07:26:35.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Functions/func_strings
Versions:	13.15.0	Frequency of Occurrence	Constant
Related Issues:
Environment:	Ubuntu 16.04, arm64	Attachments:	( 0) ASTERISK-28159.patch
Description:	On an arm64 build of Asterisk 13, a SIGABRT is raised, causing core dump. This was seen, and reproducible on a FreePBX 14 system in the macro-dial-one Dial() application, which causes a gosub to func-apply-sipheaders. This macro in turn reads HASHKEYS(SIPHEADERS), invoking the hashkeys_read() function via ast_func_read(). If there are no hash keys that match, asterisk crashes - on return from ast_func_read(), the compiler stack check fails with "stack smashing detected", causing SIGABRT. Stack trace is: {noformat} #0  0x0000ffff995ba528 in __GI_raise (sig=sig@entry=6)    at ../sysdeps/unix/sysv/linux/raise.c:54 #1  0x0000ffff995bb9e0 in __GI_abort () at abort.c:89 #2  0x0000ffff995f18c4 in __libc_message (do_abort=do_abort@entry=1,    fmt=fmt@entry=0xffff996a57e0 "*** %s ***: %s terminated\n")    at ../sysdeps/posix/libc_fatal.c:175 #3  0x0000ffff9965f668 in __GI___fortify_fail (    msg=msg@entry=0xffff996a57c0 "stack smashing detected")    at fortify_fail.c:37 #4  0x0000ffff9965f5fc in __stack_chk_fail () at stack_chk_fail.c:28 #5  0x000000000054a910 in ast_func_read (chan=chan@entry=0xffff50003bb8,    function=function@entry=0xffff1943cc50 "HASHKEYS(SIPHEADERS)",    workspace=workspace@entry=0xffff1943bc40 "", len=len@entry=4096)    at pbx_functions.c:640 #6  0x000000000054e238 in pbx_substitute_variables_helper_full (    c=c@entry=0xffff50003bb8, headp=0xffff50004380,    cp1=cp1@entry=0xffff1943ddd0 "SIPHEADERKEYS=${HASHKEYS(SIPHEADERS)}",    cp2=0xffff1943e2d6 "", cp2@entry=0xffff1943e2c8 "SIPHEADERKEYS=",    count=8177, count@entry=8191, used=used@entry=0xffff1943dda0)    at pbx_variables.c:693 #7  0x000000000054e898 in pbx_substitute_variables_helper (    c=c@entry=0xffff50003bb8,    cp1=cp1@entry=0xffff1943ddd0 "SIPHEADERKEYS=${HASHKEYS(SIPHEADERS)}", ---Type <return> to continue, or q <return> to quit---    cp2=cp2@entry=0xffff1943e2c8 "SIPHEADERKEYS=", count=count@entry=8191)    at pbx_variables.c:790 #8  0x000000000053d278 in pbx_extension_helper (c=0xffff50003bb8,    con=con@entry=0x0, context=0xffff50004570 "func-apply-sipheaders",    exten=0xffff500045c0 "s", priority=2, label=label@entry=0x0,    callerid=<optimized out>, action=action@entry=E_SPAWN,    found=0xffff194403d4, combined_find_spawn=1) at pbx.c:2873 #9  0x000000000053e25c in ast_spawn_extension (c=<optimized out>,    context=<optimized out>, exten=<optimized out>, priority=<optimized out>,    callerid=<optimized out>, found=<optimized out>,    combined_find_spawn=<optimized out>) at pbx.c:4109 #10 0x0000ffff9561a748 in ?? () {noformat}
Comments:	By: Asterisk Team (asteriskteam) 2018-11-11 20:22:41.121-0600 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. 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]. By: Michael Walton (mike@farsouthnet.com) 2018-11-11 20:26:36.173-0600 The corruption arises as a result of the following statement: {code:func_strings.c} static int hashkeys_read(struct ast_channel *chan, const char *cmd, char *data, char *buf, size_t len) { ... /* Trim the trailing comma */ buf[strlen(buf) - 1] = '\0'; return 0; } {code} Also equivalent at end of hashkeys_read2(). Simple code review shows that this will corrupt the object preceding the buffer (which may have been allocated on stack) if the buffer has zero strlen. By: Michael Walton (mike@farsouthnet.com) 2018-11-11 20:32:53.591-0600 Patch fixes this issue By: Kevin Harwell (kharwell) 2018-11-13 17:52:43.515-0600 [~mike@farsouthnet.com], Thanks for the contribution! Any interest in pushing this patch up to [gerrit|https://gerrit.asterisk.org]? If you'd like your contribution to be included faster, you should submit your patch for code review by the Asterisk Developer Community. To do so, please follow the Code Review [1] instructions on the wiki. Be sure to: * Verify that your patch conforms to the Coding Guidelines [2] * Review the Code Review Checklist [3] for common items reviewers will look for * If necessary, provide tests for the Asterisk Test Suite that verify the correctness of your patch [4] When ready, submit your patch and any tests to Gerrit [5] for code review. Thanks! [1] https://wiki.asterisk.org/wiki/display/AST/Code+Review [2] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines [3] https://wiki.asterisk.org/wiki/display/AST/Code+Review+Checklist [4] https://wiki.asterisk.org/wiki/display/AST/Asterisk+Test+Suite+Documentation [5] https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage By: Kevin Harwell (kharwell) 2018-11-16 14:57:40.437-0600 [~mike@farsouthnet.com] As you have not responded I went ahead and uploaded the patch to gerrit on your behalf. Thanks again for the contribution! By: Michael Walton (mike@farsouthnet.com) 2018-11-18 12:49:49.836-0600 Hi Kevin, it was on my to do list, but thank you for taking this on. By: Friendly Automation (friendly-automation) 2018-11-26 07:26:35.869-0600 Change 10664 merged by Jenkins2: func_strings: HASHKEY - negative array index can cause corruption [https://gerrit.asterisk.org/10664|https://gerrit.asterisk.org/10664] By: Friendly Automation (friendly-automation) 2018-11-26 07:44:40.444-0600 Change 10665 merged by Joshua Colp: func_strings: HASHKEY - negative array index can cause corruption [https://gerrit.asterisk.org/10665|https://gerrit.asterisk.org/10665] By: Friendly Automation (friendly-automation) 2018-11-26 07:45:15.892-0600 Change 10666 merged by Joshua Colp: func_strings: HASHKEY - negative array index can cause corruption [https://gerrit.asterisk.org/10666|https://gerrit.asterisk.org/10666]