[FFmpeg-devel] [PATCH 1/3] avformat/cafdec: sanity check channels and bps

[Anton Khirnov]

Quoting Michael Niedermayer (2024-06-27 01:50:12)
> On Tue, Jun 25, 2024 at 09:27:55PM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2024-06-25 21:25:46)
> > > On Thu, Mar 28, 2024 at 12:27:02AM +0100, Michael Niedermayer wrote:
> > > > On Wed, Mar 27, 2024 at 08:39:17AM +0100, Anton Khirnov wrote:
> > > > > Quoting Michael Niedermayer (2024-03-23 00:08:16)
> > > > > > Fixes: Timeout
> > > > > > Fixes: 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> > > > > > 
> > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > > > ---
> > > > > >  libavformat/cafdec.c | 5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > > 
> > > > > > diff --git a/libavformat/cafdec.c b/libavformat/cafdec.c
> > > > > > index 426c56b9bd..334077efb5 100644
> > > > > > --- a/libavformat/cafdec.c
> > > > > > +++ b/libavformat/cafdec.c
> > > > > > @@ -33,6 +33,7 @@
> > > > > >  #include "isom.h"
> > > > > >  #include "mov_chan.h"
> > > > > >  #include "libavcodec/flac.h"
> > > > > > +#include "libavcodec/internal.h"
> > > > > >  #include "libavutil/intreadwrite.h"
> > > > > >  #include "libavutil/intfloat.h"
> > > > > >  #include "libavutil/dict.h"
> > > > > > @@ -87,6 +88,10 @@ static int read_desc_chunk(AVFormatContext *s)
> > > > > >      st->codecpar->ch_layout.nb_channels = avio_rb32(pb);
> > > > > >      st->codecpar->bits_per_coded_sample = avio_rb32(pb);
> > > > > >  
> > > > > > +    if (st->codecpar->ch_layout.nb_channels > FF_SANE_NB_CHANNELS ||
> > > > > 
> > > > > I dislike this.
> > > > 
> > > > I dislike it too
> > > 
> > > so what do we do about this ?
> > 
> > About what? What is the actual problem that needs addressed?
> 
> 67044/clusterfuzz-testcase-minimized-ffmpeg_dem_CAF_fuzzer-5791144363491328
> 
> 
> > 
> > > any objections to apply this ?
> > 
> > yes, FF_SANE_NB_CHANNELS is a hack that should be removed, not spread
> 
> a maximum number for each theoretically unlimited parameter is desirable

I disagree.

> This can be a user setable value or a compile time value when such is preferred.
> 
> We can take this to the TC if you want.
> 
> The same way as the number of files or number of bytes used by some cache
> needs limits,

Those limits are implemented at the OS level, not by individual
programs. And they are runtime-configurable.

> so do channels, and number of pixels.

Does not follow.

> One can remove them but users and companies concious about security and
> efficiency with untrusted input in an (semi-) automated environment will likely
> choose the codebase providing such features.

[citation needed]

My main objection to this approach is that you're addressing a symptom
(fuzzer timeout) rather than the actual problem (some code scaling
inappropriately with input size), and it's completely unclear where that
actual problem even is, as cafdec does not seem to be doing anything
with the channel count. That suggests the problem is really in some
other code and you're just papering over it.

-- 
Anton Khirnov