[FFmpeg-devel] [PATCH 1/2] MxPEG decoder

Michael Niedermayer michaelni
Sun Nov 7 01:00:40 CET 2010


On Sun, Nov 07, 2010 at 01:51:47AM +0300, Anatoly Nenashev wrote:
> On 07.11.2010 01:25, Michael Niedermayer wrote:
>> On Sat, Nov 06, 2010 at 07:37:40AM +0300, Anatoly Nenashev wrote:
>>    
>>> On 06.11.2010 06:01, Michael Niedermayer wrote:
>>>      
>>>> On Thu, Nov 04, 2010 at 05:39:40PM +0300, Anatoly Nenashev wrote:
>>>>
>>>>        
>>>>> On 04.11.2010 03:16, Michael Niedermayer wrote:
>>>>>
>>>>>          
>>>>>> On Tue, Nov 02, 2010 at 01:58:21PM +0300, Anatoly Nenashev wrote:
>>>>>>
>>>>>>
>>>>>>            
>>>>>>>     int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
>>>>>>>     {
>>>>>>>         int len, nb_components, i, width, height, pix_fmt_id;
>>>>>>> @@ -342,14 +376,12 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
>>>>>>>                 s->avctx->pix_fmt = PIX_FMT_GRAY16;
>>>>>>>         }
>>>>>>>
>>>>>>> -    if(s->picture.data[0])
>>>>>>> -        s->avctx->release_buffer(s->avctx,&s->picture);
>>>>>>> -
>>>>>>> -    s->picture.reference= 0;
>>>>>>> -    if(s->avctx->get_buffer(s->avctx,&s->picture)<    0){
>>>>>>> -        av_log(s->avctx, AV_LOG_ERROR, "get_buffer() failed\n");
>>>>>>> -        return -1;
>>>>>>> +    if (s->avctx->codec_id == CODEC_ID_MXPEG&&    !s->got_picture) {
>>>>>>> +        if (mxpeg_allocate_picture(s)<    0) return -1;
>>>>>>> +    } else {
>>>>>>> +        if (mjpeg_allocate_picture(s)<    0) return -1;
>>>>>>>         }
>>>>>>>
>>>>>>>
>>>>>>>              
>>>>>> looks like s->picture.reference will b wrong for the first pic
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>> No, s->picture.reference will be always 1 for MxPEG frames. But I've
>>>>> reimplemented this code to be more clean. See attachment.
>>>>>
>>>>>          
>>>>>> [...]
>>>>>>
>>>>>>
>>>>>>            
>>>>>>> +static int mxpeg_decode_mxm(MJpegDecodeContext *s, char *buf, int len)
>>>>>>> +{
>>>>>>> +    int32_t  mb_width, mb_height, bitmask_size, i;
>>>>>>> +    uint32_t mb_count;
>>>>>>> +
>>>>>>> +    mb_width  = AV_RL16(&buf[4]);
>>>>>>> +    mb_height = AV_RL16(&buf[6]);
>>>>>>> +    mb_count = (uint32_t)mb_width*mb_height;
>>>>>>> +
>>>>>>> +    if (!mb_count || mb_count>    0x7FFFFFF8) {
>>>>>>> +        av_log(s->avctx, AV_LOG_ERROR, "MXM wrong macroblocks count");
>>>>>>> +        return AVERROR(EINVAL);
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    bitmask_size = (int32_t)(mb_count + 7)>>    3;
>>>>>>> +
>>>>>>> +    if (bitmask_size>    (len - 12)) {
>>>>>>> +        av_log(s->avctx, AV_LOG_ERROR, "MXM bitmask is not complete");
>>>>>>> +        return AVERROR(EINVAL);
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    av_freep(&s->mxctx.comment_buffer);
>>>>>>> +    s->mxctx.comment_buffer = buf;
>>>>>>> +    s->mxctx.mxm_bitmask = buf + 12;
>>>>>>> +
>>>>>>> +    if (!s->got_picture&&    s->picture.data[0]) {
>>>>>>> +        if (mxpeg_allocate_picture(s)<    0) return -1;
>>>>>>> +        s->got_picture = 1;
>>>>>>> +    }
>>>>>>>
>>>>>>>
>>>>>>>              
>>>>>> allocating pictures in the comment parsing code is not a good idea IMHO
>>>>>>
>>>>>>
>>>>>>            
>>>>> I don't agree.
>>>>> There are 3 types of frames in MxPEG stream:
>>>>> 1) clear JPEG which contains SOF, DHT, DQT, SOS  - may be the first
>>>>> frame in stream or (in new cameras firmware) may be periodically
>>>>> available.
>>>>> 2) Pseudo-I frame which contains SOF, DHT, DQT, MXM bitmask (in COM),
>>>>> SOS - must be periodically available but doesn't need to contain full
>>>>> parts of image.
>>>>> 3) P-frame which contains MXM bitmask, SOS and optional DHT, DQT - just
>>>>> usual frame.
>>>>>
>>>>> So where I must allocate picture for the last frame type? I've decided
>>>>> to do it in MXM parser.
>>>>>
>>>>>
>>>>>
>>>>>          
>>>>>> also maybe it would be easier to use reget_buffer() ?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>            
>>>>> No, I think reget_buffer is a bad idea. We don't need to copy all
>>>>> reference frame but only non-changed part of it.
>>>>> And we had talking about it in August. It was your idea to remove
>>>>> reget_buffer from this code to reject full image copying.
>>>>>
>>>>>          
>>>> both methods have their advantages and disadvantages.
>>>> i dont care which is implemented but it should be done cleanly
>>>> allocating the frame in the comment parsing code is not ok.
>>>> Allocating a picture currently implicates that a SOF has been successfully
>>>> parsed as the later code checks for this.
>>>> Simply allocating a picture without successfully running the SOF parsing but
>>>> maybe having it run just to the middle where it failed could lead to
>>>> exploitable security issues
>>>>
>>>>
>>>>
>>>>
>>>>        
>>> As I have specified before there are frames which don't contain SOF
>>> marker. I've named them P-frames.
>>> If we have got Pseudo-I frame before then new picture for P-frame will
>>> be allocated according to previously parsed params.
>>>      
>> you dont know if the previous params are valid. Its possible that frame
>> 1 had a correct SOF and frame 2 failed in the middle of SOF parsing and so
>> you have a mix a values now frame 3 has no SOF and uses this mix, this
>> looks bad to me
>>
>> Iam not sure how to best solve it but i dont think we can just pick a random
>> solution and hope it wont be exploitable
>>    
>>
>
> By the way, we have some information about picture parameters in P-frames.
> MXM data contains width and height of picture in macroblocks. So we can  
> check if them is the same as in previously parsed SOF.
> In this case SOS parser is a unique place in which there can be problems  
> with exploits. But we also can have same problems here with broken  
> streams in usual JPEG decoder. So what's the difference? But if you  

the difference is that if SOF fails or there is no SOF then got_picture=0
and later code will not continue


> insist, could you please approximately describe the strategy of such
> exploit.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/20101107/a6bbbca6/attachment.pgp>



More information about the ffmpeg-devel mailing list