diff mbox series

[FFmpeg-devel,2/2] avcodec/cdtoons: Fix off by 4 check on diff_size

Message ID 20200220185056.10900-2-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/2] avcodec/cdtoons: Correct several end of data checks in cdtoons_render_sprite()
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Michael Niedermayer Feb. 20, 2020, 6:50 p.m. UTC
Fixes: out of array read
Fixes: 20742/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CDTOONS_fuzzer-5738148607033344

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/cdtoons.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paul B Mahol Feb. 20, 2020, 7:11 p.m. UTC | #1
Are you sure this is correct?
Does asan reports exactly overread by 4?

On 2/20/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> Fixes: out of array read
> Fixes:
> 20742/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CDTOONS_fuzzer-5738148607033344
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/cdtoons.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/cdtoons.c b/libavcodec/cdtoons.c
> index dc4fa6bf0b..d5dce6351f 100644
> --- a/libavcodec/cdtoons.c
> +++ b/libavcodec/cdtoons.c
> @@ -269,7 +269,7 @@ static int cdtoons_decode_frame(AVCodecContext *avctx,
> void *data,
>                  diff_size  = bytestream_get_be32(&buf);
>                  width      = bytestream_get_be16(&buf);
>                  height     = bytestream_get_be16(&buf);
> -                if (diff_size < 4 || diff_size - 4 > eod - buf) {
> +                if (diff_size < 8 || diff_size - 4 > eod - buf) {
>                      av_log(avctx, AV_LOG_WARNING, "Ran (seriously) out of
> data for Diff frame data.\n");
>                      return AVERROR_INVALIDDATA;
>                  }
> --
> 2.17.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Feb. 20, 2020, 10:26 p.m. UTC | #2
On Thu, Feb 20, 2020 at 08:11:34PM +0100, Paul B Mahol wrote:
> Are you sure this is correct?
> Does asan reports exactly overread by 4?

the next line passes diff_size - 8 as a unsigned data size
if diff_size is smaller than 8, diff_size - 8 is very big and
the overread checks which use that will misbehave


[...]
> > @@ -269,7 +269,7 @@ static int cdtoons_decode_frame(AVCodecContext *avctx,
> > void *data,
> >                  diff_size  = bytestream_get_be32(&buf);
> >                  width      = bytestream_get_be16(&buf);
> >                  height     = bytestream_get_be16(&buf);
> > -                if (diff_size < 4 || diff_size - 4 > eod - buf) {
> > +                if (diff_size < 8 || diff_size - 4 > eod - buf) {
> >                      av_log(avctx, AV_LOG_WARNING, "Ran (seriously) out of
> > data for Diff frame data.\n");
> >                      return AVERROR_INVALIDDATA;
> >                  }

[...]
Paul B Mahol Feb. 20, 2020, 10:29 p.m. UTC | #3
On 2/20/20, Michael Niedermayer <michaelni@gmx.at> wrote:
> On Thu, Feb 20, 2020 at 08:11:34PM +0100, Paul B Mahol wrote:
>> Are you sure this is correct?
>> Does asan reports exactly overread by 4?
>
> the next line passes diff_size - 8 as a unsigned data size
> if diff_size is smaller than 8, diff_size - 8 is very big and
> the overread checks which use that will misbehave
>

OK then.

>
> [...]
>> > @@ -269,7 +269,7 @@ static int cdtoons_decode_frame(AVCodecContext
>> > *avctx,
>> > void *data,
>> >                  diff_size  = bytestream_get_be32(&buf);
>> >                  width      = bytestream_get_be16(&buf);
>> >                  height     = bytestream_get_be16(&buf);
>> > -                if (diff_size < 4 || diff_size - 4 > eod - buf) {
>> > +                if (diff_size < 8 || diff_size - 4 > eod - buf) {
>> >                      av_log(avctx, AV_LOG_WARNING, "Ran (seriously) out
>> > of
>> > data for Diff frame data.\n");
>> >                      return AVERROR_INVALIDDATA;
>> >                  }
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The misfortune of the wise is better than the prosperity of the fool.
> -- Epicurus
>
Michael Niedermayer Feb. 21, 2020, 8:38 p.m. UTC | #4
On Thu, Feb 20, 2020 at 11:29:51PM +0100, Paul B Mahol wrote:
> On 2/20/20, Michael Niedermayer <michaelni@gmx.at> wrote:
> > On Thu, Feb 20, 2020 at 08:11:34PM +0100, Paul B Mahol wrote:
> >> Are you sure this is correct?
> >> Does asan reports exactly overread by 4?
> >
> > the next line passes diff_size - 8 as a unsigned data size
> > if diff_size is smaller than 8, diff_size - 8 is very big and
> > the overread checks which use that will misbehave
> >
> 
> OK then.

will apply

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/cdtoons.c b/libavcodec/cdtoons.c
index dc4fa6bf0b..d5dce6351f 100644
--- a/libavcodec/cdtoons.c
+++ b/libavcodec/cdtoons.c
@@ -269,7 +269,7 @@  static int cdtoons_decode_frame(AVCodecContext *avctx, void *data,
                 diff_size  = bytestream_get_be32(&buf);
                 width      = bytestream_get_be16(&buf);
                 height     = bytestream_get_be16(&buf);
-                if (diff_size < 4 || diff_size - 4 > eod - buf) {
+                if (diff_size < 8 || diff_size - 4 > eod - buf) {
                     av_log(avctx, AV_LOG_WARNING, "Ran (seriously) out of data for Diff frame data.\n");
                     return AVERROR_INVALIDDATA;
                 }