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

[James Almer <jamrial@gmail.com>]

On 7/4/2022 1:55 PM, Michael Niedermayer wrote:
> On Sun, Jul 03, 2022 at 09:27:31PM -0300, James Almer wrote:
>> On 7/3/2022 7:00 AM, Nicolas George wrote:
>>> 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 Matroska demuxer could in theory generate a native layout with more than
>> 64 channels where popcnt(mask) != channels, and nothing seems to validate
>> what a demuxer's read_header() callback returns if you just call lavf API
>> functions like target_dem_fuzzer.c seems to do.
>>
>> Maybe:
>>
>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>> index 73ded761fd..ad7ee390a2 100644
>>> --- a/libavformat/matroskadec.c
>>> +++ b/libavformat/matroskadec.c
>>> @@ -2950,10 +2950,10 @@ static int matroska_parse_tracks(AVFormatContext *s)
>>>               st->codecpar->codec_tag   = fourcc;
>>>               st->codecpar->sample_rate = track->audio.out_samplerate;
>>>               // channel layout may be already set by codec private checks above
>>> -            if (st->codecpar->ch_layout.order == AV_CHANNEL_ORDER_NATIVE &&
>>> -                !st->codecpar->ch_layout.u.mask)
>>> +            if (!av_channel_layout_check(&st->codecpar->ch_layout)) {
>>>                   st->codecpar->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC;
>>> -            st->codecpar->ch_layout.nb_channels = track->audio.channels;
>>> +                st->codecpar->ch_layout.nb_channels = track->audio.channels;
>>> +            }
>>>               if (!st->codecpar->bits_per_coded_sample)
>>>                   st->codecpar->bits_per_coded_sample = track->audio.bitdepth;
>>>               if (st->codecpar->codec_id == AV_CODEC_ID_MP3 ||
>>
>> is enough to ensure a valid layout is propagated. This assumes parameters
>> set by codec private parsing are correct (only FLAC seem to be present right
>> now, and it is).
>>
>> Assuming i got this right, in this fuzzing sample's case it would still have
>> a billion channels (since that's what track->audio.channels contains, as
>> read from the container), but using the unspec layout, which is technically
>> valid even if nothing will really handle it, and
>> av_channel_layout_describe() will print a small string.
> 
> seems this is fixing it
> thanks

Ok, will apply it and revert 8154cb7c2f.

> 
> 
>>
>> Still, i think a check in avformat_open_input() might also be a good idea,
> 
> yes, i agree
> 
> 
>> especially once (and if) demuxers start propagating custom layouts, where
>> the map array will be allocated.
> 
> [...]
> 
> 
> _______________________________________________
> 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".
_______________________________________________
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".