[FFmpeg-devel] [PATCH] avcodec/rscc: Avoid returning frames that have nearly no undamaged pixels in them

Michael Niedermayer michael at niedermayer.cc
Fri Jan 18 01:33:50 EET 2019


On Wed, Jan 16, 2019 at 09:05:18PM -0500, Vittorio Giovara wrote:
> On Wed, Jan 16, 2019 at 7:44 PM Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > Fixes: Timeout
> > Fixes:
> > 12192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> >
> > Before:
> > clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> > in 15423 ms
> > After:
> > clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RSCC_fuzzer-6279038004363264
> > in 190 ms
> >
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by
> > <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>:
> > Michael Niedermayer <michael at niedermayer.cc>
> > ---
> >  libavcodec/rscc.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/rscc.c b/libavcodec/rscc.c
> > index 7921f149ed..fa066afd7f 100644
> > --- a/libavcodec/rscc.c
> > +++ b/libavcodec/rscc.c
> > @@ -64,6 +64,7 @@ typedef struct RsccContext {
> >      /* zlib interaction */
> >      uint8_t *inflated_buf;
> >      uLongf inflated_size;
> > +    int valid_pixels
> >  } RsccContext;
> >
> >  static av_cold int rscc_init(AVCodecContext *avctx)
> > @@ -348,7 +349,11 @@ static int rscc_decode_frame(AVCodecContext *avctx,
> > void *data,
> >          memcpy (frame->data[1], ctx->palette, AVPALETTE_SIZE);
> >      }
> >
> > -    *got_frame = 1;
> > +    // We only return a picture when more than 5% is undameged, this
> > avoids copying nearly broken frames around
> > +    if (ctx->valid_pixels < ctx->inflated_size)
> > +        ctx->valid_pixels += pixel_size;
> > +    if (ctx->valid_pixels >= ctx->inflated_size/20)
> >
> 
> this feels arbitrary and hackish, for very little gain, IMO

Would you be ok with rejecting RSCC files without a keyframe ?
or more precissely all frames before a keyframe and thus if there is
no keyframe the whole file
(that would be a superset of what this patch rejects)

I suggested that arbitrary check above as it would allow decoding more data
in case of a damaged or lost keyframe

The gain these changes have is they increase the cost for an
attacker in a DOS attack. (they also improve the efficiency of the fuzzer but
thats secondary)
As is you can have a file with huge resolution where each frame only encodes
1 pixel in about a dozen bytes per frame. That transforms a very low cost of
network bandwidth from an attacker to a rather large computational cost on
anything processing such file.
Requiring more than 1 valid pixels in a billion or Requiring a valid keyframe
would increase the cost for an attacker.

Also do you see a cleaner way to achive this as the author of this
decoder ?

Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"I am not trying to be anyone's saviour, I'm trying to think about the
 future and not be sad" - Elon Musk

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190118/4ba3ca43/attachment.sig>


More information about the ffmpeg-devel mailing list