Summary: | ASTERISK-28076: bridging: Asterisk crashes when receiving an empty realtime text frame | ||
Reporter: | Emmanuel BUU (neutrino88) | Labels: | |
Date Opened: | 2018-09-25 15:40:02 | Date Closed: | 2018-10-03 08:41:34 |
Priority: | Minor | Regression? | No |
Status: | Closed/Complete | Components: | Core/Bridging |
Versions: | 13.22.0 | Frequency of Occurrence | Constant |
Related Issues: | |||
Environment: | CentOS 7 but this bug is OS independend | Attachments: | |
Description: | When receiving an RTP packet containing an empty redundant realtime text frame, asterisk 13.22.0 crashes. | ||
Comments: | By: Asterisk Team (asteriskteam) 2018-09-25 15:40:04.439-0500 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: Emmanuel BUU (neutrino88) 2018-09-25 15:40:54.054-0500 Will submit a patch By: Richard Mudgett (rmudgett) 2018-09-27 15:25:11.415-0500 Could you attach a backtrace so we know where it crashed? By: Emmanuel BUU (neutrino88) 2018-09-28 03:16:46.922-0500 It will take a bit of time. In the meantime, here is an explanation: 1- the frame text frame has fr->datalen set to 0. 2- it is passed to ast_bridge_channel_queue_frame() 3- inside the ast_bridge_channel_queue_frame() function, it is cloned dup = ast_frdup(fr); the result is a duplicated frame with datalen set to 0 and an invalid fr->data.ptr pointer. 4- later, the function bridge_channel_handle_write() with the duplicated frame as an argument There are two lines where the invalid pointer can corrupt memory / cause a crash the call to ast_debug() inside the ast_sendtext() case AST_FRAME_TEXT: ast_debug(1, "Sending TEXT frame to '%s': %*.s\n", ast_channel_name(bridge_channel->chan), fr->datalen, (char *)fr->data.ptr); ast_sendtext(bridge_channel->chan, fr->data.ptr); By: Corey Farrell (coreyfarrell) 2018-09-28 06:26:04.109-0500 Could this issue be fixed from ast_frdup? It currently sets {{out->data.ptr}} only {{if (out->datalen)}}, otherwise it sets {{out->data.uint32}}. out->data is a union so this is modifying out->data.ptr as well. Maybe this should initialize the pointer {{if (out->datalen || out->frametype == AST_FRAME_TEXT)}}? I think it would be better to ensure that {{struct ast_frame}} is valid instead of trying to ignore invalid structures where. I have not tested this suggested change but wanted to make sure this is considered. I also have not checked if other frame types also use out->data.ptr with a potentially zero out->datalen. I just did a crude scan of asterisk using {{git grep \-e '\->datalen' \-\-and \-e '\->data\.ptr'|wc \-l}} - 101 lines in master which in theory could be at risk. By: Emmanuel BUU (neutrino88) 2018-09-28 07:23:14.778-0500 IMHO no. out->data.uint32 could be used for other cases (mostly AST_CONTROL frames). The only thing we know it that text frames with datalen set to 0 should be processed differently. I beileive this can be extended to other media (audio and video). This could be enforced in ast_write(). By: Richard Mudgett (rmudgett) 2018-09-28 07:35:37.726-0500 I think [~coreyfarrell]'s suggestion makes more sense. By: Emmanuel BUU (neutrino88) 2018-09-28 07:55:45.210-0500 So do you want me to amend the patch to implement this suggestion? By: Corey Farrell (coreyfarrell) 2018-09-28 08:01:05.178-0500 Could you test my suggestion as a replacement to your patch? Once {{ast_frdup}} stops creating frames with invalid dataptr it should be unnecessary to change {{main/bridge_channel.c}}. I think the change to bridge_channel.c might actually be undesirable. In theory someone could use a zero length text frame as a 'ping' of sorts so we should forward it. By: Richard Mudgett (rmudgett) 2018-09-28 08:02:32.913-0500 Yes. That was one reason I wanted to see the backtrace to see why it crashed. Also ast_frisolate() appears to do the same thing with fr->data.ptr when fr->datalen == 0. By: Emmanuel BUU (neutrino88) 2018-10-01 10:52:49.404-0500 Posted a new patch. By: Emmanuel BUU (neutrino88) 2018-10-03 08:41:35.077-0500 Fix has been committed and ported to branches 15 and 16 By: Joshua C. Colp (jcolp) 2018-10-03 08:44:38.398-0500 In the future there is no need to close the issue. When it is merged this is automatically closed and the reviews added as comments. By: Friendly Automation (friendly-automation) 2018-10-03 13:43:52.958-0500 Change 10369 merged by George Joseph: core/frame: Fix ast_frdup() and ast_frisolate() for empty text frames [https://gerrit.asterisk.org/10369|https://gerrit.asterisk.org/10369] By: Friendly Automation (friendly-automation) 2018-10-03 13:56:12.150-0500 Change 10371 merged by George Joseph: core/frame: Fix ast_frdup() and ast_frisolate() for empty text frames [https://gerrit.asterisk.org/10371|https://gerrit.asterisk.org/10371] By: Friendly Automation (friendly-automation) 2018-10-03 13:56:29.812-0500 Change 10269 merged by George Joseph: core/frame: Fix ast_frdup() and ast_frisolate() for empty text frames [https://gerrit.asterisk.org/10269|https://gerrit.asterisk.org/10269] By: Friendly Automation (friendly-automation) 2018-10-03 13:57:55.702-0500 Change 10370 merged by George Joseph: core/frame: Fix ast_frdup() and ast_frisolate() for empty text frames [https://gerrit.asterisk.org/10370|https://gerrit.asterisk.org/10370] |