[FFmpeg-devel] [PATCH] some length validation for mjpegdec

[Michael Niedermayer]

On Fri, Jul 23, 2010 at 06:55:17PM +0200, Reimar D?ffinger wrote:
> Hello,
> seems like in some places we might read far beyond the get_bits
> buffer and crash.
> Attached is an attempt to fix it.
> Some of the existing code doing such checks could be simplify
> by using get_bits_left, too.
> 
> Reimar

>  mjpegdec.c |   28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 60558c345376fc64f3cd9da4d4c52f22302b0074  lenval.diff
> Index: libavcodec/mjpegdec.c
> ===================================================================
> --- libavcodec/mjpegdec.c	(revision 24435)
> +++ libavcodec/mjpegdec.c	(working copy)
> @@ -118,6 +118,10 @@

diff -p helps review, iam still strugeling to remember the line numbers
of every function n every file in every revission



>      int len, index, i, j;
>  
>      len = get_bits(&s->gb, 16) - 2;
> +    if (len > get_bits_left(&s->gb) >> 3) {
> +        av_log(s->avctx, AV_LOG_ERROR, "decode_dht: not enough data\n");
> +        return -1;
> +    }
>  
>      while (len >= 65) {
>          /* only 8 bit precision handled */

ok


> @@ -155,6 +159,10 @@
>      uint8_t val_table[256];
>  
>      len = get_bits(&s->gb, 16) - 2;
> +    if (len > get_bits_left(&s->gb) >> 3) {
> +        av_log(s->avctx, AV_LOG_ERROR, "decode_dht: not enough data\n");
> +        return -1;
> +    }
>  
>      while (len > 0) {
>          if (len < 17)
> @@ -198,7 +206,6 @@
>  {
>      int len, nb_components, i, width, height, pix_fmt_id;
>  
> -    /* XXX: verify len field validity */
>      len = get_bits(&s->gb, 16);
>      s->bits= get_bits(&s->gb, 8);
>  

> @@ -225,6 +232,15 @@
>      if (nb_components <= 0 ||
>          nb_components > MAX_COMPONENTS)
>          return -1;
> +    if (len != 8 + 3*nb_components)
> +    {
> +        av_log(s->avctx, AV_LOG_DEBUG, "decode_sof0: error, len(%d) mismatch\n", len);
> +        len = 8 + 3*nb_components;
> +    }
> +    if (len > (get_bits_left(&s->gb) + 4*16) >> 3) {
> +        av_log(s->avctx, AV_LOG_ERROR, "decode_sof: not enough data\n");
> +        return -1;
> +    }

this is hmm
len is not used anywhere, setting it to a wrong value
is just wrong
the actual amount read could be tested without messing with len


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

There will always be a question for which you do not know the correct awnser.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100724/858be305/attachment.pgp>