[Home]

Summary:ASTERISK-14461: [patch] Asterisk runs over end of buffer reading manager input over HTTP and segfaults
Reporter:Peter Fern (pdf)Labels:
Date Opened:2009-07-13 23:11:40Date Closed:2010-04-15 12:57:04
Priority:CriticalRegression?No
Status:Closed/CompleteComponents:Core/HTTP
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20090916__issue15495.diff.txt
( 1) asterisk-1.4-manager-add-null.patch
( 2) asterisk-1.4-manager-add-null-not-so-stupid.patch
Description:We have a number of applications working over manager, and whilst I have not been able to nail down what precisely is causing this, it has occurred a number of times.  It looks like xml_translate is looking for a null-terminated string, but the string is not always null-terminated, so it runs off the end of the buffer and segfaults.

****** ADDITIONAL INFORMATION ******

#0  0x080c9ad5 in xml_translate (in=0xb5bbc004 <Address 0xb5bbc004 out of bounds>, vars=0x81d9e80) at manager.c:424
v = (struct ast_variable *) 0x0
dest = 0x815f153 "unknown"
out = 0x829dca0 "<response type='object' id='unknown'><generic response='Success' message='Waiting for Event...' /></response>\n<response type='object' id='unknown'><generic event='Newexten' privilege='call,all' channe"...
tmp = 0x829f20a ""
var = 0xb5bbc000 <Address 0xb5bbc000 out of bounds>
val = 0xb5bbbfeb "WaitEventComplete"
objtype = 0x815f15b "generic"
colons = 185
breaks = 205
len = 10836
count = 0
escaped = 22
inobj = 0
x = 4103
vc = (struct variable_count *) 0x82888a0
vco = (struct ao2_container *) 0x0
__PRETTY_FUNCTION__ = "xml_translate"
#1  0x080d34ae in generic_http_callback (format=2, requestor=0xb5f6ad10, uri=0xb569533e "", params=0x81d9e80, status=0xb5694220, title=0xb5694224, contentlength=0xb569421c) at manager.c:2890
tmpbuf = 0x828fe04 ""
buf = 0xb5bbb000 "Response"
l = 4096
m = {hdrcount = 3, headers = {0xb5693bc0 "action: WaitEvent", 0xb5693ba0 "Timeout: 30", 0xb5693b70 "mansession_id: 55c63fbe", 0x0 <repeats 125 times>}}
tmp = "55c63fbe", '\0' <repeats 56 times>, "\033[1;35;40mDAHDI/"
x = 3
hdrlen = 24
s = (struct mansession_session *) 0x8290728
ss = {f = 0x824c6d8, session = 0x8290728, fd = 98}
ident = 1439055806
workspace = "Content-type: text/xml\r\nSet-Cookie: mansession_id=\"55c63fbe\"; Version=\"1\"; Max-Age=60\r\n\r\n<ajax-response>\n\000\000\000\201??", '\0' <repeats 28 times>, "\030\002P?H\000P?\000\000\000\000\000\000\000\000H\000P?@\000P?\000?\000\000\000\000\000\000\000\000\000\000?\021\210\a?200\003", '\0' <repeats 23 times>, "\021\003\000\000\000\000\000\000\000\020\000\000\b\000\000\000\000\000\000\000\r`I"...
cookie = "Set-Cookie: mansession_id=\"55c63fbe\"; Version=\"1\"; Max-Age=60\r\n\0000mMacro\033[0;37;40m\000m\000m\000\000\000dDi?dDi?\220ki?????\000O?Ei??Di??\r?\020Ei?\000O??"
len = 407
blastaway = 0
c = 0xb5693f69 ""
retval = 0x0
v = (struct ast_variable *) 0x0
__PRETTY_FUNCTION__ = "generic_http_callback"
#2  0x080d3ac6 in mxml_http_callback (requestor=0xb5f6ad10, uri=0xb569533e "", params=0x81d9e80, status=0xb5694220, title=0xb5694224, contentlength=0xb569421c) at manager.c:2978
No locals.
#3  0x080bbd76 in handle_uri (sin=0xb5f6ad10, uri=0xb569533e "", status=0xb5694220, title=0xb5694224, contentlength=0xb569421c, cookies=0xb5694228, static_content=0xb5694214) at http.c:369
c = 0xb56941d8 "Hci?\226?\v\b\020???>Si? Bi?$Bi?\034Bi?(Bi?\024Bi?"
turi = 0xb569533e ""
params = 0x0
var = 0x0
val = 0xb5695358 "30"
urih = (struct ast_http_uri *) 0x81a4dec
len = 4
vars = (struct ast_variable *) 0x81d9e80
v = (struct ast_variable *) 0x8264d58
prev = (struct ast_variable *) 0x8264d58
__PRETTY_FUNCTION__ = "handle_uri"
#4  0x080bc396 in ast_httpd_helper_thread (data=0xb5f6ad08) at http.c:471
buf = "GET\000/asterisk/mxml\000action\000WaitEvent\000Timeout\00030\000HTTP/1.1\r\n", '\0' <repeats 1127 times>, "\003|?\000\000\000\0004\230?\000\000\000\000#?\026\b#?\026\b\001\000\000\000????\"?\026\b?^i??]i??\r?\034^i?\"?\026\b\001", '\0' <repeats 19 times>, "\220ki??\032?\000\000\000\000\000\000\000\000?]i?\000\000\000\000\000\000\000\000h]i?", '\0' <repeats 24 times>, "(\000\000\000\000\000\000\000?^i?", '\0' <repeats 32 times>, "\003|"...
cookie = "\000\000\000kie: mansession_id\000\"55c63fbe\000\000\000\0000.7,*;q=0.7\000\000\000on/xml;q=0.9,*/*;q=0.8\000\000\0009060200 SUSE/3.0.11-0.1.1 Firefox/3.0.11\000\000\0005903378.1240378442.54.3.utmcsr=10.10.0.124|utmccn=(referral)|utmcmd=referral|utmcct"...
timebuf = "Tue, 30 Jun 2009 00:45:38 GMT", '\0' <repeats 226 times>
ser = (struct ast_http_server_instance *) 0xb5f6ad08
vars = (struct ast_variable *) 0x0
uri = 0xb5695330 "/asterisk/mxml"
c = 0xb569535a ""
title = 0x0
status = 200
contentlength = 0
---Type <return> to continue, or q <return> to quit---
t = 1246322738
static_content = 0
__PRETTY_FUNCTION__ = "ast_httpd_helper_thread"
ASTERISK-1  0x08121fa6 in dummy_start (data=0xb7b12ef8) at utils.c:856
__cancel_buf = {__cancel_jmp_buf = {{__cancel_jmp_buf = {-1209036812, 0, -1251382384, -1251384376, -1768841520, 736813836}, __mask_was_saved = 0}}, __pad = {0xb5696480, 0x0, 0x8154878, 0x13a}}
__cancel_routine = (void (*)(void *)) 0x806a04e <ast_unregister_thread>
__cancel_arg = (void *) 0xb5696b90
not_first_call = 0
ret = (void *) 0xb7ee8f43
a = {start_routine = 0x80bc0ed <ast_httpd_helper_thread>, data = 0xb5f6ad08, name = 0xb7bb5870 "ast_httpd_helper_thread started at [  555] http.c http_root()"}
lock_info = (struct thr_lock_info *) 0x828f5f8
mutex_attr = {__size = "\001\000\000", __align = 1}
__PRETTY_FUNCTION__ = "dummy_start"
ASTERISK-2  0xb7ee9112 in start_thread () from /lib/libpthread.so.0
No symbol table info available.
ASTERISK-3  0xb7df42ee in clone () from /lib/libc.so.6
No symbol table info available.
Comments:By: Peter Fern (pdf) 2009-07-13 23:17:11

