[FFmpeg-devel] [PATCH 1/2] MxPEG decoder
Michael Niedermayer
michaelni
Thu Nov 25 16:26:47 CET 2010
On Mon, Nov 08, 2010 at 01:40:39PM +0300, Anatoly Nenashev wrote:
> On 07.11.2010 03:00, Michael Niedermayer wrote:
>> 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
>>
>>
>>
>
> I think I've found a solution for this issue. If input packet doesn't
> contain SOF data then the new picture is allocated from
> reference_picture which is initiated at decode_frame end. Thus
> reference_picture is always good. For more details see attachment.
the issue i described has not been fixed
a invalid SOF still can lead to inconsistant values and your code still naively
sets got_picture=1 indicating a valid SOF even if that is not so.
Fundamentally i think the problem is that you write the code while ignoring
security aspects entirely and expect review to find security issues.
You should make sure your code is secure and no crafted input no matter how
evil and malformed can lead to any crash or exploit before you submit your
code.
[...]
> @@ -342,10 +342,14 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
> s->avctx->pix_fmt = PIX_FMT_GRAY16;
> }
>
> - if(s->picture.data[0])
> + if (s->avctx->codec_id == CODEC_ID_MXPEG) {
> + memset(s->picture.data, 0, sizeof(s->picture.data));
this is a fragile hack and even worse so at the wrong place almost guranteeing
a memleak at some point.
when you implement exchanging current and last picture by copying the current
over the last then you at that point also must place all hacks that this
requires otherwise any "asymetry" between the 2 distant hacks would lead to
issues. (such asymetetry will be introduced by murphy one day for sure and
that is less likely if the code is together)
[...]
> +static int mxpeg_decode_mxm(MJpegDecodeContext *s, char *buf, int len)
> +{
> + int32_t bitmask_size, i;
> + uint32_t mb_count;
> +
> + av_freep(&s->mxctx.comment_buffer);
> + s->mxctx.comment_buffer = buf;
> + s->mxctx.mxm_bitmask = buf + 12;
> +
> + s->mxctx.mb_width = AV_RL16(&buf[4]);
> + s->mxctx.mb_height = AV_RL16(&buf[6]);
> + mb_count = (uint32_t)s->mxctx.mb_width * s->mxctx.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;
the cast is unneeded
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/20101125/d25c1d44/attachment.pgp>
More information about the ffmpeg-devel
mailing list