Summary: | ASTERISK-16830: [patch] crashing func_curl hashcompat with invalid data | ||
Reporter: | Walter Doekes (wdoekes) | Labels: | |
Date Opened: | 2010-10-19 04:21:01 | Date Closed: | 2011-05-03 13:25:12 |
Priority: | Critical | Regression? | No |
Status: | Closed/Complete | Components: | Functions/func_curl |
Versions: | Frequency of Occurrence | ||
Related Issues: | |||
Environment: | Attachments: | ( 0) 20101029__issue18161.diff.txt ( 1) 20110301__issue18161.diff.txt ( 2) issue18161_crash_func_curl_hashcompat.patch ( 3) issue18161_crash_func_curl_hashcompat-ignore-ws.patch ( 4) issue18161_crash_func_curl_some_more.patch | |
Description: | Hi, if you use the func_curl hashcompat mode, the remote_side of the curl call can crash asterisk in a couple of ways: (1) Supply a large amount of data, just enough for the allocation(s) of 'ast_str str' to succeed (through curl_easy_perform/WriteMemoryCallback), but too large for the fields = ast_str_create(..) and values = ast_str_create(..). When memory is full, _ast_str_create returns NULL, and then ast_str_append(&fields, 0, "%s%s", rowcount ? "," : "", name); will crash in __ast_str_helper at: int offset = (append && (*buf)->__AST_STR_LEN) ? (*buf)->__AST_STR_USED : 0; . (2) Supply '&&' in the data. This will cause: while ((piece = strsep(&remainder, "&"))) { char *name = strsep(&piece, "="); name to be "", but piece to be NULL. Then ast_uri_decode is called on piece and that function will happily dereference NULL, causing a crash. Marked as private, as it is trivial to crash an asterisk if it uses your website to get data. Regards, Walter Doekes ****** ADDITIONAL INFORMATION ****** Attached: patch with and without -w that should fix both problems. | ||
Comments: | By: Tilghman Lesher (tilghman) 2010-10-29 12:56:05 I have distilled your changes down to just the changes that are needed in my patch. A couple of notes: 1) ast_free(NULL) is a valid construction. There is no need to verify that a value is not NULL before freeing it. 2) Please only attach a single patch to an issue at a time. The latest patch should override all previous patches. By: Digium Subversion (svnbot) 2010-11-15 01:42:43.000-0600 Repository: asterisk Revision: 294988 U branches/1.6.2/funcs/func_curl.c ------------------------------------------------------------------------ r294988 | tilghman | 2010-11-15 01:42:42 -0600 (Mon, 15 Nov 2010) | 8 lines It is possible to crash Asterisk by feeding the curl config engine invalid data. (closes issue ASTERISK-16830) Reported by: wdoekes Patches: 20101029__issue18161.diff.txt uploaded by tilghman (license 14) Tested by: tilghman ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=294988 By: Digium Subversion (svnbot) 2010-11-15 01:43:58.000-0600 Repository: asterisk Revision: 294988 U branches/1.6.2/funcs/func_curl.c ------------------------------------------------------------------------ r294988 | tilghman | 2010-11-15 01:42:39 -0600 (Mon, 15 Nov 2010) | 8 lines It is possible to crash Asterisk by feeding the curl engine invalid data. (closes issue ASTERISK-16830) Reported by: wdoekes Patches: 20101029__issue18161.diff.txt uploaded by tilghman (license 14) Tested by: tilghman ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=294988 By: Digium Subversion (svnbot) 2010-11-15 01:44:41.000-0600 Repository: asterisk Revision: 294989 _U branches/1.8/ U branches/1.8/funcs/func_curl.c ------------------------------------------------------------------------ r294989 | tilghman | 2010-11-15 01:44:41 -0600 (Mon, 15 Nov 2010) | 15 lines Merged revisions 294988 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.6.2 ........ r294988 | tilghman | 2010-11-15 01:42:39 -0600 (Mon, 15 Nov 2010) | 8 lines It is possible to crash Asterisk by feeding the curl engine invalid data. (closes issue ASTERISK-16830) Reported by: wdoekes Patches: 20101029__issue18161.diff.txt uploaded by tilghman (license 14) Tested by: tilghman ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=294989 By: Digium Subversion (svnbot) 2010-11-15 01:45:46.000-0600 Repository: asterisk Revision: 294990 _U trunk/ U trunk/funcs/func_curl.c ------------------------------------------------------------------------ r294990 | tilghman | 2010-11-15 01:45:45 -0600 (Mon, 15 Nov 2010) | 22 lines Merged revisions 294989 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.8 ................ r294989 | tilghman | 2010-11-15 01:44:38 -0600 (Mon, 15 Nov 2010) | 15 lines Merged revisions 294988 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.6.2 ........ r294988 | tilghman | 2010-11-15 01:42:39 -0600 (Mon, 15 Nov 2010) | 8 lines It is possible to crash Asterisk by feeding the curl engine invalid data. (closes issue ASTERISK-16830) Reported by: wdoekes Patches: 20101029__issue18161.diff.txt uploaded by tilghman (license 14) Tested by: tilghman ........ ................ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=294990 By: Walter Doekes (wdoekes) 2011-02-28 06:59:24.000-0600 Hah. In non-optimized mode it still crashes. But now on ast_uri_decode when attempting to write a \0 to the first byte of the constant "". By: Walter Doekes (wdoekes) 2011-02-28 07:04:47.000-0600 (I don't know if it's the optimization or other changes, but in any case. "" shouldn't be written to.) The attached patch fixes it. I could've went with an if around ast_uri_decode instead, but this feels faster. By: Tilghman Lesher (tilghman) 2011-03-01 00:11:44.000-0600 It's faster to call ast_uri_decode than it is to not call it at all? Really? By: Walter Doekes (wdoekes) 2011-03-01 01:18:34.000-0600 Well.. the regular case would be when we call it. The only time we wouldn't call it would be if (piece[0] == '\0'). Imagine a document with lots of properly formatted key value pairs: we don't need to do the extra if for every iteration, we only need it for invalid data. And, if you put the if there, you implicitly have *two* checks for piece being NULL, because the second if was really only needed when piece was NULL originally. But if you do like the if, we could've went with (parts of) my original patch, and put the whole assigment stuff in the if (piece != NULL) {} block. By: Walter Doekes (wdoekes) 2011-03-03 03:51:59.000-0600 20110301__issue18161.diff.txt works for me. You still get the double-if (a triple-if if combined with ASTERISK-16727 even), but I can live with that. By: Digium Subversion (svnbot) 2011-05-03 13:25:04 Repository: asterisk Revision: 316093 U branches/1.6.2/funcs/func_curl.c ------------------------------------------------------------------------ r316093 | tilghman | 2011-05-03 13:25:02 -0500 (Tue, 03 May 2011) | 8 lines More possible crashes based upon invalid inputs. (closes issue ASTERISK-16830) Reported by: wdoekes Patches: 20110301__issue18161.diff.txt uploaded by tilghman (license 14) Tested by: wdoekes ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=316093 By: Digium Subversion (svnbot) 2011-05-03 13:25:08 Repository: asterisk Revision: 316094 _U branches/1.8/ U branches/1.8/funcs/func_curl.c ------------------------------------------------------------------------ r316094 | tilghman | 2011-05-03 13:25:07 -0500 (Tue, 03 May 2011) | 15 lines Merged revisions 316093 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.6.2 ........ r316093 | tilghman | 2011-05-02 14:04:36 -0500 (Mon, 02 May 2011) | 8 lines More possible crashes based upon invalid inputs. (closes issue ASTERISK-16830) Reported by: wdoekes Patches: 20110301__issue18161.diff.txt uploaded by tilghman (license 14) Tested by: wdoekes ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=316094 By: Digium Subversion (svnbot) 2011-05-03 13:25:11 Repository: asterisk Revision: 316095 _U trunk/ U trunk/funcs/func_curl.c ------------------------------------------------------------------------ r316095 | tilghman | 2011-05-03 13:25:11 -0500 (Tue, 03 May 2011) | 22 lines Merged revisions 316094 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.8 ................ r316094 | tilghman | 2011-05-02 14:09:55 -0500 (Mon, 02 May 2011) | 15 lines Merged revisions 316093 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.6.2 ........ r316093 | tilghman | 2011-05-02 14:04:36 -0500 (Mon, 02 May 2011) | 8 lines More possible crashes based upon invalid inputs. (closes issue ASTERISK-16830) Reported by: wdoekes Patches: 20110301__issue18161.diff.txt uploaded by tilghman (license 14) Tested by: wdoekes ........ ................ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=316095 |