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

[Michael Niedermayer <michael@niedermayer.cc>]

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

On Thu, Jun 27, 2024 at 08:53:09AM +0200, Anton Khirnov wrote:
> 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@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.

random pick, man ccache --max-files=N --max-size=SIZE


> And they are runtime-configurable.

I would favor runtime-configurable as well


> 
> > so do channels, and number of pixels.
> 
> Does not follow.

For a file with length n "bits" it is understood and expected that there will be O(n)
processing needed. having O(2^n) is not "expected"
if you have a loop going over channels, pixels, or another "unlimited" property
you can create very small files that require the maximum memory and processing
allowed by the OS for the process

Thats a DOS attack


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

ive rerun the testcase and depending on settings it either hits
OOM in av_channel_layout_custom_init() or it will timeout in

av_channel_layout_describe_bprint libavutil/channel_layout.c:627:17

The file has 33 million channels.
Now if for some reason none of the above would hit a timeout and
instead the decoder would decode 33 million channels of silence
maybe because it had a 1 byte packet and error concealment produced
a silent packet to fill the space. Still the same issue exists
only a limit on the channel number avoids this.

Surely improving all the code that is slow with millions of channels
is a good idea but even the most efficient code to produce a frame with
arbitrary # million channels in it will lead to a DOS attack opertunity

thx

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

Whats the most studid thing your enemy could do ? Blow himself up
Whats the most studid thing you could do ? Give up your rights and
freedom because your enemy blew himself up.


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