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

[John Rummell]

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

On Tue, Mar 24, 2020 at 6:00 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Mon, Mar 23, 2020 at 05:52:01PM -0700, John Rummell wrote:
> > Chromium fuzzers have caught places where uninitialized data was used due
> > to calls to avio_read() not verifying that the number of bytes expected
> was
> > actually read. So updating the code to check the result from avio_read().
>
> >  mov.c    |   26 ++++++++++++++++++--------
> >  oggdec.c |    3 ++-
> >  wavdec.c |   12 ++++++++----
> >  3 files changed, 28 insertions(+), 13 deletions(-)
> > 66938bc5adfc7d151b376f6d396c4a0dc7f97a4c
> 0001-Check-return-value-from-avio_read-to-verify-data-act.patch
> > From 7f80d50711486a4b923bd8d1e26abc9649d570e3 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.
> > ---
> >  libavformat/mov.c    | 26 ++++++++++++++++++--------
> >  libavformat/oggdec.c |  3 ++-
> >  libavformat/wavdec.c | 12 ++++++++----
> >  3 files changed, 28 insertions(+), 13 deletions(-)
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index f280f360b6..a5b4d04e37 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -1012,10 +1012,16 @@ static int mov_read_adrm(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
> >      }
> >
> >      /* drm blob processing */
> > -    avio_read(pb, output, 8); // go to offset 8, absolute position 0x251
> > -    avio_read(pb, input, DRM_BLOB_SIZE);
> > -    avio_read(pb, output, 4); // go to offset 4, absolute position 0x28d
> > -    avio_read(pb, file_checksum, 20);
> > +    /* go to offset 8, absolute position 0x251 */
> > +    if ((ret = avio_read(pb, output, 8)) != 8)
> > +        goto fail;
> > +    if ((ret = avio_read(pb, input, DRM_BLOB_SIZE)) != DRM_BLOB_SIZE)
> > +        goto fail;
> > +    /* go to offset 4, absolute position 0x28d */
> > +    if ((ret = avio_read(pb, output, 4)) != 4)
> > +        goto fail;
> > +    if ((ret = avio_read(pb, file_checksum, 20)) != 20)
> > +        goto fail;
>
> These would cause mov_read_adrm() to fail but not neccessarily return an
> error code if any of these reads less.
> Is that intended ?
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Complexity theory is the science of finding the exact solution to an
> approximation. Benchmarking OTOH is finding an approximation of the exact
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Check-return-value-from-avio_read-to-verify-data-act.patch
Type: application/octet-stream
Size: 6172 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200325/26eafdc9/attachment.obj>