[MPlayer-dev-eng] [PATCH] fix SAMI parsing

[Howard Chu]

Reimar Döffinger wrote:
> On Mon, Jun 07, 2010 at 10:40:54AM -0700, Howard Chu wrote:
>> Reimar Döffinger wrote:
>>>> Did that. Also had to check for " " in addition to " ".
>>>
>>> And how do you know that you/we did not yet miss another case?
>>> Especially since no base is specified in the strtol call.
>>
>> With base = 0 the only possibilities are base 8, 10, and 16. At most
>> base 16 packs 4 bits per byte so again, there is no chance for
>> overflow. Also keep in mind that there's 3 bytes of slop for free -
>> the "&#;" are being dropped.
>
> You said that the last time as well.
> I see more complex, more risky code at a security-relevant point
> for no benefit at all.
>
>>> This code is simply too complex for security-critical code
>>> where a mistake will allow writing arbitrary values onto the stack.
>>> Just add a
>>> if (p - text>= sizeof(text) - 8)
>>
>> Maximum length for UTF-8 is 6 bytes.
>
> Where did you get that from? To my knowledge the longest valid UTF-8
> is 4 bytes (which is without any relevance here though),

Try the original spec, RFC2279.
http://www.ietf.org/rfc/rfc2279.txt

Section 2. UTF-8 definition
 >>>>
In UTF-8, characters are encoded using sequences of 1 to 6 octets.
<<<<

> the maximum length for PUT_UTF8 is 7 bytes, and we need a
> 0-termination.

If PUT_UTF8 can output 7 bytes then it's broken.

> Here's even a patch to do this way that avoids losing any data if possible...
> Index: subreader.c
> ===================================================================
> --- subreader.c (revision 31346)
> +++ subreader.c (working copy)
> @@ -170,6 +170,8 @@
>
>          case 3: /* get all text until '<' appears */
>              if (*s == '\0') break;
> +           else if (p - text>= sizeof(text) - 8)
> +               sami_add_line(current, text,&p);
>              else if (!strncasecmp (s, "<br>", 4)) {
>                   sami_add_line(current, text,&p);
>                  s += 4;

I respect your attention to detail and security-consciousness. I don't respect 
your treatment of this topic with superstition, instead of facts.
I gave you solid facts - a binary number read from a base16 string cannot 
overflow the original space - and you're the one handwaving them away.

You gave a completely valid criticism - negative numbers need to be checked. 
Unicode is only defined for 0-0x7fffffff, i.e., positive only, and I've 
addressed that. The rest is just superstition.
-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/