Summary:	ASTERISK-10919: Buffer overflow when maxmsg not used for IMAP storage users
Reporter:	James Rothenberger (jaroth)	Labels:
Date Opened:	2007-11-28 14:48:51.000-0600	Date Closed:	2007-12-06 15:00:45.000-0600
Priority:	Critical	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_voicemail
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) allocation.patch ( 1) maxmsg_imap.patch
Description:	When users do not have maxmsg set individually, or when maxmsg is not set, memory allocation and deallocation of the "deleted" and "heard" arrays can cause a crash.    This patch uses MAXMSG to size these arrays when using IMAP.  This guarantees that the array will always be big enough to hold all messages that might be accessed.  This is even more important when using IMAP quotas instead of message count to limit the number of messages in a box, and when mailboxes can be accessed directly from an IMAP mail client.
Comments:	By: Mark Michelson (mmichelson) 2007-11-28 17:34:07.000-0600 I think this is somewhat related to issue 11101, but I might be off. The issue there is that more than 256 messages will cause a crash due to a buffer overflow of the msgArray. The reason I see these as related is that I think the patch I have provided may also fix this issue, too. The main problem I have with maxmsg_imap.patch is that it doesn't actually improve on what the code does for the case you have stated in the description. The reason is that vmu->maxmsg will always be set to MAXMSG if a maxmsg is not specified in voicemail.conf. The patch is problematic in the case where a user *has* defined a maxmsg, especially if maxmsg is greater than MAXMSG. Hard coding the allocation of the deleted array to MAXMSG is error-prone since the msgArray parameter is sized at 256. This means that you can only delete or hear the first 100 messages in a mailbox. If you attempt to hear or delete any message beyond the 100th, you will crash Asterisk since you will be accessing out-of-bounds memory. By: James Rothenberger (jaroth) 2007-11-29 11:28:11.000-0600 I see what you mean.  If nothing else, maybe you could use the allocation error detection part of this patch, since there were "TODO" comments in the code to provide some sort of error handling there.   I just submitted another patch (11415) that addresses handling max messages in IMAP as well as some other IMAP quota changes.   Maybe that will help as well.  If you want me to just resubmit the error checking part of this patch I can do that as well. By: Mark Michelson (mmichelson) 2007-12-06 13:10:35.000-0600 I agree that the handling of the allocation error is an excellent idea. If you don't mind, yes, please just resubmit that portion of the patch. By: James Rothenberger (jaroth) 2007-12-06 14:48:11.000-0600 Modification as indicated. By: Mark Michelson (mmichelson) 2007-12-06 14:53:54.000-0600 Great! Thanks very much! By: Digium Subversion (svnbot) 2007-12-06 15:00:45.000-0600 Repository: asterisk Revision: 91579 U   trunk/apps/app_voicemail.c ------------------------------------------------------------------------ r91579 | mmichelson | 2007-12-06 15:00:44 -0600 (Thu, 06 Dec 2007) | 5 lines Handle allocation failure of the heard and deleted arrays of the vm_state. (closes issue ASTERISK-10919, reported and patched by jaroth) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=91579