| Message ID | 20240426235211.3718252-5-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avcodec/pngdec: Check last AVFrame before deref | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
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 [...]
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 ?
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;
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(+)