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

[Michael Niedermayer <michael@niedermayer.cc>]

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

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


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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 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".