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

[Michael Niedermayer]

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
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, so do channels, and number of pixels.

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.

thx

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

The day soldiers stop bringing you their problems is the day you have stopped 
leading them. They have either lost confidence that you can help or concluded 
you do not care. Either case is a failure of leadership. - Colin Powell
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240627/ee1b5984/attachment.sig>