[FFmpeg-devel] [PATCH] fix for bfi buffer overread (roundup issue 2497)

[Michael Niedermayer]

On Thu, Jan 06, 2011 at 05:05:12PM -0500, Daniel Kang wrote:
> On Thu, Jan 6, 2011 at 4:38 PM, Michael Niedermayer <michaelni at gmx.at>wrote:
> 
> >  >  bfi.c |    7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > f75684d72bc0d5aa9b9e00887eb627ce31b2ac29  bfi_buffer_sanity_check.diff
> > > From 21d730f77e910810c1d3b2b69ce167d2da702be9 Mon Sep 17 00:00:00 2001
> > > From: Daniel Kang <daniel.d.kang at gmail.com>
> > > Date: Wed, 5 Jan 2011 23:46:33 -0500
> > > Subject: [PATCH] Sanity check on buffer reads
> > >
> > > ---
> > >  libavcodec/bfi.c |    7 ++++++-
> > >  1 files changed, 6 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/libavcodec/bfi.c b/libavcodec/bfi.c
> > > index 91c8f6d..00631f0 100644
> > > --- a/libavcodec/bfi.c
> > > +++ b/libavcodec/bfi.c
> > > @@ -47,7 +47,7 @@ static av_cold int bfi_decode_init(AVCodecContext *
> > avctx)
> > >  static int bfi_decode_frame(AVCodecContext * avctx, void *data,
> > >                              int *data_size, AVPacket *avpkt)
> > >  {
> > > -    const uint8_t *buf = avpkt->data;
> > > +    const uint8_t *buf = avpkt->data, *buf2 = avpkt->data;
> > >      int buf_size = avpkt->size;
> > >      BFIContext *bfi = avctx->priv_data;
> > >      uint8_t *dst = bfi->dst;
> > > @@ -99,6 +99,11 @@ static int bfi_decode_frame(AVCodecContext * avctx,
> > void *data,
> > >          unsigned int code = byte >> 6;
> > >          unsigned int length = byte & ~0xC0;
> > >
> > > +        if (buf-buf2 >= buf_size) {
> > > +            av_log(NULL, AV_LOG_ERROR, "Input resolution larger than
> > actual frame.\n");
> >                      ^^^^
> > should be avctx, so the user knows from where the error message comes from.
> >
> > also with buf_end= avpkt->data + buf_size
> > the check becomes buf >= buf_end which is a bit simpler and maybe slightly
> > more
> > readable though thats nitpicking
> >
> > also the following can still overread:
> >        switch (code) {
> >
> >        case 0:                //Normal Chain
> >            bytestream_get_buffer(&buf, dst, length);
> >            dst += length;
> >
> 
> I have updated the patch to use buf_end and include a second check.

>  bfi.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> e49f9bcda43631706025ad413421f05e5ecd2c99  bfi_buffer_sanity_check.diff
> From dd3ed1e69b99d479948593c795ecef05dd419ede Mon Sep 17 00:00:00 2001
> From: Daniel Kang <daniel.d.kang at gmail.com>
> Date: Wed, 5 Jan 2011 23:46:33 -0500
> Subject: [PATCH] Sanity check on buffer reads
> 
> ---
>  libavcodec/bfi.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/libavcodec/bfi.c b/libavcodec/bfi.c
> index 91c8f6d..13dd712 100644
> --- a/libavcodec/bfi.c
> +++ b/libavcodec/bfi.c
> @@ -47,7 +47,7 @@ static av_cold int bfi_decode_init(AVCodecContext * avctx)
>  static int bfi_decode_frame(AVCodecContext * avctx, void *data,
>                              int *data_size, AVPacket *avpkt)
>  {
> -    const uint8_t *buf = avpkt->data;
> +    const uint8_t *buf = avpkt->data, *buf_end = avpkt->data + avpkt->size;
>      int buf_size = avpkt->size;
>      BFIContext *bfi = avctx->priv_data;
>      uint8_t *dst = bfi->dst;
> @@ -99,6 +99,11 @@ static int bfi_decode_frame(AVCodecContext * avctx, void *data,
>          unsigned int code = byte >> 6;
>          unsigned int length = byte & ~0xC0;
> 
> +        if (buf >= buf_end) {
> +            av_log(avctx, AV_LOG_ERROR, "Input resolution larger than actual frame.\n");
> +            return -1;
> +        }
> +
>          /* Get length and offset(if required) */
>          if (length == 0) {
>              if (code == 1) {
> @@ -121,6 +126,10 @@ static int bfi_decode_frame(AVCodecContext * avctx, void *data,
>          switch (code) {
> 
>          case 0:                //Normal Chain
> +            if (buf + length >= buf_end) {

This has a very small chance that buf + length overflows, that is if buf is
close to 0xFFFFFFFF on a 32bit system or 0xFFFFFFFFFFFFFFFF on 64bit then
buf + length can be smaller than buf and thus also smaller than buf_end

This is unlikely in practice but its easy to avoid this problem by using

if(length >= buf_end - buf)

as a rule of thumb, the variable to be checked for validity should be
alone on one side of the </>/<=/>= to avoid this kind of problem

The same issue exists with code like
if(something * sizeof(some struct) > foo)
vs.
if(something > foo / sizeof(some struct))

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

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110106/e25c645d/attachment.pgp>