[PATCH 1/5] avcodec/pngdec: Check last AVFrame before deref
Fixes: NULL pointer dereference Fixes: 68184/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-4926478069334016 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c index f7751223b81..b24bfa248dc 100644 --- a/libavcodec/pngdec.c +++ b/libavcodec/pngdec.c @@ -1218,7 +1218,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s, return AVERROR_INVALIDDATA; } - if ((sequence_number == 0 || !s->last_picture.f->data[0]) && + if ((sequence_number == 0 || !s->last_picture.f || !s->last_picture.f->data[0]) && dispose_op == APNG_DISPOSE_OP_PREVIOUS) { // No previous frame to revert to for the first frame // Spec says to just treat it as a APNG_DISPOSE_OP_BACKGROUND -- 2.43.2
Fixes: Assertion !f->f && !f->progress failed at libavcodec/decode.c:1688 Fixes: 68190/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_THEORA_fuzzer-5942090287611904 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vp3.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c index 2a5f68dfa8e..09527607767 100644 --- a/libavcodec/vp3.c +++ b/libavcodec/vp3.c @@ -2651,6 +2651,7 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame, if (avctx->skip_frame >= AVDISCARD_NONKEY && !s->keyframe) return buf_size; + ff_progress_frame_unref(&s->current_frame); ret = ff_progress_frame_get_buffer(avctx, &s->current_frame, AV_GET_BUFFER_FLAG_REF); if (ret < 0) { -- 2.43.2
Michael Niedermayer:
Fixes: Assertion !f->f && !f->progress failed at libavcodec/decode.c:1688 Fixes: 68190/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_THEORA_fuzzer-5942090287611904
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vp3.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c index 2a5f68dfa8e..09527607767 100644 --- a/libavcodec/vp3.c +++ b/libavcodec/vp3.c @@ -2651,6 +2651,7 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame, if (avctx->skip_frame >= AVDISCARD_NONKEY && !s->keyframe) return buf_size;
+ ff_progress_frame_unref(&s->current_frame); ret = ff_progress_frame_get_buffer(avctx, &s->current_frame, AV_GET_BUFFER_FLAG_REF); if (ret < 0) {
LGTM. (If I am not mistaken, this would have triggered the av_log(avctx, AV_LOG_ERROR, "pic->data[*]!=NULL in get_buffer_internal\n") codepath before switching to ProgressFrames, i.e. using an assert instead of erroring out uncovered a bug. Maybe we should use an assert in ff_get_buffer(), too?) - Andreas
Hi On Sat, Apr 27, 2024 at 11:47:32AM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: Assertion !f->f && !f->progress failed at libavcodec/decode.c:1688 Fixes: 68190/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_THEORA_fuzzer-5942090287611904
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vp3.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c index 2a5f68dfa8e..09527607767 100644 --- a/libavcodec/vp3.c +++ b/libavcodec/vp3.c @@ -2651,6 +2651,7 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame, if (avctx->skip_frame >= AVDISCARD_NONKEY && !s->keyframe) return buf_size;
+ ff_progress_frame_unref(&s->current_frame); ret = ff_progress_frame_get_buffer(avctx, &s->current_frame, AV_GET_BUFFER_FLAG_REF); if (ret < 0) {
LGTM.
will apply thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator
Fixes: NULL pointer dereference Fixes: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/decode.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libavcodec/decode.c b/libavcodec/decode.c index d031b1ca176..a6131941f43 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -1744,6 +1744,8 @@ void ff_progress_frame_report(ProgressFrame *f, int n) void ff_progress_frame_await(const ProgressFrame *f, int n) { + if (!f->progress) + return; ff_thread_progress_await(&f->progress->progress, n); } -- 2.43.2
Michael Niedermayer:
Fixes: NULL pointer dereference Fixes: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/decode.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index d031b1ca176..a6131941f43 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -1744,6 +1744,8 @@ void ff_progress_frame_report(ProgressFrame *f, int n)
void ff_progress_frame_await(const ProgressFrame *f, int n) { + if (!f->progress) + return; ff_thread_progress_await(&f->progress->progress, n); }
Can I get the sample? I see two places in VP8 where the VP8Frame pointers are set before the actual frame inside it is properly allocated. (Actually, it was intended for this API to not support waiting on non-existent frames (i.e. let the caller check for this; in most instances, it is already guaranteed that the frame one waits one exists, so this is unnecessary for them).) - Andreas
On Sat, Apr 27, 2024 at 01:13:54PM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: NULL pointer dereference Fixes: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/decode.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index d031b1ca176..a6131941f43 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -1744,6 +1744,8 @@ void ff_progress_frame_report(ProgressFrame *f, int n)
void ff_progress_frame_await(const ProgressFrame *f, int n) { + if (!f->progress) + return; ff_thread_progress_await(&f->progress->progress, n); }
Can I get the sample? I see two places in VP8 where the VP8Frame pointers are set before the actual frame inside it is properly allocated.
(Actually, it was intended for this API to not support waiting on non-existent frames (i.e. let the caller check for this; in most instances, it is already guaranteed that the frame one waits one exists, so this is unnecessary for them).)
did you receive the sample i sent you in april ? any update on this ? its still crashing without this patch Running: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904 libavcodec/decode.c:1766:44: runtime error: member access within null pointer of type 'struct ProgressInternal' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/decode.c:1766:44 in libavcodec/threadprogress.c:72:36: runtime error: member access within null pointer of type 'ThreadProgress' (aka 'struct ThreadProgress') SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/threadprogress.c:72:36 in thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are too smart to engage in politics are punished by being governed by those who are dumber. -- Plato
Michael Niedermayer:
On Sat, Apr 27, 2024 at 01:13:54PM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: NULL pointer dereference Fixes: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/decode.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index d031b1ca176..a6131941f43 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -1744,6 +1744,8 @@ void ff_progress_frame_report(ProgressFrame *f, int n)
void ff_progress_frame_await(const ProgressFrame *f, int n) { + if (!f->progress) + return; ff_thread_progress_await(&f->progress->progress, n); }
Can I get the sample? I see two places in VP8 where the VP8Frame pointers are set before the actual frame inside it is properly allocated.
(Actually, it was intended for this API to not support waiting on non-existent frames (i.e. let the caller check for this; in most instances, it is already guaranteed that the frame one waits one exists, so this is unnecessary for them).)
did you receive the sample i sent you in april ?
any update on this ? its still crashing without this patch
Running: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904 libavcodec/decode.c:1766:44: runtime error: member access within null pointer of type 'struct ProgressInternal' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/decode.c:1766:44 in libavcodec/threadprogress.c:72:36: runtime error: member access within null pointer of type 'ThreadProgress' (aka 'struct ThreadProgress') SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/threadprogress.c:72:36 in
Totally forgot about this. Will look into it. Thanks for the reminder. - Andreas
On Tue, Jun 25, 2024 at 09:51:32PM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
On Sat, Apr 27, 2024 at 01:13:54PM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: NULL pointer dereference Fixes: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/decode.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index d031b1ca176..a6131941f43 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -1744,6 +1744,8 @@ void ff_progress_frame_report(ProgressFrame *f, int n)
void ff_progress_frame_await(const ProgressFrame *f, int n) { + if (!f->progress) + return; ff_thread_progress_await(&f->progress->progress, n); }
Can I get the sample? I see two places in VP8 where the VP8Frame pointers are set before the actual frame inside it is properly allocated.
(Actually, it was intended for this API to not support waiting on non-existent frames (i.e. let the caller check for this; in most instances, it is already guaranteed that the frame one waits one exists, so this is unnecessary for them).)
did you receive the sample i sent you in april ?
any update on this ? its still crashing without this patch
Running: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904 libavcodec/decode.c:1766:44: runtime error: member access within null pointer of type 'struct ProgressInternal' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/decode.c:1766:44 in libavcodec/threadprogress.c:72:36: runtime error: member access within null pointer of type 'ThreadProgress' (aka 'struct ThreadProgress') SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/threadprogress.c:72:36 in
Totally forgot about this. Will look into it. Thanks for the reminder.
Hello ? this issue is still open thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway
Fixes: NULL pointer dereferences Fixes: 68197/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6382538823106560 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/hevcdec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c index fcfb275f63a..92b0e45eee0 100644 --- a/libavcodec/hevcdec.c +++ b/libavcodec/hevcdec.c @@ -1969,13 +1969,13 @@ static void hls_prediction_unit(HEVCLocalContext *lc, int x0, int y0, if (current_mv.pred_flag & PF_L0) { ref0 = refPicList[0].ref[current_mv.ref_idx[0]]; - if (!ref0 || !ref0->frame->data[0]) + if (!ref0 || !ref0->frame || !ref0->frame->data[0]) return; hevc_await_progress(s, ref0, ¤t_mv.mv[0], y0, nPbH); } if (current_mv.pred_flag & PF_L1) { ref1 = refPicList[1].ref[current_mv.ref_idx[1]]; - if (!ref1 || !ref1->frame->data[0]) + if (!ref1 || !ref1->frame || !ref1->frame->data[0]) return; hevc_await_progress(s, ref1, ¤t_mv.mv[1], y0, nPbH); } -- 2.43.2
Michael Niedermayer:
Fixes: NULL pointer dereferences Fixes: 68197/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6382538823106560
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/hevcdec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c index fcfb275f63a..92b0e45eee0 100644 --- a/libavcodec/hevcdec.c +++ b/libavcodec/hevcdec.c @@ -1969,13 +1969,13 @@ static void hls_prediction_unit(HEVCLocalContext *lc, int x0, int y0,
if (current_mv.pred_flag & PF_L0) { ref0 = refPicList[0].ref[current_mv.ref_idx[0]]; - if (!ref0 || !ref0->frame->data[0]) + if (!ref0 || !ref0->frame || !ref0->frame->data[0]) return; hevc_await_progress(s, ref0, ¤t_mv.mv[0], y0, nPbH); } if (current_mv.pred_flag & PF_L1) { ref1 = refPicList[1].ref[current_mv.ref_idx[1]]; - if (!ref1 || !ref1->frame->data[0]) + if (!ref1 || !ref1->frame || !ref1->frame->data[0]) return; hevc_await_progress(s, ref1, ¤t_mv.mv[1], y0, nPbH); }
Same as with 1/5: Checking for !ref0->frame is enough as HEVCFrame.f is set if and only if the HEVCFrame.f->data[0] is set (with the possible exception of hw-accelerated pixel formats that don't use AVFrame.data at all (I don't know whether they exist); in any case, HEVCFrame.f is set if and only if HEVCFrame.f->buf[0] is set). Actually, I checked all the decoder that I ported to ProgressFrames for this pattern, but apparently I overlooked way too much (maybe I only checked for the ->buf[0] pattern?). Sorry for that. - Andreas
On Sat, Apr 27, 2024 at 12:14:18PM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: NULL pointer dereferences Fixes: 68197/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6382538823106560
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/hevcdec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c index fcfb275f63a..92b0e45eee0 100644 --- a/libavcodec/hevcdec.c +++ b/libavcodec/hevcdec.c @@ -1969,13 +1969,13 @@ static void hls_prediction_unit(HEVCLocalContext *lc, int x0, int y0,
if (current_mv.pred_flag & PF_L0) { ref0 = refPicList[0].ref[current_mv.ref_idx[0]]; - if (!ref0 || !ref0->frame->data[0]) + if (!ref0 || !ref0->frame || !ref0->frame->data[0]) return; hevc_await_progress(s, ref0, ¤t_mv.mv[0], y0, nPbH); } if (current_mv.pred_flag & PF_L1) { ref1 = refPicList[1].ref[current_mv.ref_idx[1]]; - if (!ref1 || !ref1->frame->data[0]) + if (!ref1 || !ref1->frame || !ref1->frame->data[0]) return; hevc_await_progress(s, ref1, ¤t_mv.mv[1], y0, nPbH); }
Same as with 1/5: Checking for !ref0->frame is enough as HEVCFrame.f is set if and only if the HEVCFrame.f->data[0] is set (with the possible
will apply with this simplification thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Into a blind darkness they enter who follow after the Ignorance, they as if into a greater darkness enter who devote themselves to the Knowledge alone. -- Isha Upanishad
Fixes: memleak Fixes: 68212/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-4963488540721152 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mov.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libavformat/mov.c b/libavformat/mov.c index 97a24e6737e..5b8278f736e 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -8136,6 +8136,9 @@ static int mov_read_infe(MOVContext *c, AVIOContext *pb, MOVAtom atom, int idx) avio_rb24(pb); // flags. size -= 4; + if (c->heif_item[idx].name) + return AVERROR_INVALIDDATA; + if (version < 2) { avpriv_report_missing_feature(c->fc, "infe version < 2"); return 1; -- 2.43.2
On 4/26/2024 8:52 PM, Michael Niedermayer wrote:
Fixes: memleak Fixes: 68212/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-4963488540721152
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mov.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/libavformat/mov.c b/libavformat/mov.c index 97a24e6737e..5b8278f736e 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -8136,6 +8136,9 @@ static int mov_read_infe(MOVContext *c, AVIOContext *pb, MOVAtom atom, int idx) avio_rb24(pb); // flags. size -= 4;
+ if (c->heif_item[idx].name) + return AVERROR_INVALIDDATA;
I prefer to free the old one before the av_bprint_finalize() call instead of aborting.
+ if (version < 2) { avpriv_report_missing_feature(c->fc, "infe version < 2"); return 1;
On Fri, Apr 26, 2024 at 08:57:02PM -0300, James Almer wrote:
On 4/26/2024 8:52 PM, Michael Niedermayer wrote:
Fixes: memleak Fixes: 68212/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-4963488540721152
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mov.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/libavformat/mov.c b/libavformat/mov.c index 97a24e6737e..5b8278f736e 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -8136,6 +8136,9 @@ static int mov_read_infe(MOVContext *c, AVIOContext *pb, MOVAtom atom, int idx) avio_rb24(pb); // flags. size -= 4; + if (c->heif_item[idx].name) + return AVERROR_INVALIDDATA;
I prefer to free the old one before the av_bprint_finalize() call instead of aborting.
does the format allow this to be changing ? because security wise, allowing an attacker to change things is worse than allowing her to just set it once. and heif seems to have a rather high density of issues already, judging from how many of the recent mov issues where in heif code thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship naturally arises out of democracy, and the most aggravated form of tyranny and slavery out of the most extreme liberty. -- Plato
On 4/26/2024 9:03 PM, Michael Niedermayer wrote:
On Fri, Apr 26, 2024 at 08:57:02PM -0300, James Almer wrote:
On 4/26/2024 8:52 PM, Michael Niedermayer wrote:
Fixes: memleak Fixes: 68212/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-4963488540721152
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mov.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/libavformat/mov.c b/libavformat/mov.c index 97a24e6737e..5b8278f736e 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -8136,6 +8136,9 @@ static int mov_read_infe(MOVContext *c, AVIOContext *pb, MOVAtom atom, int idx) avio_rb24(pb); // flags. size -= 4; + if (c->heif_item[idx].name) + return AVERROR_INVALIDDATA;
I prefer to free the old one before the av_bprint_finalize() call instead of aborting.
does the format allow this to be changing ?
This code is not supposed to be invoked twice if an iinf box is correctly parsed. It can only happen if a second iinf box shows up after the first and demuxing wasn't aborted, either because the error was ignored, or an infe box within was version < 2.
because security wise, allowing an attacker to change things is worse than allowing her to just set it once.
A second iinf box could show up after the first was not finished parsing, and as long as it has no names for the items, it would still finish parsing even after this patch. So better just clean old values and keep parsing. Further sanity checks will happen when all the items are turned into something meant to be exported.
and heif seems to have a rather high density of issues already, judging from how many of the recent mov issues where in heif code
thx
[...]
_______________________________________________ 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 4/26/2024 9:23 PM, James Almer wrote:
On 4/26/2024 9:03 PM, Michael Niedermayer wrote:
On Fri, Apr 26, 2024 at 08:57:02PM -0300, James Almer wrote:
On 4/26/2024 8:52 PM, Michael Niedermayer wrote:
Fixes: memleak Fixes: 68212/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-4963488540721152
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mov.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/libavformat/mov.c b/libavformat/mov.c index 97a24e6737e..5b8278f736e 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -8136,6 +8136,9 @@ static int mov_read_infe(MOVContext *c, AVIOContext *pb, MOVAtom atom, int idx) avio_rb24(pb); // flags. size -= 4; + if (c->heif_item[idx].name) + return AVERROR_INVALIDDATA;
I prefer to free the old one before the av_bprint_finalize() call instead of aborting.
does the format allow this to be changing ?
This code is not supposed to be invoked twice if an iinf box is correctly parsed. It can only happen if a second iinf box shows up after the first and demuxing wasn't aborted, either because the error was ignored, or an infe box within was version < 2.
because security wise, allowing an attacker to change things is worse than allowing her to just set it once.
A second iinf box could show up after the first was not finished parsing, and as long as it has no names for the items, it would still finish parsing even after this patch. So better just clean old values and keep parsing. Further sanity checks will happen when all the items are turned into something meant to be exported.
and heif seems to have a rather high density of issues already, judging from how many of the recent mov issues where in heif code
thx
Can you test https://ffmpeg.org//pipermail/ffmpeg-devel/2024-April/326317.html ?
Michael Niedermayer:
Fixes: NULL pointer dereference Fixes: 68184/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-4926478069334016
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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c index f7751223b81..b24bfa248dc 100644 --- a/libavcodec/pngdec.c +++ b/libavcodec/pngdec.c @@ -1218,7 +1218,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s, return AVERROR_INVALIDDATA; }
- if ((sequence_number == 0 || !s->last_picture.f->data[0]) && + if ((sequence_number == 0 || !s->last_picture.f || !s->last_picture.f->data[0]) && dispose_op == APNG_DISPOSE_OP_PREVIOUS) { // No previous frame to revert to for the first frame // Spec says to just treat it as a APNG_DISPOSE_OP_BACKGROUND
Just checking for !s->last_picture.f is enough -- s->last_picture.f is set if and only if s->last_picture.f->data[0] is set. - Andreas
On Sat, Apr 27, 2024 at 11:36:30AM +0200, Andreas Rheinhardt wrote:
Michael Niedermayer:
Fixes: NULL pointer dereference Fixes: 68184/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-4926478069334016
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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c index f7751223b81..b24bfa248dc 100644 --- a/libavcodec/pngdec.c +++ b/libavcodec/pngdec.c @@ -1218,7 +1218,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s, return AVERROR_INVALIDDATA; }
- if ((sequence_number == 0 || !s->last_picture.f->data[0]) && + if ((sequence_number == 0 || !s->last_picture.f || !s->last_picture.f->data[0]) && dispose_op == APNG_DISPOSE_OP_PREVIOUS) { // No previous frame to revert to for the first frame // Spec says to just treat it as a APNG_DISPOSE_OP_BACKGROUND
Just checking for !s->last_picture.f is enough -- s->last_picture.f is set if and only if s->last_picture.f->data[0] is set.
ok will apply with that simplified thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is a danger to trust the dream we wish for rather than the science we have, -- Dr. Kenneth Brown
participants (3)
-
Andreas Rheinhardt -
James Almer -
Michael Niedermayer