On Mon, Nov 14, 2016 at 09:55:15PM +0100, Andreas Cadhalpun wrote: > 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
> libopusdec.c | 6 ++++++
> 1 file changed, 6 insertions(+)
> 0b663c14f4a6dae3e1da453239dbe429aef7886e
> 0001-libopusdec-default-to-stereo-for-invalid-number-of-c.patch
> 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;
> + }
This looks wrong
opusdec uses ff_opus_parse_extradata() to set the number of channels
from extradata.
The value provided by the demuxer if any should not matter
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
