[FFmpeg-devel] [PATCH] Fix 'while' loop condition to prevent movtext crashes by tracking packet size

[Philip Langdale]

On Sun, 17 May 2015 21:36:42 +0530
Niklesh Lalwani <niklesh.lalwani at iitb.ac.in> wrote:

> From: Niklesh <niklesh.lalwani at iitb.ac.in>
> 
> Hi all,
> This patch fixes some movtext crashes caused due to incorrect 'while'
> loop condition. I will post several other patches to improve upon the
> code and null pointer dereferences once this is applied.
> 
> Signed-off-by: Niklesh <niklesh.lalwani at iitb.ac.in>
> ---
>  libavcodec/movtextdec.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/movtextdec.c b/libavcodec/movtextdec.c
> index 3059599..d03a188 100644
> --- a/libavcodec/movtextdec.c
> +++ b/libavcodec/movtextdec.c
> @@ -96,7 +96,7 @@ static int mov_text_decode_frame(AVCodecContext
> *avctx, char *ptr = avpkt->data;
>      char *end;
>      //char *ptr_temp;
> -    int text_length, tsmb_type, style_entries, tsmb_size;
> +    int text_length, tsmb_type, style_entries, tsmb_size, tracksize;
>      int **style_start = {0,};
>      int **style_end = {0,};
>      int **style_flags = {0,};
> @@ -135,11 +135,12 @@ static int mov_text_decode_frame(AVCodecContext
> *avctx, (AVRational){1,100});
>  
>      tsmb_size = 0;
> +    tracksize = 2 + text_length;
>      // Note that the spec recommends lines be no longer than 2048
> characters. av_bprint_init(&buf, 0, AV_BPRINT_SIZE_UNLIMITED);
>      if (text_length + 2 != avpkt->size) {
> -        while (text_length + 2 + tsmb_size < avpkt->size)  {
> -            tsmb = ptr + text_length + tsmb_size;
> +        while (tracksize < avpkt->size) {
> +            tsmb = ptr + tracksize -2;
>              tsmb_size = AV_RB32(tsmb);

You should check immediately if this size takes you past avpkt->size
and stop processing boxes. This is a red flat that you shouldn't process
the box, even if it isn't actually too big based on the style entries.

You also need to check if the number of style entries will cause you to
read past the end of the packet.

>              tsmb += 4;
>              tsmb_type = AV_RB32(tsmb);
> @@ -176,6 +177,7 @@ static int mov_text_decode_frame(AVCodecContext
> *avctx, av_freep(&style_end);
>                  av_freep(&style_flags);
>              }
> +            tracksize = tracksize + tsmb_size;

You also need to add +4 for the size field itself (which isn't counted
in the size value, IIRC. And you'll also need to adjust for large boxes
when you support those).

>          }
>      } else
>          text_to_ass(&buf, ptr, end, NULL, NULL, 0, 0);

--phil