Summary:	ASTERISK-25603: [patch]udptl: Uninitialized lengths and bufs in udptl_rx_packet cause ast_frdup crash
Reporter:	Walter Doekes (wdoekes)	Labels:
Date Opened:	2015-12-02 10:35:21.000-0600	Date Closed:	2016-02-03 15:29:43.000-0600
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Core/UDPTL
Versions:	11.20.0 13.6.0	Frequency of Occurrence
Related Issues:	causes ASTERISK-17460 Crash in ast_frdup causes ASTERISK-17649 crash in ast_frdup with oversized udptl frame causes ASTERISK-21041 Asterisk crashes during a frame copy while receiving a fax is duplicated by ASTERISK-25742 Secondary IFP Packets can result in accessing uninitialized pointers and a crash is related to ASTERISK-19762 Segfault in ast_frdup when invalid data length specified in duplicated frame
Environment:		Attachments:	( 0) ASTERISK-25603_fix_udptl_crash.patch ( 1) ASTERISK-25603_fix_udptl_crash-2.patch
Description:	UDPTL frames get decoded by udptl_rx_packet. If no packets are lost, the subpacket IFP length is read into ifp_len, which is initialized to 0. The first frame in a list of frames is filled with the T38 data, it is returned to ast_udptl_read, and it ends up in ast_read() (channel.c). However, if the sequence number does a jump, Asterisk tries to use the error correcting redundancy packets available. There the IFP length nor the buffer are initialized. And if the supplied IFP length is 0, it is not stored: those uninitialized values end up in the frame {{data}} and {{datalen}} attributes. Serving UDPTL packets with the right amount of missing sequence numbers and enough redundant 0-length IFP packets, will make Asterisk enqueue the excess redundant frames through ast_queue_frame(). There, an ast_frdup() will attempt to read the message: datalen could be huge or data may point outside the process address space. In either case, Asterisk crashes. This has been broken since the introduction of the UDPTL code in 2006 (0f5e4e47). It has been mitigated somewhat by 3f789aa8 in 2012, but that apparently wasn't enough. According to my tests, if can you force Asterisk to read UDPTL, if even for bridging only, the right pcap will make it crash with absolute certainty. This was tested against 11.19, after reports were received that an Asterisk 1.4 was crashing on this same issue. Fix is attached. Cheers, Walter Doekes OSSO B.V.
Comments:	By: Asterisk Team (asteriskteam) 2015-12-02 10:35:22.919-0600 Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution. A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report. Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process]. By: Richard Mudgett (rmudgett) 2015-12-07 13:06:07.919-0600 Looking at the code in v11 the patch should also set {{*p_object = NULL}} in addition to {{*p_num_octets = 0}}.  If you were running under valgrind I expect it would be complaining of conditional move or jump dependent on uninitialized value in {{udptl_rx_packet()}} for {{bufs\[\]}} in addition to the {{lengths\[\]}} the patch initializes. By: Walter Doekes (wdoekes) 2015-12-07 13:58:33.537-0600 Well, in the case that I was looking at, the {{if (!bufs\[total_count + i] || !lengths\[total_count + i])}} check drops the entries with length zero, so not setting {{*p_object}} isn't a problem. As far as I can tell, it wouldn't be in the FEC error recovery bit either, since {{data}} is only used for memcpy, which doesn't do any reading if length is 0. But I do agree that it's ugly not to set it. Since {{octet_cnt}} is unsigned, dropping the {{if (octet_cnt > 0)}} works well and makes for cleaner code anyway; see patch-v2. By: Michael Spiceland (mspiceland) 2015-12-16 16:34:45.408-0600 Walter, thank you very much for the help and your time working with us on this issue. I'm following up to let you know that we're planning on doing the security release in January, after the Asterisk 13.7 release. I'll be able to give a more specific date closer to the actual date. By: Michael Spiceland (mspiceland) 2016-01-20 13:53:36.874-0600 Walter, thanks again for the help in reporting this.  I wanted to post a quick update to let you know that due to some internal coordination, it now looks like this will not happen in January.  We'll post updates as we know more.  Thanks for your patience.