[Home]

Summary:ASTERISK-13344: [patch] app_page causes undefined behavior when paging a page group with more than 128 extensions
Reporter:Alex Villacís Lasso (a_villacis)Labels:
Date Opened:2009-01-12 10:06:42.000-0600Date Closed:2009-01-13 20:11:01.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Applications/app_page
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20080912-asterisk-app_page-fix-buffer-overflow.patch
Description:When defining a paging group, if this group has more than 128 extensions, an attempt to ring this paging group causes *all* calls (including those belonging to extensions not part of the group) to be hung up. Analysis of the root problem shows that a range of undefined behaviors can occur, up to and including the crash of the Asterisk server.

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

Classic buffer overflow.

When defining a paging group, a list of extensions must be defined, separated with ampersands. Within the code, this list is tokenized and used to populate dialing contexts in a static array declared as a local variable of a function. This static array has a capacity of MAX_DIALS = 128, but the code apparently has no verifications to ensure that the buffer is not being filled beyond its declared capacity. A client of ours declared a paging group with 130 extensions, and we saw the buffer overflowing and scribbling over the next few variables, including the variable that keeps track of the number of actual extensions. On context cleanup, the number of extensions is now invalid and leads to undefined behavior. In this particular client case, he experienced hangup of all calls, including calls on extensions not included in the page group.

While fixing this problem, we found a potential second problem: in the scenario in which the n-th dialing context cannot be created (for whatever reason), or if the n-th extension happens to be the very extension that is issuing the paging, the array position reserved for the n-th dialing context created remains uninitialized. In the cleanup loop, the code incorrectly assumes that the n-th position is always valid and points to a valid context. On the previously described scenario, this leads to invalid memory accesses, which may lead to Asterisk crashing.

No attempt has been made to verify whether this overflow is a security issue. But an user that belongs to the problematic paging group can certainly crash or otherwise disrupt the server by paging to its own group.
Comments:By: Alex Villacís Lasso (a_villacis) 2009-01-12 10:09:02.000-0600

We at Palosanto Solutions have been shipping our asterisk build with this patch applied, but I consider this patch should be reviewed and possibly merged upstream, for both 1.4.x and 1.6.x series of Asterisk.

The attached patch (20080912-asterisk-app_page-fix-buffer-overflow.patch) fixes both identified issues. This patch counts the number of extensions in the list and dynamically allocates enough memory for the actual number of extensions in the page group. As a side effect, the MAX_PAGE limit is now removed and the extension list can be arbitrarily long, memory permitting. Also the array positions are initialized to NULL before context allocation and checked for NULL on cleanup.


* apps/app_page.c
- Fix buffer overflow caused by attempts to define a page group with more than 128 extensions.
- Fix potential invalid memory access on cleanup of a dialing structure through an uninitialized pointer after failure to create at least one dialing structure.
By: Leif Madsen (lmadsen) 2009-01-12 10:30:50.000-0600

Assigned to otherwiseguy for review. Thanks!
By: Terry Wilson (twilson) 2009-01-13 18:12:03.000-0600

There are a couple of issues, I think.  First, I'm totally surprised that paging 128 phones simultaneously works, wow!.  Second, I'm not sure completely removing the upper limit on number of phones paged simultaneously is a great idea, but the question is always where to draw the line I suppose.  Third, the ast_malloc for the dial_list allocates too much space, it is an array of pointers, not an array of struct ast_dials, and since you are setting the pointer to NULL anyway, it would be better to:

dial_list = ast_calloc(num_dials, sizeof(void *));

and get rid of the later initialization.

Anyway, this is something that obviously need to be fixed quickly, I may go ahead and make some changes to the patch instead of waiting for it to be edited.  Of course, the simple thing to do would be to just enforce the 128 call limit.
By: Terry Wilson (twilson) 2009-01-13 19:16:55.000-0600

Ok, the changes I made to the patch are as follows:

