diff mbox series

[FFmpeg-devel,4/6] avformat/flvdec: Check for nesting depth in amf_parse_object()

Message ID 20210123221056.3366-4-michael@niedermayer.cc
State Accepted
Headers show
Series [FFmpeg-devel,1/6] avutil/common: Add FFABSU() for a signed -> unsigned ABS
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Michael Niedermayer Jan. 23, 2021, 10:10 p.m. UTC
Fixes: out of array access
Fixes: 29202/clusterfuzz-testcase-minimized-ffmpeg_dem_KUX_fuzzer-5112845840809984

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/flvdec.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Anton Khirnov Jan. 24, 2021, 1:14 p.m. UTC | #1
Quoting Michael Niedermayer (2021-01-23 23:10:54)
> Fixes: out of array access
> Fixes: 29202/clusterfuzz-testcase-minimized-ffmpeg_dem_KUX_fuzzer-5112845840809984
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/flvdec.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> index 07ef342278..e15be0a221 100644
> --- a/libavformat/flvdec.c
> +++ b/libavformat/flvdec.c
> @@ -41,6 +41,8 @@
>  
>  #define RESYNC_BUFFER_SIZE (1<<20)
>  
> +#define MAX_DEPTH 10

Why 10 specifically. And which buffer overflows?
Michael Niedermayer Jan. 25, 2021, 8:29 p.m. UTC | #2
On Sun, Jan 24, 2021 at 02:14:43PM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2021-01-23 23:10:54)
> > Fixes: out of array access
> > Fixes: 29202/clusterfuzz-testcase-minimized-ffmpeg_dem_KUX_fuzzer-5112845840809984
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/flvdec.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> > index 07ef342278..e15be0a221 100644
> > --- a/libavformat/flvdec.c
> > +++ b/libavformat/flvdec.c
> > @@ -41,6 +41,8 @@
> >  
> >  #define RESYNC_BUFFER_SIZE (1<<20)
> >  
> > +#define MAX_DEPTH 10
> 
> Why 10 specifically.

10 is arbitrary, we could pick 5 or 100 probably


> And which buffer overflows?

stack


[...]
Anton Khirnov Jan. 26, 2021, 3:37 p.m. UTC | #3
Quoting Michael Niedermayer (2021-01-25 21:29:21)
> On Sun, Jan 24, 2021 at 02:14:43PM +0100, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2021-01-23 23:10:54)
> > > Fixes: out of array access
> > > Fixes: 29202/clusterfuzz-testcase-minimized-ffmpeg_dem_KUX_fuzzer-5112845840809984
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/flvdec.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> > > index 07ef342278..e15be0a221 100644
> > > --- a/libavformat/flvdec.c
> > > +++ b/libavformat/flvdec.c
> > > @@ -41,6 +41,8 @@
> > >  
> > >  #define RESYNC_BUFFER_SIZE (1<<20)
> > >  
> > > +#define MAX_DEPTH 10
> > 
> > Why 10 specifically.
> 
> 10 is arbitrary, we could pick 5 or 100 probably

or a round number like 16 :)

> 
> > And which buffer overflows?
> 
> stack

Okay, please add a comment like
// arbitrary limit to prevent unbounded recursion
Michael Niedermayer Jan. 26, 2021, 4:17 p.m. UTC | #4
On Tue, Jan 26, 2021 at 04:37:38PM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2021-01-25 21:29:21)
> > On Sun, Jan 24, 2021 at 02:14:43PM +0100, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2021-01-23 23:10:54)
> > > > Fixes: out of array access
> > > > Fixes: 29202/clusterfuzz-testcase-minimized-ffmpeg_dem_KUX_fuzzer-5112845840809984
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavformat/flvdec.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> > > > index 07ef342278..e15be0a221 100644
> > > > --- a/libavformat/flvdec.c
> > > > +++ b/libavformat/flvdec.c
> > > > @@ -41,6 +41,8 @@
> > > >  
> > > >  #define RESYNC_BUFFER_SIZE (1<<20)
> > > >  
> > > > +#define MAX_DEPTH 10
> > > 
> > > Why 10 specifically.
> > 
> > 10 is arbitrary, we could pick 5 or 100 probably
> 
> or a round number like 16 :)
> 
> > 
> > > And which buffer overflows?
> > 
> > stack
> 
> Okay, please add a comment like
> // arbitrary limit to prevent unbounded recursion

will apply with that

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
index 07ef342278..e15be0a221 100644
--- a/libavformat/flvdec.c
+++ b/libavformat/flvdec.c
@@ -41,6 +41,8 @@ 
 
 #define RESYNC_BUFFER_SIZE (1<<20)
 
+#define MAX_DEPTH 10
+
 typedef struct FLVContext {
     const AVClass *class; ///< Class for private options.
     int trust_metadata;   ///< configure streams according onMetaData
@@ -493,6 +495,9 @@  static int amf_parse_object(AVFormatContext *s, AVStream *astream,
     double num_val;
     amf_date date;
 
+    if (depth > MAX_DEPTH)
+        return AVERROR_PATCHWELCOME;
+
     num_val  = 0;
     ioc      = s->pb;
     if (avio_feof(ioc))