Summary:	ASTERISK-09924: socket_read() does not properly check for recvfrom() error
Reporter:	timrobbins (timrobbins)	Labels:
Date Opened:	2007-07-21 22:09:07	Date Closed:	2007-07-23 07:12:38
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_iax2
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 10265.patch.txt
Description:	socket_read() in chan_iax2.c stores the result of recvfrom() in a variable with an unsigned type (size_t). It then attempts to check for errors by testing for a negative value, but this cannot ever work. It looks like this could be used to cause a crash: if another thread is already processing a full frame for the same call as the one the current thread last processed, defer_full_frame() will be called and will try to make a copy of the current thread's receive buffer onto the heap. Due to integer wraparound it will allocate sizeof(*pkt_buf) - 1 bytes and attempt to copy SIZE_MAX bytes into that area. In the other case (frame does not need to be deferred), socket_process() will detect a "midget" frame. Suggested fix is to store the result of recvfrom() in a variable of type ssize_t until after the check for receive errors. This bug was found with the Intel C/C++ Compiler version 10.0: chan_iax2.c(6374): warning ASTERISK-183: pointless comparison of unsigned integer with zero        if (thread->buf_len < 0) {
Comments:	By: Russell Bryant (russell) 2007-07-22 16:45:41 I am going to mark this as private as I work on verifying the extent of this issue. By: Russell Bryant (russell) 2007-07-22 17:05:03 I agree with your suggested fix.  I have uploaded a patch to reflect the exact change. By: Russell Bryant (russell) 2007-07-23 07:04:32 I am making this issue public again.  I don't consider it a security issue as I do not think there is a way to remotely cause recvfrom() to return an error. By: Digium Subversion (svnbot) 2007-07-23 07:07:55 Repository: asterisk Revision: 76485 ------------------------------------------------------------------------ r76485 | russell | 2007-07-23 07:07:54 -0500 (Mon, 23 Jul 2007) | 6 lines Use a signed integer for storing the number of bytes in the packet read from the network.  Using an unsigned value here made it impossible to handle an error returned from recvfrom().  Furthermore, in the case that recvfrom() did return an error, this would cause a crash due to a heap overflow. (closes issue ASTERISK-9924, reported by and fix suggested by timrobbins) ------------------------------------------------------------------------ By: Digium Subversion (svnbot) 2007-07-23 07:12:38 Repository: asterisk Revision: 76486 ------------------------------------------------------------------------ r76486 | russell | 2007-07-23 07:12:37 -0500 (Mon, 23 Jul 2007) | 14 lines Merged revisions 76485 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r76485 | russell | 2007-07-23 07:25:01 -0500 (Mon, 23 Jul 2007) | 6 lines Use a signed integer for storing the number of bytes in the packet read from the network.  Using an unsigned value here made it impossible to handle an error returned from recvfrom().  Furthermore, in the case that recvfrom() did return an error, this would cause a crash due to a heap overflow. (closes issue ASTERISK-9924, reported by and fix suggested by timrobbins) ........ ------------------------------------------------------------------------