| Message ID | 20170228032830.3094-2-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 7b5ff7d57355dc608f0fd86e3ab32a2fda65e752 |
| Headers | show |
Hi, On Mon, Feb 27, 2017 at 10:28 PM, Michael Niedermayer < michael@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@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? Ronald
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@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@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 If you want i can also add an error message that says that the bitstream end is reached before the image had finished decoding [...]
Hi Michael, On Tue, Feb 28, 2017 at 11:29 AM, Michael Niedermayer < michael@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@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@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
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@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@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@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 [...]
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; + if (mb_y == 0) prev_td = td; else @@ -2394,18 +2398,19 @@ static av_always_inline void decode_mb_row_no_filter(AVCodecContext *avctx, void update_pos(td, mb_y, mb_x); } } + return 0; } -static void vp7_decode_mb_row_no_filter(AVCodecContext *avctx, void *tdata, +static int vp7_decode_mb_row_no_filter(AVCodecContext *avctx, void *tdata, int jobnr, int threadnr) { - decode_mb_row_no_filter(avctx, tdata, jobnr, threadnr, 1); + return decode_mb_row_no_filter(avctx, tdata, jobnr, threadnr, 1); } -static void vp8_decode_mb_row_no_filter(AVCodecContext *avctx, void *tdata, +static int vp8_decode_mb_row_no_filter(AVCodecContext *avctx, void *tdata, int jobnr, int threadnr) { - decode_mb_row_no_filter(avctx, tdata, jobnr, threadnr, 0); + return decode_mb_row_no_filter(avctx, tdata, jobnr, threadnr, 0); } static av_always_inline void filter_mb_row(AVCodecContext *avctx, void *tdata, @@ -2488,13 +2493,16 @@ int vp78_decode_mb_row_sliced(AVCodecContext *avctx, void *tdata, int jobnr, VP8ThreadData *next_td = NULL, *prev_td = NULL; VP8Frame *curframe = s->curframe; int mb_y, num_jobs = s->num_jobs; + int ret; td->thread_nr = threadnr; for (mb_y = jobnr; mb_y < s->mb_height; mb_y += num_jobs) { if (mb_y >= s->mb_height) break; td->thread_mb_pos = mb_y << 16; - s->decode_mb_row_no_filter(avctx, tdata, jobnr, threadnr); + ret = s->decode_mb_row_no_filter(avctx, tdata, jobnr, threadnr); + if (ret < 0) + return ret; if (s->deblock_filter) s->filter_mb_row(avctx, tdata, jobnr, threadnr); update_pos(td, mb_y, INT_MAX & 0xFFFF); diff --git a/libavcodec/vp8.h b/libavcodec/vp8.h index 374e1388e2..6218fe0567 100644 --- a/libavcodec/vp8.h +++ b/libavcodec/vp8.h @@ -275,7 +275,7 @@ typedef struct VP8Context { */ int mb_layout; - void (*decode_mb_row_no_filter)(AVCodecContext *avctx, void *tdata, int jobnr, int threadnr); + int (*decode_mb_row_no_filter)(AVCodecContext *avctx, void *tdata, int jobnr, int threadnr); void (*filter_mb_row)(AVCodecContext *avctx, void *tdata, int jobnr, int threadnr); int vp7;
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@niedermayer.cc> --- libavcodec/vp8.c | 20 ++++++++++++++------ libavcodec/vp8.h | 2 +- 2 files changed, 15 insertions(+), 7 deletions(-)