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

[Michael Niedermayer]

On Mon, Jul 01, 2024 at 09:01:28PM -0300, James Almer wrote:
> On 7/1/2024 8:42 PM, Michael Niedermayer wrote:
> > On Sun, Jun 30, 2024 at 08:07:28PM -0300, James Almer wrote:
> > > On 6/29/2024 8:37 PM, Michael Niedermayer wrote:
> > > > On Wed, Jun 26, 2024 at 09:52:44PM -0300, James Almer wrote:
> > > > > On 3/22/2024 8:08 PM, Michael Niedermayer wrote:
> > > > > > 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 ||
> > > > > > +        st->codecpar->bits_per_coded_sample > 64)
> > > > > 
> > > > > Where does the process take so long that oss-fuzz gets a timeout if these
> > > > > are unreasonably high? I don't see nb_channels used anywhere in here where
> > > > > that matters.
> > > > > Is it in the PCM decoder? Because that decoder is meant to handle any
> > > > > arbitrary amount of channels, so limiting it to whatever FF_SANE_NB_CHANNELS
> > > > > is set to is not ok.
> > > > 
> > > > libavutil/channel_layout.c:627:17
> > > > or it will OOM before depending on how much memory is available
> > > 
> > > Does this fix the timeout?
> > > 
> > > > diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
> > > > index 2d6963b6df..a4fcd199e1 100644
> > > > --- a/libavutil/channel_layout.c
> > > > +++ b/libavutil/channel_layout.c
> > > > @@ -623,6 +623,8 @@ int av_channel_layout_describe_bprint(const AVChannelLayout *channel_layout,
> > > >           for (i = 0; i < channel_layout->nb_channels; i++) {
> > > >               enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i);
> > > > 
> > > > +            if (!av_bprint_is_complete(bp))
> > > > +                break;
> > > >               if (i)
> > > >                   av_bprintf(bp, "+");
> > > >               av_channel_name_bprint(bp, ch);
> > > 
> > > But this is not ok as it will affect the buffer len value
> > > av_channel_layout_describe() returns on success when truncation took place,
> > > so something else would have to be done.
> > 
> > This makes the testcase which is 108 bytes long take a bit more than 7 seconds
> > which is below the threshold for the timeout, but i would guess 30x on the channel
> > number would bring it well above
> 
> If you try to process that file without the fuzzer, does it also take 7
> seconds before it stops?

real	0m3.232s
user	0m1.064s
sys	0m2.156s

for a 108 byte file with your speed up patch, without that patch i didnt try as i have not
enough patience ATM

with 66 instead of 33 million channels

real	0m6.181s
user	0m1.774s
sys	0m4.385s


> If not, then the fuzzer is having Valgrind levels
> of penalty hit, and i don't think adding dubious checks in our codebase just
> to appease it is correct.

Please think again

The fuzzer is slower than non instrumented code, but its purpose is to detect issues

First i dont see how that leads to "dubious checks" or "appease" these are loaded
words not appropriate for a technical discusison

Second, if the fuzzer spends the majority of its time in a busy loop copying millions
of channels. Thats time it will not find any issues.
So both the fuzzing process itself as well as DOS of users are affected

Third if it goes over its threshold, for its purpose it found an issue.
If thats not the case the fuzzers configuration does not match the definition of a
DOS issue. Thats a problem in itself. (this isnt applying as the 7 sec is below)

Fourth, we should try to investigate and fix issues (truth we often dont have the
resources to fully investigate everything). Here the issue is clearly 33 million
channels being worked on as valid data. It would take time, alot of time to process each
sample times 33 million. The user does not have 33 million speakers and its not
reasonable for her computer to have to process files just to later reject them.
She should have a way to reject this earlier without the processing. This sample
also is no worst case at all. We can apply your speedup and wait for the fuzzer
to find a worse one, this may take time or might never happen. This doesnt
mean the worst case is 6 seconds for a 108 byte file. That said a web browser
opening 100 media files each 108 bytes and consuming 6 seconds cpu time
is already quite bad, also the memory requirement is bad too. This is a random
example maybe its not possible exactly like that, its more to show this issue
100 108 byte files is 10kb 10minutes of cpu time is alot for that, maybe a real
exploit would use a differnt demuxer or whatever. The issue iam having is that
Adding a check on channels is blocked on arguments that a specific testcase isnt
bad enough NOT on arguments that the WORST case would not be bad enough.
Thats like saying going 1 byte over the array but still being in an allocated
struct is not so bad disregarding what the maximum would be.

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/20240702/b165b1ba/attachment.sig>