[vlc-devel] Security: Subtitle StripTags heap corruption, potentially exploitable. Patch included

[Harry Sintonen]

On Sun, 16 Jan 2011, Harry Sintonen wrote:

>  Hello
>
> I ran into some corrupt mkv that'd crash vlc. Debugging revealed an issue in 
> StripTags() function: It can run past the input string termination resulting 
> in a heap corruption.
>
> Assuming the input string contains a '<' char but doesn't include the 
> terminating '>' the routine will run past end of the string termination. It 
> happens because of psz_subtitle += strcspn( psz_subtitle, ">" ); in 
> combination with psz_subtitle++; will advance psz_subtitle past the string 
> termination. Bytes after the string termination will be copied to the 
> destination buffer, smashing the heap.
>
> It is trivial to test this issue by calling StripTags with a specially 
> crafted input string such as "<foo\0crashme" (use valgrind or a similar tool 
> to track the corruption).

Here's a way to reproduce the issue:

1. download http://www.mediafire.com/file/jezxko22nyn
2. echo -ne '<foo\0crashme' | dd conv=notrunc bs=1 seek=877862 \
of=refined-australia-blu720p-sample.mkv
3. vlc --sub-language English refined-australia-blu720p-sample.mkv

Depending on your platform you might need to run a debugger such as 
valgrind to actually see the heap corruption.


> Diff fixing the issue follows:
>
> diff --git a/modules/codec/subsdec.c b/modules/codec/subsdec.c
> index 22c951e..d0beae8 100644
> --- a/modules/codec/subsdec.c
> +++ b/modules/codec/subsdec.c
> @@ -640,6 +640,9 @@ static char *StripTags( char *psz_subtitle )
>             *psz_text++ = *psz_subtitle;
>         }
>
> +        /* Security fix: Account for the case where input ends early */
> +        if( *psz_subtitle == '\0' ) break;
> +
>         psz_subtitle++;
>     }
>     *psz_text = '\0';
> diff --git a/modules/codec/subsusf.c b/modules/codec/subsusf.c
> index c69c454..3886c52 100644
> --- a/modules/codec/subsusf.c
> +++ b/modules/codec/subsusf.c
> @@ -1072,6 +1072,9 @@ static char *StripTags( char *psz_subtitle )
>             *psz_text++ = *psz_subtitle;
>         }
>
> +        /* Security fix: Account for the case where input ends early */
> +        if( *psz_subtitle == '\0' ) break;
> +
>         psz_subtitle++;
>     }
>     *psz_text = '\0';

After checking some more it appears that similar unsafe strcspn() usage is 
present in other parts of the source tree as well (at least 
modules/codec/subsdec.c/CreateHtmlSubtitle() looks suspect). Whether 
these actually translate to vulnerabilities depends on where the input 
originates and how the output is processed. Dynamically allocated output 
should be able to handle the source buffer run off.


   Regards,
-- 
l=2001;main(i){float o,O,_,I,D;for(;O=I=l/571.-1.75,l;)for(putchar(--l%80?
i:10),o=D=l%80*.05-2,i=31;_=O*O,O=2*o*O+I,o=o*o-_+D,o+_+_<4+D&i++<87;);puts
("  Harry 'Piru' Sintonen <sintonen at iki.fi> http://www.iki.fi/sintonen");}