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

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

2016-11-14 21:55 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>:

>> 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.

I don't know the exact "broken" case you are referring to but
generally, FFmpeg should not reject files because a field in
their header is set incorrectly, especially if such "broken"
files were played in the past.

Carl Eugen

On Mon, Nov 14, 2016 at 9:55 PM, Andreas Cadhalpun
<andreas.cadhalpun@googlemail.com> 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.
>

Well, not necessarily. Just because the container info is wrong or
missing does not mean the stream is undecodable - not all containers
have such levels of info after all, or sometimes none (see mpegts).
Compressed codecs are often designed to be independent of container info.

- Hendrik

On 14.11.2016 22:59, Carl Eugen Hoyos wrote:
> 2016-11-14 21:55 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>:
> 
>>> 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.
> 
> I don't know the exact "broken" case you are referring to but
> generally, FFmpeg should not reject files because a field in
> their header is set incorrectly, especially if such "broken"
> files were played in the past.

Well, if the field should contain the number of channels but doesn't,
the sample is not correct.
Anyway, zero channels is borderline, but what about a negative number
of channels?

Best regards,
Andreas

2016-11-14 23:40 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>:
> On 14.11.2016 22:59, Carl Eugen Hoyos wrote:
>> 2016-11-14 21:55 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>:
>>
>>>> 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.
>>
>> I don't know the exact "broken" case you are referring to but
>> generally, FFmpeg should not reject files because a field in
>> their header is set incorrectly, especially if such "broken"
>> files were played in the past.
>
> Well, if the field should contain the number of channels but doesn't,
> the sample is not correct.

Assuming the opus bitstream contains the number of channels,
the value in the demuxer should be ignored by default.

Carl Eugen

On 14.11.2016 21:55, Andreas Cadhalpun wrote:
> From d33ded293d15e8ceab666bea834d436f3a225bcc Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun@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@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;

Ping. It would be good to have this fixed in 3.2.1.

Best regards,
Andreas

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@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@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

[...]