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

Updated to SIZE_MAX. Thank you for your comments.

On Wed, Dec 14, 2016 at 5:39 PM, Andreas Cadhalpun <
andreas.cadhalpun@googlemail.com> wrote:

> On 15.12.2016 00:36, Matthew Wolenetz wrote:
> > From 9d45f272a682b0ea831c20e36f696e15cc0c55fe Mon Sep 17 00:00:00 2001
> > From: Matt Wolenetz <wolenetz@chromium.org>
> > Date: Tue, 6 Dec 2016 12:33:08 -0800
> > Subject: [PATCH] lavf/mov.c: Avoid heap allocation wrap in mov_read_uuid
> >
> > Core of patch is from paul@paulmehta.com
> > Reference https://crbug.com/643951
> > ---
> >  libavformat/mov.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 7254505..e506d20 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -4393,6 +4393,8 @@ static int mov_read_uuid(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
> >      } else if (!memcmp(uuid, uuid_xmp, sizeof(uuid))) {
> >          uint8_t *buffer;
> >          size_t len = atom.size - sizeof(uuid);
> > +        if (len >= UINT_MAX)
>
> This should also use SIZE_MAX.
>
> > +            return AVERROR_INVALIDDATA;
> >
> >          buffer = av_mallocz(len + 1);
> >          if (!buffer) {
>
> Best regards,
> Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

On 08/02/17 00:09, Matthew Wolenetz wrote:
> From 1763ad5ae340e09081d8f50e867c2702cb5ec61e Mon Sep 17 00:00:00 2001
> From: Matt Wolenetz <wolenetz@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@paulmehta.com
> Reference https://crbug.com/643951
> 
> Signed-off-by: Matt Wolenetz <wolenetz@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);

(If there is additional information hidden behind the link which renders my comments invalid then please do share it directly.)

- Mark

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@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@paulmehta.com
> > Reference https://crbug.com/643951
> > 
> > Signed-off-by: Matt Wolenetz <wolenetz@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

[...]