[FFmpeg-devel] [PATCH 2/2] avcodec/vp8: Check for bitsteam end in decode_mb_row_no_filter()

[Ronald S. Bultje]

Hi Michael,

On Tue, Feb 28, 2017 at 11:29 AM, Michael Niedermayer <
michael at niedermayer.cc> wrote:

> On Tue, Feb 28, 2017 at 07:44:12AM -0500, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Mon, Feb 27, 2017 at 10:28 PM, Michael Niedermayer <
> > michael at niedermayer.cc> wrote:
> >
> > > Fixes: 686/clusterfuzz-testcase-5853946876788736
> > >
> > > Found-by: continuous fuzzing process https://github.com/google/oss-
> > > fuzz/tree/master/targets/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >  libavcodec/vp8.c | 20 ++++++++++++++------
> > >  libavcodec/vp8.h |  2 +-
> > >  2 files changed, 15 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
> > > index c1c3eb7072..cc158528ef 100644
> > > --- a/libavcodec/vp8.c
> > > +++ b/libavcodec/vp8.c
> > > @@ -2275,7 +2275,7 @@ static void vp8_decode_mv_mb_modes(
> AVCodecContext
> > > *avctx, VP8Frame *cur_frame,
> > >  #define update_pos(td, mb_y, mb_x) while(0)
> > >  #endif
> > >
> > > -static av_always_inline void decode_mb_row_no_filter(AVCodecContext
> > > *avctx, void *tdata,
> > > +static av_always_inline int decode_mb_row_no_filter(AVCodecContext
> > > *avctx, void *tdata,
> > >                                          int jobnr, int threadnr, int
> > > is_vp7)
> > >  {
> > >      VP8Context *s = avctx->priv_data;
> > > @@ -2291,6 +2291,10 @@ static av_always_inline void
> > > decode_mb_row_no_filter(AVCodecContext *avctx, void
> > >          curframe->tf.f->data[1] +  8 * mb_y * s->uvlinesize,
> > >          curframe->tf.f->data[2] +  8 * mb_y * s->uvlinesize
> > >      };
> > > +
> > > +    if (c->end <= c->buffer && c->bits >= 0)
> > > +         return AVERROR_INVALIDDATA;
> >
> >
> > From vp56.h:
> >
> >     if(bits >= 0 && c->buffer < c->end) {
> >         code_word |= bytestream_get_be16(&c->buffer) << bits;
> >         bits -= 16;
> >     }
> >
> > So this looks supicious, c->end should never be more than 1 byte beyond
> > c->buffer (which is padded by AV_INPUT_BUFFER_PADDING_SIZE). What is the
> > real issue?
>
> The real issue is that the code runs with blindfolds and hands tied
> behind its back, so it doesnt know when its running toward a wall
> and in fact it doesnt stop running when it hits the wall not even
> once it starved, its even running in its grave.
>
> You can feed the code with approximately a header with huge resolution
> and 0 bytes of actual image and it wont notice, and after a really long
> time it still wont notice it is decoding a image out of a non existing
> bitstream.
>
> That check checks if the end of the bitstream is reached and returns
> an error in that case. That way the test sample finished in 10sec
> instead of not finishing within the time i waited


Aaaaah, that explains things.

The commit message uses "fuzz" and "fix" in the same sentence construction,
suggesting a security issue. You're now telling me there is in fact no
security issue. That suggests the commit message is wrong.

I'm fine with the patch as is, but change the commit message. I would
suggest still referring to the filename, but avoiding the word "fix" since
it doesn't fix anything - since there is no bug. And then explain (in the
commit message) that this shortcuts (i.e. speeds up) the error and
return-to-user when decoding a truncated frame. No error message is
necessary, it'll be something silly like "truncated bitstream" which
clobbers my terminal when I'm fuzzing and is otherwise virtually useless.

Ronald