[Home]

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:02Date Closed:2018-10-03 08:41:34
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/Bridging
Versions:13.22.0 Frequency of
Occurrence
Constant
Related
Issues:
Environment:CentOS 7 but this bug is OS independendAttachments:
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]