[FFmpeg-devel] [PATCH] nutdec: reject negative sm_size

[Andreas Cadhalpun]

On 19.12.2015 13:58, Michael Niedermayer wrote:
> On Sat, Dec 19, 2015 at 12:19:44PM +0100, Andreas Cadhalpun wrote:
>> On 19.12.2015 01:51, Michael Niedermayer wrote:
>>> On Fri, Dec 18, 2015 at 05:22:31PM +0100, Andreas Cadhalpun wrote:
>>>> If it is negative, it makes size larger than the size of the packet
>>>> buffer, causing invalid writes in avio_read.
>>>>
>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>>>> ---
>>>>  libavformat/nutdec.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c
>>>> index 286d1ee..47ae7a7 100644
>>>> --- a/libavformat/nutdec.c
>>>> +++ b/libavformat/nutdec.c
>>>> @@ -1146,6 +1146,11 @@ static int decode_frame(NUTContext *nut, AVPacket *pkt, int frame_code)
>>>>              goto fail;
>>>>          }
>>>>          sm_size = avio_tell(bc) - pkt->pos;
>>>> +        if (sm_size < 0) {
>>>
>>> did sm_size overflow and should be 64bit ?
>>
>> No.
>>
>>> did the byte position (avio_tell) move backward ?
>>
>> Yes.
>>
>>> (this should not happen)
>>
>> In that case, the check needs to be moved to read_sm_data.
>> Patch doing that is attached.
>>
>> Best regards,
>> Andreas
>>
> 
>>  nutdec.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 2f0ac251ee05a8a36ffbaec5a9d5346ac0ef4240  0001-nutdec-reject-negative-value_len-in-read_sm_data.patch
>> From bdca159087d426f5f989656a08464fec967b4bc3 Mon Sep 17 00:00:00 2001
>> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>> Date: Sat, 19 Dec 2015 12:02:56 +0100
>> Subject: [PATCH] nutdec: reject negative value_len in read_sm_data
>>
>> If it is negative, it can cause the byte position to move backwards in
>> avio_skip, which in turn makes sm_size negative and thus size larger
>> than the size of the packet buffer, causing invalid writes in avio_read.
>>
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>> ---
>>  libavformat/nutdec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/nutdec.c b/libavformat/nutdec.c
>> index 286d1ee..b33b3e2 100644
>> --- a/libavformat/nutdec.c
>> +++ b/libavformat/nutdec.c
>> @@ -934,7 +934,7 @@ static int read_sm_data(AVFormatContext *s, AVIOContext *bc, AVPacket *pkt, int
>>                  return ret;
>>              }
>>              value_len = ffio_read_varlen(bc);
>> -            if (avio_tell(bc) + value_len >= maxpos)
>> +            if (value_len < 0 || avio_tell(bc) + value_len >= maxpos)
>>                  return AVERROR_INVALIDDATA;
> 
> ok, also while at it please fix avio_tell(bc) + value_len, which i
> belive can overflow

OK, updated patch attached.

> you could alternatively change value_len to uint64_t which might
> simplify the check

It wouldn't help, because either avio_tell(bc) + value_len can be too large
or maxpos - avio_tell(bc) negative, which would be treated as a large
unsigned value, if value_len is unsigned.

Best regards,
Andreas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-nutdec-reject-negative-value_len-in-read_sm_data.patch
Type: text/x-diff
Size: 1302 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151219/a72ed1cb/attachment.patch>