[FFmpeg-devel] [PATCH] Check return value from avio_read() to verify data actually read

[Michael Niedermayer]

On Wed, Mar 25, 2020 at 02:39:19PM -0700, John Rummell wrote:
> >
> > These would cause mov_read_adrm() to fail but not neccessarily return an
> > error code if any of these reads less.
> > Is that intended ?
> 
> 
> Not at all. Updated to always return AVERROR_INVALIDDATA.
[...]

>  mov.c    |   34 ++++++++++++++++++++++++++--------
>  oggdec.c |    3 ++-
>  wavdec.c |   12 ++++++++----

This should be split in 3 patches


>  3 files changed, 36 insertions(+), 13 deletions(-)
> 771646822442ae7d4c0e00b350fbcc872cb15ab9  0002-Check-return-value-from-avio_read-to-verify-data-act.patch
> From 6751e6f594b0e0cba6fb0fbfdb7b0ab2c30c8512 Mon Sep 17 00:00:00 2001
> From: John Rummell <jrummell at chromium.org>
> Date: Mon, 23 Mar 2020 15:48:33 -0700
> Subject: [PATCH] Check return value from avio_read() to verify data actually
>  read
> 
> If the buffer doesn't contain enough bytes when reading a stream,
> fail rather than continuing on with unitialized data. One attempt
> caught by Chromium fuzzers (crbug.com/1054229), rest done by looking
> for calls to avio_read() that don't check the result in Chromium
> code search.

[...]
> @@ -1876,7 +1890,8 @@ static int mov_read_wave(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>                  AV_WB32(st->codecpar->extradata    , ALAC_EXTRADATA_SIZE);
>                  AV_WB32(st->codecpar->extradata + 4, MKTAG('a','l','a','c'));
>                  AV_WB64(st->codecpar->extradata + 12, buffer);
> -                avio_read(pb, st->codecpar->extradata + 20, 16);
> +                if (avio_read(pb, st->codecpar->extradata + 20, 16) != 16)
> +                    return AVERROR_INVALIDDATA;
>                  avio_skip(pb, atom.size - 24);
>                  return 0;

The commit message suggests that these fix uninitialized data
This hare as well as some others work on 0 filled arrays.
Its still good to check i think but for some of these
cases the clearing becomes redundant if a check is added so the clearing should
be removed then, also the commit message should be more clear that not all the
changed cases fix uninitialized data



>              }
> @@ -4376,7 +4391,8 @@ static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>          c->meta_keys[i] = av_mallocz(key_size + 1);
>          if (!c->meta_keys[i])
>              return AVERROR(ENOMEM);
> -        avio_read(pb, c->meta_keys[i], key_size);
> +        if (avio_read(pb, c->meta_keys[i], key_size) != key_size)
> +            return AVERROR_INVALIDDATA;
>      }
>  
>      return 0;

This too is cleared  

[...]

thx

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200327/aa4838b3/attachment.sig>