Attached patch is inelegant, but solves problem by appending a null to the string buffer.  I may add code to do this properly, but needed to fix it quickly.


By: Peter Fern (pdf) 2009-07-13 23:59:42

Umm, let's try that again without doing something really stupid.
By: Jason Parker (jparker) 2009-08-20 14:28:04

Are you able to provide sample input that causes this to happen?
By: Peter Fern (pdf) 2009-08-20 22:05:37

Unfortunately it's been difficult for us to reproduce, or I would have included it in the original report.  It's also fairly sporadic, so the only way it was reproduced was by running against a live and loaded system for a couple of days, which I can't unpatch and do again without some pain.

I'll see if I can come up with some sort of plan to capture the data, but it's not going to be easy in the short term.  If I can come up with something, what format do you need it in?  The only way I can think that might work is to capture all manager packets arriving on an unpatched machine, and try correlate the crash with the packet that caused it, though that might be difficult with multiple manager clients running.
By: Peter Fern (pdf) 2009-09-02 02:07:02

Capturing the input and correlating to the crash is proving just too difficult - it takes more time than we have to reproduce in the lab, and we can't leave live systems unpatched if they're going to be susceptible to remote crashes.

The most successful method of reproducing the bug has been to have many users making calls via:

https://addons.mozilla.org/en-US/firefox/addon/8510

