Re: [FFmpeg-devel] [PATCH] Check return value from avio_read() to verify data actually read
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
> 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.
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] Check return value from avio_read() to verify data actually read
> > 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 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 > > 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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". 0002-Check-return-value-from-avio_read-to-verify-data-act.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] Check return value from avio_read() to verify data actually read
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 > 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 signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] Check return value from avio_read() to verify data actually read
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(). 0001-Check-return-value-from-avio_read-to-verify-data-act.patch Description: Binary data ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
