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

[wm4]

On Thu, 25 Feb 2016 22:39:51 +0100
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> 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.

Well, coded_width is supposed to be the actual frame size, while width
applies cropping. So width should always be <= coded_width. For codecs
where this should not hold up for some reason, coded_width is obviously
inappropriate to use in the first place.

> 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.

That sounds scary.

> 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.