On 11/27/2019 3:42 PM, Paul B Mahol wrote:
> On 11/27/19, Tomas Härdin <tjop...@acc.umu.se> wrote:
>> tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol:
>>> On 11/26/19, James Almer <jamr...@gmail.com> wrote:
>>>> On 11/26/2019 6:47 AM, Paul B Mahol wrote:
>>>>> On 11/25/19, Tomas Härdin <tjop...@acc.umu.se> wrote:
>>>>>> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
>>>>>>> Signed-off-by: Paul B Mahol <one...@gmail.com>
>>>>>>> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx,
>>>>>>> AVFrame
>>>>>>> *frame)
>>>>>>> +{
>>>>>>> + GetByteContext *gb = &s->gb;
>>>>>>> + GetBitContext mask;
>>>>>>> + GetByteContext idx9;
>>>>>>> + uint16_t nb_vectors, intra_flag;
>>>>>>> + const uint8_t *vec;
>>>>>>> + const uint8_t *mask_start;
>>>>>>> + uint8_t *skip;
>>>>>>> + int mask_size;
>>>>>>> + int idx9bits = 0;
>>>>>>> + int idx9val = 0;
>>>>>>> + int num_blocks;
>>>>>>> +
>>>>>>> + nb_vectors = bytestream2_get_le16(gb);
>>>>>>> + intra_flag = bytestream2_get_le16(gb);
>>>>>>> + if (intra_flag) {
>>>>>>> + num_blocks = (avctx->width / 2) * (avctx->height / 2);
>>>>>>
>>>>>> Will UB if width*height/4 > INT_MAX
>>>>>>
>>>>>>> + } else {
>>>>>>> + int skip_linesize;
>>>>>>> +
>>>>>>> + num_blocks = bytestream2_get_le32(gb);
>>>>>>
>>>>>> Might want to use uint32_t so this doesn't lead to weirdness on
>>>>>> 32-bit
>>>>>>
>>>>>>> + skip_linesize = avctx->width >> 1;
>>>>>>> + mask_start = gb->buffer_start + bytestream2_tell(gb);
>>>>>>> + mask_size = (avctx->width >> 5) * (avctx->height >> 2);
>>>>>>
>>>>>> This can also UB
>>>>>>
>>>>>> /Tomas
>>>>>>
>>>>>
>>>>> Nothing of this can actually happen.
>>
>> This assumes max_pixels will never be increased above INT_MAX. "64k"
>> video is most definitely within the range of possibility in the coming
>> years, if it isn't already. Film archival and DPX come to mind.
>>
>>>> It can and i'm fairly sure it will happen as soon as the fuzzer starts
>>>> testing this decoder using big dimensions.
>>>
>>> I'm not that guy doing such work. Please stop bikesheding those
>>> patches for once.
>>
>> This reads like an admission of pushing insecure code via "not my
>> problem"
>>
>>>> You don't need asserts here, you just need to check the calculations
>>>> will not overflow. Do something like "if ((int64_t)avctx->width *
>>>> avctx->height / 4 > INT_MAX) return AVERROR_INVALIDDATA" and call it a
>>>> day.
>>>> Also, maybe num_blocks should be unsigned, seeing you set it using
>>>> bytestream2_get_le32() for P-frames.
>>>
>>> No decoder does this.
>>
>> zmbv does
>>
>> All this is really about the lack of any way to reason about
>> assumptions like "dimensions can't be larger than X*Y" at compile time,
>> which is a thing I've been pointing out on this list for a while now.
>>
>
> Nobody tells you not to fix it yourself.
Just add a the suggested overflow checks, Christ. It's a one line
addition each. What do you gain arguing like this?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
