[FFmpeg-devel] [PATCH] buffer read sanity check (issue 2503)

[Daniel Kang]

On Fri, Jan 7, 2011 at 9:46 AM, Michael Niedermayer <michaelni at gmx.at>wrote:

>  On Thu, Jan 06, 2011 at 11:51:58PM -0500, Daniel Kang wrote:
> > ffmpeg does not check for buffer overreading in the dpx decoder. For
> > invalid headers, the target_packet_size or width could be too large. The
> > patch attached adds a sanity check.
>
> >  dpx.c |    9 +++++++++
> >  1 file changed, 9 insertions(+)
> > e3c6cd5042d736935c2c5a730c93ee96f108b988  dpx_buffer_sanity_check.diff
> > From ae6ec1ce796735a666afb6b911c8c9e594b1eb8a Mon Sep 17 00:00:00 2001
> > From: Daniel Kang <daniel.d.kang at gmail.com>
> > Date: Thu, 6 Jan 2011 23:34:05 -0500
> > Subject: [PATCH] dpx buffer overread sanity check
> >
> > ---
> >  libavcodec/dpx.c |    9 +++++++++
> >  1 files changed, 9 insertions(+), 0 deletions(-)
> >
> > diff --git a/libavcodec/dpx.c b/libavcodec/dpx.c
> > index f92b3d0..588ec6c 100644
> > --- a/libavcodec/dpx.c
> > +++ b/libavcodec/dpx.c
> > @@ -55,6 +55,7 @@ static int decode_frame(AVCodecContext *avctx,
> >                          AVPacket *avpkt)
> >  {
> >      const uint8_t *buf = avpkt->data;
> > +    const uint8_t *buf_end = avpkt->data + avpkt->size;
> >      int buf_size       = avpkt->size;
> >      DPXContext *const s = avctx->priv_data;
> >      AVFrame *picture  = data;
> > @@ -174,6 +175,10 @@ static int decode_frame(AVCodecContext *avctx,
> >          case 16:
> >              if (source_packet_size == target_packet_size) {
> >                  for (x = 0; x < avctx->height; x++) {
> > +                    if (buf + target_packet_size*avctx->width > buf_end)
> {
> > +                        av_log(avctx, AV_LOG_ERROR, "Overread buffer.
> Invalid header?\n");
> > +                        return -1;
> > +                    }
> >                      memcpy(ptr, buf, target_packet_size*avctx->width);
> >                      ptr += stride;
> >                      buf += source_packet_size*avctx->width;
> > @@ -182,6 +187,10 @@ static int decode_frame(AVCodecContext *avctx,
> >                  for (x = 0; x < avctx->height; x++) {
> >                      uint8_t *dst = ptr;
> >                      for (y = 0; y < avctx->width; y++) {
> > +                        if (buf + target_packet_size > buf_end) {
> > +                            av_log(avctx, AV_LOG_ERROR, "Overread
> buffer. Invalid header?\n");
> > +                            return -1;
> > +                        }
>
> something like
>
> if(source_packet_size * avctx->width > buf_end - buf)
> at a single spot should be able to catch all i think


It would be possible to move the check outside the loop, but I am not
sure if target_packet_size*avctx->width*avctx->height would overflow or
not. Should I check for this instead?