diff mbox series

[FFmpeg-devel,4/4] avcodec/faxcompr: Check for invalid VLC in decode_group3_1d_line()

Message ID 20210428150636.28396-4-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/4] avformat/mvdec: Check sample rate in parse_audio_var()
Related show

Checks

Context Check Description
andriy/x86_make_warn warning New warnings during build
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 April 28, 2021, 3:06 p.m. UTC
Fixes: infinite loop
Fixes: 33674/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TIFF_fuzzer-4816457818046464

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

Comments

Andreas Rheinhardt April 28, 2021, 3:37 p.m. UTC | #1
Michael Niedermayer:
> Fixes: infinite loop
> Fixes: 33674/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TIFF_fuzzer-4816457818046464
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/faxcompr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/faxcompr.c b/libavcodec/faxcompr.c
> index 7bf11d80ca..f1f7e67762 100644
> --- a/libavcodec/faxcompr.c
> +++ b/libavcodec/faxcompr.c
> @@ -213,7 +213,7 @@ static int decode_group3_1d_line(AVCodecContext *avctx, GetBitContext *gb,
>          run += t;
>          if (t < 64) {
>              *runs++ = run;
> -            if (runs >= runend) {
> +            if (runs >= runend || t < 0) {
>                  av_log(avctx, AV_LOG_ERROR, "Run overrun\n");
>                  return AVERROR_INVALIDDATA;
>              }
> 
This t is unsigned, so your added check is void. There is btw an else
part here that checks for errors: "} else if ((int)t == -1) {".
The only way I can think of for an infinite loop is that the part after
the end of the get_bits-reader needn't be zeroed and so it can be
mistaken for a valid code and enter the codepath for valid codes; in
particular, it can be a code corresponding to the symbol 0 in which case
one is not saved by the "if (pix_left <= run) {" check. And given that
this code does not use the unchecked bitstream reader, it will never
advance.
So it seems like the best way to fix this is to check for whether there
are any bits left before the get_vlc2() call.

- Andreas
Michael Niedermayer April 29, 2021, 2:29 p.m. UTC | #2
On Wed, Apr 28, 2021 at 05:37:39PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Fixes: infinite loop
> > Fixes: 33674/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TIFF_fuzzer-4816457818046464
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/faxcompr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/faxcompr.c b/libavcodec/faxcompr.c
> > index 7bf11d80ca..f1f7e67762 100644
> > --- a/libavcodec/faxcompr.c
> > +++ b/libavcodec/faxcompr.c
> > @@ -213,7 +213,7 @@ static int decode_group3_1d_line(AVCodecContext *avctx, GetBitContext *gb,
> >          run += t;
> >          if (t < 64) {
> >              *runs++ = run;
> > -            if (runs >= runend) {
> > +            if (runs >= runend || t < 0) {
> >                  av_log(avctx, AV_LOG_ERROR, "Run overrun\n");
> >                  return AVERROR_INVALIDDATA;
> >              }
> > 
> This t is unsigned, so your added check is void. There is btw an else
> part here that checks for errors: "} else if ((int)t == -1) {".
> The only way I can think of for an infinite loop is that the part after
> the end of the get_bits-reader needn't be zeroed and so it can be
> mistaken for a valid code and enter the codepath for valid codes; in
> particular, it can be a code corresponding to the symbol 0 in which case
> one is not saved by the "if (pix_left <= run) {" check. And given that
> this code does not use the unchecked bitstream reader, it will never
> advance.
> So it seems like the best way to fix this is to check for whether there
> are any bits left before the get_vlc2() call.

yes, ill do that.

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/faxcompr.c b/libavcodec/faxcompr.c
index 7bf11d80ca..f1f7e67762 100644
--- a/libavcodec/faxcompr.c
+++ b/libavcodec/faxcompr.c
@@ -213,7 +213,7 @@  static int decode_group3_1d_line(AVCodecContext *avctx, GetBitContext *gb,
         run += t;
         if (t < 64) {
             *runs++ = run;
-            if (runs >= runend) {
+            if (runs >= runend || t < 0) {
                 av_log(avctx, AV_LOG_ERROR, "Run overrun\n");
                 return AVERROR_INVALIDDATA;
             }