[FFmpeg-devel] [PATCH 2/2] avformat/mpc8: fix hang with fuzzed file

[Reimar Döffinger]

On 03.02.2015, at 22:06, wm4 <nfxjfg at googlemail.com> wrote:
> On Tue, 3 Feb 2015 22:00:11 +0100
> Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> 
>> On Tue, Feb 03, 2015 at 09:54:51PM +0100, wm4 wrote:
>>> On Tue, 3 Feb 2015 21:47:57 +0100
>>> Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
>>> 
>>>> On Tue, Feb 03, 2015 at 07:04:12PM +0100, wm4 wrote:
>>>>> This can lead to an endless loop by seeking back a few bytes after each
>>>>> attempted chunk read. Assuming negative sizes are always invalid, this
>>>>> is easy to fix. Other code in this demuxer treats negative sizes as
>>>>> invalid as well.
>>>>> 
>>>>> Fixes ticket #4262.
>>>>> ---
>>>>> libavformat/mpc8.c | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>> 
>>>>> diff --git a/libavformat/mpc8.c b/libavformat/mpc8.c
>>>>> index d6ca338..6524c7e 100644
>>>>> --- a/libavformat/mpc8.c
>>>>> +++ b/libavformat/mpc8.c
>>>>> @@ -223,6 +223,10 @@ static int mpc8_read_header(AVFormatContext *s)
>>>>>     while(!avio_feof(pb)){
>>>>>         pos = avio_tell(pb);
>>>>>         mpc8_get_chunk_header(pb, &tag, &size);
>>>>> +        if (size < 0) {
>>>> 
>>>> Isn't the only way for this to become negative for a too
>>>> large uint64_t to be assigned to a int64_t?
>>>> I.e. undefined behaviour.
>>>> In that case this isn't quite the right way in the strictest sense,
>>>> though it is likely to work "normally".
>>> 
>>> This is what mpc8_get_chunk_header() does:
>>> 
>>>    pos = avio_tell(pb);
>>>    *tag = avio_rl16(pb);
>>>    *size = ffio_read_varlen(pb);
>>>    *size -= avio_tell(pb) - pos;
>>> 
>>> ffio_read_varlen() returns uint64_t, so I don't think it's supposed to
>>> read negative values. But the last line could make it negative anyway
>>> if the chunk size field read here is inconsistent.
>> 
>> The problem is in this line:
>> *size = ffio_read_varlen(pb);
>> *size is int64_t.
>> If ffio_read_varlen returns a value > 0x7f..ffull then this
>> assignment is undefined, which a fairly risky thing to allow
>> for a security-relevant check.
> 
> AFAIK it's implementation-defined, rather than undefined. Which means
> it works on all CPUs with complement-of-2 integers anyway.

You're right, I got confused.

> If you prefer, I can change them all to unsigned...

I don't think I like that all that much, I'm fine with leaving it as-is.