[FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder

[Paul B Mahol]

On 11/27/19, Tomas Härdin <tjoppen at acc.umu.se> wrote:
> ons 2019-11-27 klockan 20:00 +0100 skrev Paul B Mahol:
>> On 11/27/19, James Almer <jamrial at gmail.com> wrote:
>> > On 11/27/2019 3:42 PM, Paul B Mahol wrote:
>> > > On 11/27/19, Tomas Härdin <tjoppen at acc.umu.se> wrote:
>> > > > tis 2019-11-26 klockan 20:29 +0100 skrev Paul B Mahol:
>> > > > > On 11/26/19, James Almer <jamrial at gmail.com> wrote:
>> > > > > > On 11/26/2019 6:47 AM, Paul B Mahol wrote:
>> > > > > > > On 11/25/19, Tomas Härdin <tjoppen at acc.umu.se> wrote:
>> > > > > > > > mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
>> > > > > > > > > Signed-off-by: Paul B Mahol <onemda at 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?
>>
>> Than next day he or you will come with another great idea.
>
> Great ideas like pushing inevitable 0days?

Very friendly reviews and developers.