[FFmpeg-devel] Fix mjpeg decoder runaway from internal buffer

Michael Niedermayer michaelni
Fri Oct 22 00:54:05 CEST 2010


On Thu, Oct 21, 2010 at 10:32:25AM +0400, Anatoly Nenashev wrote:
> On 21.10.2010 03:13, Michael Niedermayer wrote:
>> On Wed, Oct 20, 2010 at 07:36:51PM +0400, Anatoly Nenashev wrote:
>>    
>>> On 19.10.2010 19:14, Michael Niedermayer wrote:
>>>      
>>>> On Tue, Oct 19, 2010 at 06:50:21PM +0400, Anatoly Nenashev wrote:
>>>>
>>>>        
>>>>> On 19.10.2010 18:31, Michael Niedermayer wrote:
>>>>>
>>>>>          
>>>>>> On Tue, Oct 19, 2010 at 05:51:55PM +0400, Anatoliy Nenashev wrote:
>>>>>>
>>>>>>
>>>>>>            
>>>>>>> Hi!
>>>>>>> In some cases there is a situation when mjpeg decoder runaway from
>>>>>>> allocated s->buffer.
>>>>>>> Usually it happens in VLC decoder for DC-AC coefficients when input
>>>>>>> frame is cirrupted.
>>>>>>> In this case it is caused by "specific" garbage at the end of the memory
>>>>>>> allocated for s->buffer.
>>>>>>>
>>>>>>> Here is a fix to prevent this situation.
>>>>>>>
>>>>>>>
>>>>>>>              
>>>>>> i dont see how this would prevent overreading the buffer. And no i dont
>>>>>> care that on your computer with your sample this week it works.
>>>>>> unless you can show that this always works (which i doubt) its not
>>>>>> a correct solution.
>>>>>>
>>>>>>
>>>>>>            
>>>>> 0xFF  value aligned to byte is deprecated for VLC value because it is
>>>>> used for markers. Thats why VLC decoder will  stop within error  when
>>>>> intersects s->buffer_size position.
>>>>>
>>>>>          
>>>> what you write makes no sense. any VLC is allowed, 0xFF occuring
>>>> in the bitstream are explicitly escaped. If i missed something in the
>>>> jpeg spec that disallows such vlcs then please refer to this part of the spec
>>>>
>>>>        
>>> You are right. Sorry, it was my mistake in specification reading.
>>> I found another way to fix original problem. I have added new macro in
>>> get_bits.h to check up if the buffer overreaded.
>>> This macro is used in mjpeg decoder. It also may be used in other decoders.
>>>
>>>
>>>      
>>    
>>>   get_bits.h |   24 ++++++++++++++++++++++++
>>>   1 file changed, 24 insertions(+)
>>> 1915f32b1baadab644335e3137c93c05ad3814ac  getbits.patch
>>> diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h
>>> index f4b3646..398e0f8 100644
>>> --- a/libavcodec/get_bits.h
>>> +++ b/libavcodec/get_bits.h
>>> @@ -124,6 +124,12 @@ LAST_SKIP_CACHE(name, gb, num)
>>>   LAST_SKIP_BITS(name, gb, num)
>>>       is equivalent to LAST_SKIP_CACHE; SKIP_COUNTER
>>>
>>> +INIT_OVERREAD_CHECKER(name,gb)
>>> +    initialize local variables for overread checker
>>> +
>>> +BUFFER_OVERREADED(name,gb)
>>> +    check if buffer overreaded
>>> +
>>>   for examples see get_bits, show_bits, skip_bits, get_vlc
>>>   */
>>>
>>> @@ -181,6 +187,12 @@ for examples see get_bits, show_bits, skip_bits, get_vlc
>>>   #   define GET_CACHE(name, gb)\
>>>           ((uint32_t)name##_cache)
>>>
>>> +#   define INIT_OVERREAD_CHECKER(name, gb)\
>>> +    const int name##_size_in_bits= (gb)->size_in_bits;\
>>> +
>>> +#   define BUFFER_OVERREADED(name, gb)\
>>> +        (name##_index>= name##_size_in_bits)
>>> +
>>>   static inline int get_bits_count(const GetBitContext *s){
>>>       return s->index;
>>>   }
>>> @@ -235,6 +247,12 @@ static inline void skip_bits_long(GetBitContext *s, int n){
>>>   #   define GET_CACHE(name, gb)\
>>>           ((uint32_t)name##_cache)
>>>
>>> +#   define INIT_OVERREAD_CHECKER(name, gb)\
>>> +    const uint8_t * name##_buffer_end= (gb)->buffer_end;\
>>> +
>>> +#   define BUFFER_OVERREADED(name, gb)\
>>> +        (name##_buffer_ptr>  name##_buffer_end + 1)
>>> +
>>>   static inline int get_bits_count(const GetBitContext *s){
>>>       return (s->buffer_ptr - s->buffer)*8 - 16 + s->bit_count;
>>>   }
>>> @@ -310,6 +328,12 @@ static inline void skip_bits_long(GetBitContext *s, int n){
>>>   #   define GET_CACHE(name, gb)\
>>>           (name##_cache0)
>>>
>>> +#   define INIT_OVERREAD_CHECKER(name, gb)\
>>> +    const uint32_t * name##_buffer_end= (gb)->buffer_end;\
>>> +
>>> +#   define BUFFER_OVERREADED(name, gb)\
>>> +        (name##_buffer_ptr>  name##_buffer_end + 1)
>>> +
>>>   static inline int get_bits_count(const GetBitContext *s){
>>>       return ((uint8_t*)s->buffer_ptr - s->buffer)*8 - 32 + s->bit_count;
>>>   }
>>>      
>>    
>>>   mjpegdec.c |   17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>> 207d3a410e151e1e30ddaddde51449846e111c51  mjpegdec.patch
>>> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
>>> index 5686380..04f2dfe 100644
>>> --- a/libavcodec/mjpegdec.c
>>> +++ b/libavcodec/mjpegdec.c
>>> @@ -407,6 +407,7 @@ static int decode_block(MJpegDecodeContext *s, DCTELEM *block,
>>>       /* AC coefs */
>>>       i = 0;
>>>       {OPEN_READER(re,&s->gb)
>>> +     INIT_OVERREAD_CHECKER(re,&s->gb)
>>>       for(;;) {
>>>           UPDATE_CACHE(re,&s->gb);
>>>           GET_VLC(code, re,&s->gb, s->vlcs[1][ac_index].table, 9, 2)
>>> @@ -440,6 +441,11 @@ static int decode_block(MJpegDecodeContext *s, DCTELEM *block,
>>>               j = s->scantable.permutated[i];
>>>               block[j] = level * quant_matrix[j];
>>>           }
>>> +
>>> +        if (BUFFER_OVERREADED(re,&s->gb)) {
>>> +            av_log(s->avctx, AV_LOG_ERROR, "buffer overreaded in decode_block\n");
>>> +            return -1;
>>> +        }
>>>       }
>>>       CLOSE_READER(re,&s->gb)}
>>>
>>>      
>> checking after each coefficient is too slow.
>> Finding out how many bits a block can contain in worst case allocating
>> that amount extra and then just checking once a block is simpler.
>> This also should then not require these macros
>>
>>    
> This is good decision for non-broken input data. In some cases VLC  
> decoder can read unpredictable amount of data.
> For example, let's look at the loop in function "decode_block":
>
> for(;;) {
>         UPDATE_CACHE(re, &s->gb);
>         GET_VLC(code, re, &s->gb, s->vlcs[1][ac_index].table, 9, 2)
>
>         /* EOB */
>         if (code == 0x10)
>             break;
>         i += ((unsigned)code) >> 4;
>         if(code != 0x100){
>             code &= 0xf;
>             if(code > MIN_CACHE_BITS - 16){
>                 UPDATE_CACHE(re, &s->gb)
>             }
>             {
>                 int cache=GET_CACHE(re,&s->gb);
>                 int sign=(~cache)>>31;
>                 level = (NEG_USR32(sign ^ cache,code) ^ sign) - sign;
>             }
>
>             LAST_SKIP_BITS(re, &s->gb, code)
>
>             if (i >= 63) {
>                 if(i == 63){
>                     j = s->scantable.permutated[63];
>                     block[j] = level * quant_matrix[j];
>                     break;
>                 }
>                 av_log(s->avctx, AV_LOG_ERROR, "error count: %d\n", i);
>                 return -1;
>             }
>             j = s->scantable.permutated[i];
>             block[j] = level * quant_matrix[j];
>         }
>     }
>
> If in every loop VLC decoder returns code=0x100 than it will be infinite  
> loop.

ive fixed that, actually this case was possibly exploitable as the i>=63
is a signed check and i can overflow that way

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101022/2123f9c8/attachment.pgp>



More information about the ffmpeg-devel mailing list