[FFmpeg-devel] [libav-devel] [PATCH] libopusdec: fix out-of-bounds read

Michael Niedermayer michael at niedermayer.cc
Wed Nov 23 04:07:29 EET 2016


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.Cadhalpun at 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.Cadhalpun at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161123/394e8f77/attachment.sig>


More information about the ffmpeg-devel mailing list