[FFmpeg-devel,v2,7/8] avformat/wavdec: fix s337m/spdif probing beyond data_end

---
 libavformat/wavdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Am Mi., 15. Jan. 2020 um 11:56 Uhr schrieb Nicolas Gaullier
<nicolas.gaullier@cji.paris>:
>
> ---
>  libavformat/wavdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c
> index 3571733817..d8a27c79cf 100644
> --- a/libavformat/wavdec.c
> +++ b/libavformat/wavdec.c
> @@ -77,7 +77,7 @@ static void set_spdif_s337m(AVFormatContext *s, WAVDemuxContext *wav)
>                  ret = AVERROR(ENOMEM);
>              } else {
>                  int64_t pos = avio_tell(s->pb);
> -                len = ret = avio_read(s->pb, buf, len);
> +                len = ret = avio_read(s->pb, buf, FFMIN(len, wav->data_end - pos));

Sorry if this was already answered:
What exactly does this fix? Is it possible that avio_read() overreads without
this check?

Carl Eugen

>>
>> ---
>>  libavformat/wavdec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c index 
>> 3571733817..d8a27c79cf 100644
>> --- a/libavformat/wavdec.c
>> +++ b/libavformat/wavdec.c
>> @@ -77,7 +77,7 @@ static void set_spdif_s337m(AVFormatContext *s, WAVDemuxContext *wav)
>>                  ret = AVERROR(ENOMEM);
>>              } else {
>>                  int64_t pos = avio_tell(s->pb);
>> -                len = ret = avio_read(s->pb, buf, len);
>> +                len = ret = avio_read(s->pb, buf, FFMIN(len, 
>> + wav->data_end - pos));
>
>Sorry if this was already answered:
>What exactly does this fix? Is it possible that avio_read() overreads without this check?
>
>Carl Eugen

There is no severe low level avio overread risk, but there is a risk to read bytes that are not audio data in case audio data is followed by other data (a RIFF chunk that would come just after the audio data RIFF chunk).

Here is a sample a build : http://0x0.st/zhiO.wav This sample is detected as ac3 whereas the sync codes are beyond the data chunk, in a 'junk' (I called it this way) chunk that is not supposed to be read (ex: the duration can be cross-checked with sox or audacity for example).
But again, this is obviously very improbable "in the nature" as they are three conditions :
- the wav file must be short enough (for the probe to seek in the end of the file)
- the audio data chunk must be followed by another chunk with misc data (usually those are considered "header metadata" and stand before it)
- the misc data chunk must emulate the sync codes

Nicolas

Am Mi., 15. Jan. 2020 um 16:07 Uhr schrieb Gaullier Nicolas
<nicolas.gaullier@cji.paris>:
>
> >>
> >> ---
> >>  libavformat/wavdec.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c index
> >> 3571733817..d8a27c79cf 100644
> >> --- a/libavformat/wavdec.c
> >> +++ b/libavformat/wavdec.c
> >> @@ -77,7 +77,7 @@ static void set_spdif_s337m(AVFormatContext *s, WAVDemuxContext *wav)
> >>                  ret = AVERROR(ENOMEM);
> >>              } else {
> >>                  int64_t pos = avio_tell(s->pb);
> >> -                len = ret = avio_read(s->pb, buf, len);
> >> +                len = ret = avio_read(s->pb, buf, FFMIN(len,
> >> + wav->data_end - pos));
> >
> >Sorry if this was already answered:
> >What exactly does this fix? Is it possible that avio_read() overreads without this check?
> >
> >Carl Eugen
>
> There is no severe low level avio overread risk, but there is a risk to read bytes that are not audio data in case audio data is followed by other data (a RIFF chunk that would come just after the audio data RIFF chunk).
>
> Here is a sample a build : http://0x0.st/zhiO.wav This sample is detected as ac3 whereas the sync codes are beyond the data chunk, in a 'junk' (I called it this way) chunk that is not supposed to be read (ex: the duration can be cross-checked with sox or audacity for example).
> But again, this is obviously very improbable "in the nature" as they are three conditions :
> - the wav file must be short enough (for the probe to seek in the end of the file)
> - the audio data chunk must be followed by another chunk with misc data (usually those are considered "header metadata" and stand before it)
> - the misc data chunk must emulate the sync codes

Thank you for the explanation.

Carl Eugen