1) Started num_dials at 1 to remove axtra num_dials++ and remoted the if (*tmp) since the while(*p) was the same check.  Also reformatted it a bit to conform with coding guidelines.
2) The above mentioned ast_malloc to ast_calloc change, combined with doing the allocation and check for failure at the same time, ala if (!(dial_list = ast_calloc(num_dials, sizeof(void *)) {}
3) Rmoved the "Ensure this dial slot is valid" if (dial = NULL) continue; line.  It looks like pos is only incremented when there is a non-NULL dial, so if n=8, pos will equal 9 when the last entry is added.  The for loop is for(i = 0; i < 9;i++) in that case which would stay within the bounds of our array.

If you find any errors with any of this, please reopen the ticket and let me know.
By: Digium Subversion (svnbot) 2009-01-13 19:27:19.000-0600

Repository: asterisk
Revision: 168593

U   branches/1.4/apps/app_page.c

------------------------------------------------------------------------
r168593 | twilson | 2009-01-13 19:27:19 -0600 (Tue, 13 Jan 2009) | 20 lines

Don't overflow when paging more than 128 extensions

The number of available slots for calls in app_page was hardcoded to 128.
Proper bounds checking was not in place to enforce this limit, so if more than
128 extensions were passed to the Page() app, Asterisk would crash.  This patch
instead dynamically allocates memory for the ast_dial structures and removes
the (non-functional) arbitrary limit.

This issue would have special importance to anyone who is dynamically creating
the argument passed to the Page application and allowing more than 128
extensions to be added by an outside user via some external interface.

The patch posted by a_villacis was slightly modified for some coding guidelines
and other cleanups.  Thanks, a_villacis!
(closes issue ASTERISK-13344)
Reported by: a_villacis
Patches:
     20080912-asterisk-app_page-fix-buffer-overflow.patch uploaded by a (license 660)
Tested by: otherwiseguy

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

http://svn.digium.com/view/asterisk?view=rev&revision=168593
By: Digium Subversion (svnbot) 2009-01-13 20:00:40.000-0600

Repository: asterisk
Revision: 168594

_U  trunk/
U   trunk/apps/app_page.c

------------------------------------------------------------------------
r168594 | twilson | 2009-01-13 20:00:40 -0600 (Tue, 13 Jan 2009) | 27 lines

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

........
 r168593 | twilson | 2009-01-13 19:27:18 -0600 (Tue, 13 Jan 2009) | 20 lines
 
 Don't overflow when paging more than 128 extensions
 
 The number of available slots for calls in app_page was hardcoded to 128.
 Proper bounds checking was not in place to enforce this limit, so if more than
 128 extensions were passed to the Page() app, Asterisk would crash.  This patch
 instead dynamically allocates memory for the ast_dial structures and removes
 the (non-functional) arbitrary limit.
 
 This issue would have special importance to anyone who is dynamically creating
 the argument passed to the Page application and allowing more than 128
 extensions to be added by an outside user via some external interface.
 
 The patch posted by a_villacis was slightly modified for some coding guidelines
 and other cleanups.  Thanks, a_villacis!
 (closes issue ASTERISK-13344)
 Reported by: a_villacis
 Patches:
       20080912-asterisk-app_page-fix-buffer-overflow.patch uploaded by a (license 660)
 Tested by: otherwiseguy
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=168594
By: Digium Subversion (svnbot) 2009-01-13 20:06:20.000-0600

Repository: asterisk
Revision: 168595

_U  branches/1.6.0/
U   branches/1.6.0/apps/app_page.c

------------------------------------------------------------------------
r168595 | twilson | 2009-01-13 20:06:19 -0600 (Tue, 13 Jan 2009) | 34 lines

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

................
 r168594 | twilson | 2009-01-13 20:00:40 -0600 (Tue, 13 Jan 2009) | 27 lines
 
 Merged revisions 168593 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r168593 | twilson | 2009-01-13 19:27:18 -0600 (Tue, 13 Jan 2009) | 20 lines
   
   Don't overflow when paging more than 128 extensions
   
   The number of available slots for calls in app_page was hardcoded to 128.
   Proper bounds checking was not in place to enforce this limit, so if more than
   128 extensions were passed to the Page() app, Asterisk would crash.  This patch
   instead dynamically allocates memory for the ast_dial structures and removes
   the (non-functional) arbitrary limit.
   
   This issue would have special importance to anyone who is dynamically creating
   the argument passed to the Page application and allowing more than 128
   extensions to be added by an outside user via some external interface.
   
   The patch posted by a_villacis was slightly modified for some coding guidelines
   and other cleanups.  Thanks, a_villacis!
   (closes issue ASTERISK-13344)
   Reported by: a_villacis
   Patches:
         20080912-asterisk-app_page-fix-buffer-overflow.patch uploaded by a (license 660)
   Tested by: otherwiseguy
 ........
................

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

http://svn.digium.com/view/asterisk?view=rev&revision=168595
By: Digium Subversion (svnbot) 2009-01-13 20:11:01.000-0600

Repository: asterisk
Revision: 168596

_U  branches/1.6.1/
U   branches/1.6.1/apps/app_page.c

------------------------------------------------------------------------
r168596 | twilson | 2009-01-13 20:11:01 -0600 (Tue, 13 Jan 2009) | 34 lines

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

................
 r168594 | twilson | 2009-01-13 20:00:40 -0600 (Tue, 13 Jan 2009) | 27 lines
 
 Merged revisions 168593 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r168593 | twilson | 2009-01-13 19:27:18 -0600 (Tue, 13 Jan 2009) | 20 lines
   
   Don't overflow when paging more than 128 extensions
   
   The number of available slots for calls in app_page was hardcoded to 128.
   Proper bounds checking was not in place to enforce this limit, so if more than
   128 extensions were passed to the Page() app, Asterisk would crash.  This patch
   instead dynamically allocates memory for the ast_dial structures and removes
   the (non-functional) arbitrary limit.
   
   This issue would have special importance to anyone who is dynamically creating
   the argument passed to the Page application and allowing more than 128
   extensions to be added by an outside user via some external interface.
   
   The patch posted by a_villacis was slightly modified for some coding guidelines
   and other cleanups.  Thanks, a_villacis!
   (closes issue ASTERISK-13344)
   Reported by: a_villacis
   Patches:
         20080912-asterisk-app_page-fix-buffer-overflow.patch uploaded by a (license 660)
   Tested by: otherwiseguy
 ........
................

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

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