| Message ID | 20190817232841.14709-4-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
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".
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
[...]
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".
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 [...]
On Sun, Aug 18, 2019 at 01:28:39AM +0200, Michael Niedermayer 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: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/pngdec.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) There are now 4 testcases this patch fixes: Fixes: 16097/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-5664690889293824 Fixes: 16927/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-5170612070252544 Fixes: 16927/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-5706325622784000 Fixes: 18705/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-5650989302677504 [...]
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);
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(+)