[FFmpeg-devel] [PATCH] mss2: Fix buffer overflow.

[Reimar Döffinger]

On Thu, Feb 25, 2016 at 09:25:08PM +0100, wm4 wrote:
> On Thu, 25 Feb 2016 21:06:46 +0100
> Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> 
> > Reported as https://trac.mplayerhq.hu/ticket/2264 but have
> > not been able to reproduce with FFmpeg-only.
> > I have no idea what coded_height is used for here exactly,
> > so this might not be the best fix.
> > Fixes the following chain of events:
> > ff_mss12_decode_init sets coded_height while not setting height.
> > ff_mpv_decode_init then copies coded_height into MpegEncContext height.
> > This is then used by init_context_frame to allocate the data structures.
> > However the wmv9rects are validated/initialized based on avctx->height, not
> > avctx->coded_height.
> > Thus the decode_wmv9 function will try to decode a larger video that we
> > allocated data structures for, causing out-of-bounds writes.
> > 
> > Signed-off-by: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
> > ---
> >  libavcodec/mss12.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/mss12.c b/libavcodec/mss12.c
> > index 6b58aa29..d42093b 100644
> > --- a/libavcodec/mss12.c
> > +++ b/libavcodec/mss12.c
> > @@ -581,8 +581,8 @@ av_cold int ff_mss12_decode_init(MSS12Context *c, int version,
> >          return AVERROR_INVALIDDATA;
> >      }
> >  
> > -    avctx->coded_width  = AV_RB32(avctx->extradata + 20);
> > -    avctx->coded_height = AV_RB32(avctx->extradata + 24);
> > +    avctx->coded_width  = FFMAX(AV_RB32(avctx->extradata + 20), avctx->width);
> > +    avctx->coded_height = FFMAX(AV_RB32(avctx->extradata + 24), avctx->height);
> >      if (avctx->coded_width > 4096 || avctx->coded_height > 4096) {
> >          av_log(avctx, AV_LOG_ERROR, "Frame dimensions %dx%d too large",
> >                 avctx->coded_width, avctx->coded_height);
> 
> Just a side remark...
> 
> I'm wondering why ff_set_dimensions doesn't have coded_width/height
> arguments and checks them. I bet this situation happens often.

I am not sure there is a fixed requirement for the relation
between coded_height and height that necessarily applies
to all codecs.
For example I don't know if for mss1 that case I am filtering
out is actually invalid! In which case the FFMAX might
need to go into mss2.c instead...
Note that this mostly happens because the mss2 codec essentially
instantiates multiple (partial) decoders into a single avctx,
and they might have differing requirements.
Though I admit I am not sure the mss12 code can handle this
case correctly either.
I guess in that way checking it might be good since it means
you at least don't have to guess if the check was just forgotten
or left out intentionally.