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

[Michael Niedermayer]

On Tue, Feb 28, 2017 at 10:19:05PM -0500, Ronald S. Bultje wrote:
> 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

ive added "Timeout" after fix as thats what is "fixed", should avoid
the ambiguity


> 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.

added the suggeted text

applied

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170301/0155e2c4/attachment.sig>