Summary:	ASTERISK-29328: translate.c: possible buffer overflow when upsampling
Reporter:	Jean Aunis - Prescom (PrescomJA)	Labels:	patch
Date Opened:	2021-03-04 07:08:27.000-0600	Date Closed:	2021-04-28 16:34:22
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/CodecInterface
Versions:	16.16.0	Frequency of Occurrence	Constant
Related Issues:
Environment:		Attachments:	( 0) fix_translation_size.patch
Description:	I may have found a buffer size miscalculation in translate.c. It may have security implications since it could result in a buffer overflow. There is a piece of code in translate.c (function framein) that checks if the translator has got enough free space in its outbuf. Here is the code: {code} static int framein(struct ast_trans_pvt *pvt, struct ast_frame *f) { [snip]        if (pvt->samples + f->samples > pvt->t->buffer_samples) {            ast_log(LOG_WARNING, "Out of buffer space\n");            return -1;        } } {code} It seems to me this code assumes that the number of samples remains the same through the translation process. Which will not be the case when up- or down-sampling. When upsampling, it may overflow the outbuf. Shouldn't we re-write the condition like this: {code} int src_srate = pvt->t->src_codec->sample_rate; int dst_srate = pvt->t->dst_codec->sample_rate; if (pvt->samples + (f->samples * dst_srate/src_srate) > pvt->t->buffer_samples) {      ast_log(LOG_WARNING, "Out of buffer space\n"); return -1; } {code} For the moment I have not been able to create the conditions of a crash.
Comments:	By: Asterisk Team (asteriskteam) 2021-03-04 07:08:29.650-0600 This issue has been automatically restricted and set to a blocker due to being a security type issue. If this is not a security vulnerability issue it will be moved to the appropriate issue type when triaged. Please DO NOT put a code review up for this change at this time. Attach any applicable patches to this issue. By: Asterisk Team (asteriskteam) 2021-03-04 07:08:30.250-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. Please note that log messages and other files should not be sent to the Sangoma Asterisk Team unless explicitly asked for. All files should be placed on this issue in a sanitized fashion as needed. 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]. Please note that once your issue enters an open state it has been accepted. As Asterisk is an open source project there is no guarantee or timeframe on when your issue will be looked into. If you need expedient resolution you will need to find and pay a suitable developer. Asking for an update on your issue will not yield any progress on it and will not result in a response. All updates are posted to the issue when they occur. Please note that by submitting data, code, or documentation to Sangoma through JIRA, you accept the Terms of Use present at [https://www.asterisk.org/terms-of-use/|https://www.asterisk.org/terms-of-use/]. By: Joshua C. Colp (jcolp) 2021-03-04 07:26:10.475-0600 The codec_resample module doesn't appear as though it relies on the given check. The codec_g722 module does upsample internally, so it may be suspectible. Preliminary investigation though I think it isn't possible. You'd need to have a frame of a large size, or a chain of a ton of frames that are translated all together. For media from outside this doesn't happen. You translate a frame at a time and we're limited by UDP generally. The codec modules also use large buffers. File playback is also a frame at a time, and appears to have a fixed small buffer size there for g722. By: Jean Aunis - Prescom (PrescomJA) 2021-03-04 07:36:48.124-0600 It may be an issue for this open-source Opus codec: https://github.com/lminiero/asterisk-opus/blob/asterisk-13.7/codecs/codec_opus_open_source.c That's just for information, I don't hold you responsible for the code you didn't write! By: Joshua C. Colp (jcolp) 2021-03-04 07:40:07.950-0600 I can't comment or look at the code, so can't say anything. By: Jean Aunis - Prescom (PrescomJA) 2021-03-17 08:08:34.049-0500 Please find attached a patch for this issue. By: Kevin Harwell (kharwell) 2021-04-06 17:47:19.937-0500 [~PrescomJA] Thanks for the prudent reporting of this issue. It's always better to be safe. After some investigation I am not seeing a way for someone to exploit this potential problem with current core Asterisk codec modules. Several of the codec modules have their own buffer overflow checks, and those that don't appear to calculate a one-to-one frame to buffer samples (even codec_g722 for the most part). As well In Asterisk each frame is read in (framein), translated and then usually written out (frameout). The internal buffer is then cleared for the next frame. Also, MTU puts a cap on the maximum frame size, which should be smaller than most if not all codec buffer sizes. So currently it does not appear there is a way to externally force a buffer overflow during translation. At this time we feel like this issue is not a security issue, however before making it public what are your thoughts with regards to this? Have you found a way to force a crash? Or feel there might still be a security concern? Thanks! By: Kevin Harwell (kharwell) 2021-04-06 17:48:14.493-0500 Oh and to be clear I'd still consider this issue a bug, and the patch acceptable for inclusion. While the current code may be working okay it could be something was 1) missed, 2) a future codec module falls prey, or 3) external community codec modules are susceptible. Your patch also makes it more clear what should be happening. By: Jean Aunis - Prescom (PrescomJA) 2021-04-12 02:38:22.798-0500 I could not create the conditions of a crash. However, I encountered an issue linked to this part of the code when using the above-mentioned community Opus codec: when down-sampling 60ms frames from opus (48000) to slin (8000), Asterisk complained about missing buffer space. The issue disappeared when applying the patch. So this is actually the exact opposite of the case which would lead to a crash (since we are down-sampling instead of up-sampling), but the cause is the same: the buffer size is miscalculated due to different sampling frequencies. I suppose it is possible to make Asterisk crash with this particular community codec, but I can't prove it at the moment. If you let me another few days, I think I can devise the appropriate test scenario. Still, for my particular use case, I would be OK with making the issue public because the conditions for a crash will not exist in our production systems. But this may not be the case for other community members. By: Kevin Harwell (kharwell) 2021-04-12 10:27:09.864-0500 Sounds good and not a problem. Just let us know if/when you might have something. By: Jean Aunis - Prescom (PrescomJA) 2021-04-20 05:17:13.078-0500 Still can't produce a crash after more tests. It looks like the codec's default buffer is large enough to handle most cases, and anyway it is extremely difficult to make Asterisk setup a translation path that does *not* use codec_resample. So I think you can make the issue public. By: Kevin Harwell (kharwell) 2021-04-20 11:55:54.428-0500 [~PrescomJA] Thanks for the testing and feedback. Since you already have a patch is this something you'd like to [push up|https://wiki.asterisk.org/wiki/display/AST/Code+Review] to [gerrit|https://gerrit.asterisk.org/] yourself? By: Jean Aunis - Prescom (PrescomJA) 2021-04-21 01:19:11.939-0500 All right, I'll do this today. By: Kevin Harwell (kharwell) 2021-04-21 09:40:01.005-0500 Excellent and thank you! By: Friendly Automation (friendly-automation) 2021-04-28 16:34:23.168-0500 Change 15801 merged by Friendly Automation: translate.c: Take sampling rate into account when checking codec's buffer size [https://gerrit.asterisk.org/c/asterisk/+/15801|https://gerrit.asterisk.org/c/asterisk/+/15801] By: Friendly Automation (friendly-automation) 2021-04-28 16:34:32.598-0500 Change 15832 merged by Friendly Automation: translate.c: Take sampling rate into account when checking codec's buffer size [https://gerrit.asterisk.org/c/asterisk/+/15832|https://gerrit.asterisk.org/c/asterisk/+/15832] By: Friendly Automation (friendly-automation) 2021-04-28 16:35:58.885-0500 Change 15833 merged by Friendly Automation: translate.c: Take sampling rate into account when checking codec's buffer size [https://gerrit.asterisk.org/c/asterisk/+/15833|https://gerrit.asterisk.org/c/asterisk/+/15833]