Re: [FFmpeg-devel] [PATCH] avutil/channel_layout: don't error out on truncated strings

[Nicolas George <george@nsup.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2507 bytes --]

Andreas Rheinhardt (12022-07-03):
> >              if (!av_bprint_is_complete(bp))
> > -                return AVERROR(ENOMEM);
> > +                break;
> >  

> Isn't this actually still against the API? av_channel_layout_describe()
> will not return the correct number of bytes necessary to write the
> string for the channel layout.

You are both right.

BPrint-based APIs are not supposed to check for truncation, because
printing into a bounded buffer to determine the necessary size is a
valid use (see AV_BPRINT_SIZE_COUNT_ONLY).

What is wrong is Michael's original fix:

>> commit 8154cb7c2ff2afcb1a0842de8c215b7714c814d0
>> Author: Michael Niedermayer <michael@niedermayer.cc>
>> Date:   2022-06-30 00:00:32 +0200
>> 
>>     avutil/channel_layout: av_channel_layout_describe_bprint: Check for buffer end
>> 
>>     Fixes: Timeout printing a billion channels
>>     Fixes: 48099/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-6754782204788736
>> 
>>     Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>     Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> 
>> diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
>> index 21b70173b7..1887987789 100644
>> --- a/libavutil/channel_layout.c
>> +++ b/libavutil/channel_layout.c
>> @@ -757,6 +757,10 @@ int av_channel_layout_describe_bprint(const AVChannelLayout *channel_layout,
>>              if (channel_layout->order == AV_CHANNEL_ORDER_CUSTOM &&

>>                  channel_layout->u.map[i].name[0])

If the channel count is insanely high, then this will cause invalid
reads.

>>                  av_bprintf(bp, "@%s", channel_layout->u.map[i].name);
>> +
>> +            if (!av_bprint_is_complete(bp))
>> +                return AVERROR(ENOMEM);
>> +
>>          }
>>          if (channel_layout->nb_channels) {
>>              av_bprintf(bp, ")");

Obviously, this fuzzer found a case where a demuxer or a decoder
constructs an invalid channel layout in memory without proper
validation. There is a bug, possibly an security-related one, and this
only hides it from the test suite.

The proper fix is to find where the input is improperly validated, and
then revert this change.

I do not know how to exploit the
"48099/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-6754782204788736"
information to reproduce the bug and investigate.

Regards,

-- 
  Nicolas George

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".