But I haven't been able to differentiate crash-causing input from the rest, and it can take hours to days to for a segfault to occur, even though it looks like predominantly the same data is being sent down the wire.

Please let me know what else we can do to help get this bug fixed - I'm sure you'll agree that remote segfaults are serious business.


By: Leif Madsen (lmadsen) 2009-09-02 08:27:59

I'm assigning this to Tilghman in order to get this thoughts, and see if he has some suggestions for how to move this issue forward. Thanks!
By: Tilghman Lesher (tilghman) 2009-09-02 16:31:18

I think it's much more sane simply to NULL-terminate the file, before mmap'ing it into memory.  Patch uploaded.
By: Peter Fern (pdf) 2009-09-02 22:27:29

I did it after it was mmap'd because I wasn't sure if there were concurrency issues with modifying the file at this point (was my first instinct).  I'll deploy this to a couple of sites next week and confirm that it resolves the issue.

Thanks
By: Peter Fern (pdf) 2009-09-09 02:20:45

Since gcc generates a warning using this patch:

manager.c: In function ‘generic_http_callback’:
manager.c:2886: warning: zero-length printf format string

it's probably not a candidate for inclusion in the tree.

Shouldn't the null character really be added wherever that tmpfile is being written to?  I don't understand why it is missing a null at this point, because I haven't traced the code all the way back, but it concerns me that it may be using an asterisk function that is also called elsewhere, possibly resulting in a similar crash in another part of the code.  My patch was only really meant as a band-aid for this case.
By: Tilghman Lesher (tilghman) 2009-09-16 14:39:32

New patch uploaded that should not generate a warning.  The null character should not be added at all times when tmpfile is written, because that would advance the file pointer and cause the string read back out to be terminated early.
By: Peter Fern (pdf) 2009-09-16 20:44:34

Hmm, OK.  Give me a day or two and I'll get this tested.
By: Tilghman Lesher (tilghman) 2009-10-22 10:31:15

pdf:  It has now been more than a month.  Have you had sufficient time to test this yet?
By: Peter Fern (pdf) 2009-10-22 20:04:44

Apologies for the delay - things have been flat out here for the past few weeks.  I have had it installed on a lightly loaded system and have not experienced the crash, however I've been planning on having it deployed to some larger sites before confirming here.  I have that scheduled for next week, but at this stage, the fix appears to be functioning as expected.
By: Digium Subversion (svnbot) 2009-10-27 15:21:40

Repository: asterisk
Revision: 226138

U   branches/1.4/main/manager.c

------------------------------------------------------------------------
r226138 | tilghman | 2009-10-27 15:21:39 -0500 (Tue, 27 Oct 2009) | 7 lines

