On 14.11.2016 20:54, Anton Khirnov wrote:
> Quoting Andreas Cadhalpun (2016-11-14 20:30:10)
>> On 14.11.2016 00:01, Luca Barbato wrote:
>>> On 13/11/2016 19:23, Andreas Cadhalpun wrote:
>>>> avc->channels can be 0.
>>>
>>> 0 and less than zero shouldn't be an error?
>>
>> Such values should be rejected, wherever they are set.
>> However, ensuring that is a larger change I'm currently
>> working on.
>> Meanwhile, this patch is a trivial fix for the potential
>> security problem that can easily be backported.
>
> channels being zero is perfectly valid, it means the caller does not
> know the channel count and expects the decoder to read it from the
> bitstream.
In general code this is correct, however if e.g. the matroska demuxer
reads an audio stream which claims to have 0 channels, it should
be rejected as broken.
> This should fail for codecs that do not store this
> information in the bitstream, but work fine otherwise.
>
> In the case of opus, the channel count is always known -- when the
> extradata is present, the channel count is stored there. Otherwise the
> stream is simple and can be decoded either as mono or stereo, as we
> want.
>
> The patch does not seem to be doing the right thing -- I think it will
> simply fail on the opus_multistream_decoder_create() call.
Correct.
> What it should do instead is just default to stereo.
OK, patch doing that is attached.
> Even better, you could replace the whole extradata parsing block with
> a call to ff_opus_parse_extradata(), though that would require some
> refactoring.
The fix should be easily backportable, which excludes refactoring.
Best regards,
Andreas
>From d33ded293d15e8ceab666bea834d436f3a225bcc Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
Date: Mon, 14 Nov 2016 21:41:45 +0100
Subject: [PATCH] libopusdec: default to stereo for invalid number of channels
This fixes an out-of-bounds read if avc->channels is 0.
Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
---
libavcodec/libopusdec.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/libavcodec/libopusdec.c b/libavcodec/libopusdec.c
index acc62f1..61f68ed 100644
--- a/libavcodec/libopusdec.c
+++ b/libavcodec/libopusdec.c
@@ -47,6 +47,12 @@ static av_cold int libopus_decode_init(AVCodecContext *avc)
int ret, channel_map = 0, gain_db = 0, nb_streams, nb_coupled;
uint8_t mapping_arr[8] = { 0, 1 }, *mapping;
+ if (avc->channels <= 0) {
+ av_log(avc, AV_LOG_WARNING,
+ "Invalid number of channels %d, defaulting to stereo\n", avc->channels);
+ avc->channels = 2;
+ }
+
avc->sample_rate = 48000;
avc->sample_fmt = avc->request_sample_fmt == AV_SAMPLE_FMT_FLT ?
AV_SAMPLE_FMT_FLT : AV_SAMPLE_FMT_S16;
--
2.10.2
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
