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

[Howard Chu]

Reimar Döffinger wrote:
> On Mon, Jun 07, 2010 at 12:29:00AM -0700, Howard Chu wrote:
>> Howard Chu wrote:
>>> Reimar Döffinger wrote:
>>>> On Sun, Jun 06, 2010 at 07:24:46PM -0400, hyc at highlandsun.com wrote:
>>>>> On Mon, Jun 07, 2010 at 12:26:33AM +0200, Reimar D?ffinger wrote:
>>>>>> On Sun, Jun 06, 2010 at 02:51:29PM -0700, Howard Chu wrote:
>>>>>>
>>>>>>> +		uint32_t c = strtol(s+2,&s, 0);
>>>>>>> +		uint8_t tmp;
>>>>>>> +		PUT_UTF8(c, tmp, *p++ = tmp;)
>>>>>>> +		if (*s == ';') s++; }
>>>>>>
>>>>>> I'm sorry, but I think you'll have to somehow "prove"
>>>>>> this is not a security issue.
>>>>>> Checking and documenting that PUT_UTF8 will never write
>>>>>> more than we read might be possible.
>>>>>> Or just "blindly" checking we still have at least 8 bytes
>>>>>> free should do as well.
>>>>>
>>>>> Not necessary. Decimal numbers encode only 3.25 bits per byte, while UTF-8 encodes 7 bits per byte. This conversion will always fit.
>>>>
>>>> Given that 3.25 bits just don't exist, it obviously is not that easy.
>>>> And you forgot another case as well: negative numbers.
>>>> This kind of thing just can't be done hand-wavy like that, or you'll
>>>> always miss a case.
>>
>>> Of course, we could just check for a '-' in the string and drop this entity.
>>> Nobody should be using negative character codes.
>>
>> 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.

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

> break;
> or so.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/