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

[Harry Sintonen]

   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).

Considering the advanced state of heap corruption exploits these days this 
issue should be considered exploitable security vulnerability. Address 
space layout randomization or non-executable stack only slows down a 
determined attacker.


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';


Note that the fix is conservative: The check for early termination could 
be located just after the psz_subtitle += strcspn( psz_subtitle, ">" ); as 
well. However this would make it easier to reintroduce the bug later on 
due to forgetting to check for the termination when needed. I think it's 
more sensible to have a more broad generic check to avoid this.


References:
http://www.owasp.org/index.php/Buffer_Overflows
https://secure.wikimedia.org/wikipedia/en/wiki/Heap_overflow


   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");}