[FFmpeg-devel] [PATCH] libavcodec/vp9: fix ref-frame size judging method

[Xiang, Haihao]

On Thu, 2019-05-23 at 08:12 -0400, Ronald S. Bultje wrote:
Hi guys,

On Wed, May 22, 2019 at 11:14 PM 严小复 <mryancen at gmail.com<mailto:mryancen at gmail.com>> wrote:
code”, I'm little confused about the red word,would you mean encode process
need validity checks or there need to check the reference id of each frame?

And this patch will act on decode process, all references have already been
appointed by the stream.

No. As said before, the *decode* process needs such checks.

But since you don't seem to understand, let me try to be more helpful.

If you have an array of references, and we refer to that as s->s.refs[8], in which some reference is missing, e.g. s->s.refs[5] is NULL (but the rest is fine).

Do you mean s->s.refs[5].f ? s->s.refs[5] is not a pointer.

Now let's also say that we have a header in s->s.h in which there is an array of reference indices s->s.h.refidx[7] assigned as per the "spec". Let's imagine that we encounter a frame in which s->s.refs[5] is used as an active reference in this frame, for example s->s.h.refidx[0] = 5. Right now, with the code in git/master, we abort decoding. Your patch will make it continue decoding.

If so, even without Cen's patch, there is still a similar issue because the reference is used without any check in decode_frame_header () in vp9.c line 794-795

    AVFrame *ref = s->s.refs[s->s.h.refidx[i]].f;
   int refw = ref->width, refh = ref->height;

So I presumed s->s.refs[s->s.h.refidx[i]].f is assigned somewhere, otherwise we must add a check here.



So now, imagine that we encounter, in this frame, an inter block in which we use this reference (so b->ref[0] = 0, which means s->s.h.refidx[b->ref[0]] = 5), and have prediction code that does something like vp9_mc_template.c line 39:

    ThreadFrame *tref1 = &s->s.refs[s->s.h.refidx[b->ref[0]]];

Then later on this code will call a function which may in some cases be called mc_luma_dir() as per vp9_mc_template.c line 413, which is implemented in vp9recon.c line 298 in the function called mc_luma_unscaled() (see #define on line 383 of same file). This function now calls a DSP function in line 331:

mc[!!mx][!!my](dst, dst_stride, ref, ref_stride, bh, mx << 1, my << 1);

which directly accesses the reference pixels (see e.g. vp9dsp_template.c line 2044).

Note how especially *none of these functions* check anywhere whether s->s.refs[s->s.h.refidx[b->ref[0]]] (or tref1) is NULL. They don't do that, because ... the header check already did that, so the check was redundant.

But! You are *removing* that header check, so in this brave new world which will exist after applying your patch, what will happen is that I can craft a special stream where s->s.refs[5] becomes NULL, then send a subsequent frame using refs[5] by having s->s.h.refidx[0] = 5, then have a frame in which the block uses inter coding and uses reference 0 so that this causes access into s->s.refs[s->s.h.refidx[b->ref[0]]]], which is a NULL pointer, which is undefined behaviour by the C standard, which means a bad person could craft such a stream that would allow this bad person to break into your computer and steal all your data. In more straightforward cases, it might also crash Firefox and VLC, which use the FFmpeg VP9 decoder.

Note also that having your data stolen or your application crashing is considered *bad*. We *don't want that*. Therefore, as I've tried to say a few times already, if you remove the header check, you need to add a per-block check instead, so that Firefox/VLC don't crash and so that bad persons cannot steal your data.

Please add such a check and test using a fuzzer that the check prevents crashes as described above. Similar checks may be needed in other places also, since there's multiple places where the software decoder does MC and accesses references. Good luck finding these places by reading the code and fuzzing away.

HTH, and please ask more questions (on IRC please, #ffmpeg-devel on Freenode) if this is still unclear.
Ronald