Manager output is not always NULL-terminated, so force a NULL at the end of the filestream.
(closes issue ASTERISK-14461)
Reported by: pdf
Patches:
      20090916__issue15495.diff.txt uploaded by tilghman (license 14)
Tested by: pdf

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=226138
By: Digium Subversion (svnbot) 2009-10-27 15:27:00

Repository: asterisk
Revision: 226159

_U  trunk/
U   trunk/main/manager.c

------------------------------------------------------------------------
r226159 | tilghman | 2009-10-27 15:27:00 -0500 (Tue, 27 Oct 2009) | 14 lines

Merged revisions 226138 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r226138 | tilghman | 2009-10-27 15:16:49 -0500 (Tue, 27 Oct 2009) | 7 lines
 
 Manager output is not always NULL-terminated, so force a NULL at the end of the filestream.
 (closes issue ASTERISK-14461)
  Reported by: pdf
  Patches:
        20090916__issue15495.diff.txt uploaded by tilghman (license 14)
  Tested by: pdf
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=226159
By: Digium Subversion (svnbot) 2009-10-27 15:28:46

Repository: asterisk
Revision: 226167

_U  branches/1.6.0/
U   branches/1.6.0/main/manager.c

------------------------------------------------------------------------
r226167 | tilghman | 2009-10-27 15:28:45 -0500 (Tue, 27 Oct 2009) | 21 lines

Merged revisions 226159 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r226159 | tilghman | 2009-10-27 15:22:07 -0500 (Tue, 27 Oct 2009) | 14 lines
 
 Merged revisions 226138 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r226138 | tilghman | 2009-10-27 15:16:49 -0500 (Tue, 27 Oct 2009) | 7 lines
   
   Manager output is not always NULL-terminated, so force a NULL at the end of the filestream.
   (closes issue ASTERISK-14461)
    Reported by: pdf
    Patches:
          20090916__issue15495.diff.txt uploaded by tilghman (license 14)
    Tested by: pdf
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=226167
By: Digium Subversion (svnbot) 2009-10-27 15:28:53

Repository: asterisk
Revision: 226169

_U  branches/1.6.1/
U   branches/1.6.1/main/manager.c

------------------------------------------------------------------------
r226169 | tilghman | 2009-10-27 15:28:53 -0500 (Tue, 27 Oct 2009) | 21 lines

Merged revisions 226159 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r226159 | tilghman | 2009-10-27 15:22:07 -0500 (Tue, 27 Oct 2009) | 14 lines
 
 Merged revisions 226138 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r226138 | tilghman | 2009-10-27 15:16:49 -0500 (Tue, 27 Oct 2009) | 7 lines
   
   Manager output is not always NULL-terminated, so force a NULL at the end of the filestream.
   (closes issue ASTERISK-14461)
    Reported by: pdf
    Patches:
          20090916__issue15495.diff.txt uploaded by tilghman (license 14)
    Tested by: pdf
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=226169
By: Digium Subversion (svnbot) 2009-10-27 15:28:59

Repository: asterisk
Revision: 226170

_U  branches/1.6.2/
U   branches/1.6.2/main/manager.c

------------------------------------------------------------------------
r226170 | tilghman | 2009-10-27 15:28:59 -0500 (Tue, 27 Oct 2009) | 21 lines

Merged revisions 226159 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r226159 | tilghman | 2009-10-27 15:22:07 -0500 (Tue, 27 Oct 2009) | 14 lines
 
 Merged revisions 226138 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r226138 | tilghman | 2009-10-27 15:16:49 -0500 (Tue, 27 Oct 2009) | 7 lines
   
   Manager output is not always NULL-terminated, so force a NULL at the end of the filestream.
   (closes issue ASTERISK-14461)
    Reported by: pdf
    Patches:
          20090916__issue15495.diff.txt uploaded by tilghman (license 14)
    Tested by: pdf
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=226170