[FFmpeg-devel] [PATCH] lavf/mov.c: Avoid heap allocation wrap in mov_read_uuid

[Michael Niedermayer]

On Wed, Feb 08, 2017 at 01:25:58AM +0000, Mark Thompson wrote:
> On 08/02/17 00:09, Matthew Wolenetz wrote:
> > From 1763ad5ae340e09081d8f50e867c2702cb5ec61e Mon Sep 17 00:00:00 2001
> > From: Matt Wolenetz <wolenetz at google.com>
> > Date: Wed, 14 Dec 2016 15:26:19 -0800
> > Subject: [PATCH] lavf/mov.c: Avoid heap allocation wrap in mov_read_uuid
> > 
> > Core of patch is from paul at paulmehta.com
> > Reference https://crbug.com/643951
> > 
> > Signed-off-by: Matt Wolenetz <wolenetz at chromium.org>
> > ---
> >  libavformat/mov.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 6fd43a0a4e..93aece510c 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -4849,6 +4849,8 @@ static int mov_read_uuid(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >          uint8_t *buffer;
> >          size_t len = atom.size - sizeof(uuid);
> >          if (c->export_xmp) {
> > +            if (len >= SIZE_MAX)
> > +                return AVERROR_INVALIDDATA;
> >              buffer = av_mallocz(len + 1);
> >              if (!buffer) {
> >                  return AVERROR(ENOMEM);
> > -- 
> > 2.11.0.483.g087da7b7c-goog
> 
> I don't know precisely what this is trying to do, but it doesn't look very sensible to me (I can't see any context because your reference link is blocked behind some sort of login page).
> 
> Since SIZE_MAX is the largest size_t (i.e. -1), only len == SIZE_MAX can satisfy the test so it is actually "if (atom.size == sizeof(uuid) - 1)", and if atom.size is something less than sizeof(uuid) - 1 then you are passing a silly value to av_mallocz().
> 
> I admit that it doesn't actually invoke undefined behaviour in this case, but the general rules for overflow checks to avoid any surprises are:
> (a) Always put the check before the operation which might overflow.
> (b) Always express the check in a way which cannot itself overflow.
> 
> So, I think you would be better off writing:
> 
> size_t len;
> if (atom.size < sizeof(uuid))
>     return AVERROR_INVALIDDATA;
> len = atom.size - sizeof(uuid);

that alone is not enough

ill push a fix which appears to be sufficent, dont want to leave either
of these open while its discussed whats the best solution

if theres some issue in my fixes dont hesitate to improve or replace,
ill go to bed after testing and pushing my fixes

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are best at talking, realize last or never when they are wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170208/c31fb107/attachment.sig>