[Home]

Summary:ASTERISK-16830: [patch] crashing func_curl hashcompat with invalid data
Reporter:Walter Doekes (wdoekes)Labels:
Date Opened:2010-10-19 04:21:01Date Closed:2011-05-03 13:25:12
Priority:CriticalRegression?No
Status:Closed/CompleteComponents: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