[FFmpeg-devel,4/6] avcodec/pngdec: Check amount decoded

Submitted by Michael Niedermayer on Aug. 17, 2019, 11:28 p.m.

Details

Message ID 20190817232841.14709-4-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Aug. 17, 2019, 11:28 p.m.
Fixes: Timeout (70sec -> 243ms)
Fixes: 16097/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-5664690889293824

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

Comments

Paul B Mahol Aug. 18, 2019, 7:21 a.m.
NAK

On Sun, Aug 18, 2019 at 1:36 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> Fixes: Timeout (70sec -> 243ms)
> Fixes:
> 16097/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-5664690889293824
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by
> <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>:
> Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/pngdec.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> index 4ca4f7bdc1..6e6856ab3e 100644
> --- a/libavcodec/pngdec.c
> +++ b/libavcodec/pngdec.c
> @@ -320,6 +320,15 @@ static void deloco_ ## NAME(TYPE *dst, int size, int
> alpha) \
>  YUV2RGB(rgb8, uint8_t)
>  YUV2RGB(rgb16, uint16_t)
>
> +static int percent_missing(PNGDecContext *s)
> +{
> +    if (s->interlace_type) {
> +        return 100 - 100 * s->pass / (NB_PASSES - 1);
> +    } else {
> +        return 100 - 100 * s->y / s->cur_h;
> +    }
> +}
> +
>  /* process exactly one decompressed row */
>  static void png_handle_row(PNGDecContext *s)
>  {
> @@ -1354,6 +1363,9 @@ exit_loop:
>          return 0;
>      }
>
> +    if (percent_missing(s) > avctx->discard_damaged_percentage)
> +        return AVERROR_INVALIDDATA;
> +
>      if (s->bits_per_pixel <= 4)
>          handle_small_bpp(s, p);
>
> --
> 2.22.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 Aug. 18, 2019, 8:25 a.m.
On Sun, Aug 18, 2019 at 09:21:25AM +0200, Paul B Mahol wrote:
> NAK

What problem do you see in this patch ?

What change do you suggest ?
or what alternative fix for the issue do you suggest ?

a DOS issue in png will have to be fixed, otherwise major
users would have to use different libraries for *png and
disable it in their libavcodec.

Thanks

[...]
Paul B Mahol Aug. 18, 2019, 9:04 a.m.
On Sun, Aug 18, 2019 at 10:25 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Sun, Aug 18, 2019 at 09:21:25AM +0200, Paul B Mahol wrote:
> > NAK
>
> What problem do you see in this patch ?
>
> What change do you suggest ?
> or what alternative fix for the issue do you suggest ?
>
> a DOS issue in png will have to be fixed, otherwise major
> users would have to use different libraries for *png and
> disable it in their libavcodec.
>

What other png libraries do this thing?


>
> Thanks
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> There will always be a question for which you do not know the correct
> answer.
> _______________________________________________
> 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 Sept. 2, 2019, 6:08 p.m.
On Sun, Aug 18, 2019 at 11:04:20AM +0200, Paul B Mahol wrote:
> On Sun, Aug 18, 2019 at 10:25 AM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Sun, Aug 18, 2019 at 09:21:25AM +0200, Paul B Mahol wrote:
> > > NAK
> >
> > What problem do you see in this patch ?
> >
> > What change do you suggest ?
> > or what alternative fix for the issue do you suggest ?
> >
> > a DOS issue in png will have to be fixed, otherwise major
> > users would have to use different libraries for *png and
> > disable it in their libavcodec.
> >
> 
> What other png libraries do this thing?

which libraries do you want me to check ?
libpng seems to support incremental decoding of images so that pixels
"sparkle in" as they are decoded. I dont think it rejects based on a
user parameter.
such a sparkle in feature doesnt exist in libavcodecs API in this
form

Iam not sure how this information above helps us with this patch

We can reject every image that has any pixel missing, this would
break decoding of real files probably which are truncated

We can reject no image based on missing pixels, this would make
the decoder more vulnerable to DOS attacks. 

We can honor the user parameter intended for this purpose and
reject images missing most pixels, this is what the patch does

We can hardcode some arbitrary threshold of how much can be
missing before rejection

We could do something completely different, but i need to know what
exactly is preferred

What does the community prefer ?

Thanks

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 4ca4f7bdc1..6e6856ab3e 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -320,6 +320,15 @@  static void deloco_ ## NAME(TYPE *dst, int size, int alpha) \
 YUV2RGB(rgb8, uint8_t)
 YUV2RGB(rgb16, uint16_t)
 
+static int percent_missing(PNGDecContext *s)
+{
+    if (s->interlace_type) {
+        return 100 - 100 * s->pass / (NB_PASSES - 1);
+    } else {
+        return 100 - 100 * s->y / s->cur_h;
+    }
+}
+
 /* process exactly one decompressed row */
 static void png_handle_row(PNGDecContext *s)
 {
@@ -1354,6 +1363,9 @@  exit_loop:
         return 0;
     }
 
+    if (percent_missing(s) > avctx->discard_damaged_percentage)
+        return AVERROR_INVALIDDATA;
+
     if (s->bits_per_pixel <= 4)
         handle_small_bpp(s, p);