| Message ID | 20220918171410.31835-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/13] avformat/flvdec: Use 64bit for sum_flv_tag_size | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
sön 2022-09-18 klockan 19:13 +0200 skrev Michael Niedermayer: > Fixes: signed integer overflow: 9223372036854775807 - -2146905566 > cannot be represented in type 'long' > Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer- > 6570996594769920 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/mxfdec.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > index e63e803aa56..da81fea3bc1 100644 > --- a/libavformat/mxfdec.c > +++ b/libavformat/mxfdec.c > @@ -3681,6 +3681,7 @@ static int mxf_read_header(AVFormatContext *s) > KLVPacket klv; > int64_t essence_offset = 0; > int ret; > + int64_t run_in; > > mxf->last_forward_tell = INT64_MAX; > > @@ -3690,7 +3691,10 @@ static int mxf_read_header(AVFormatContext *s) > } > avio_seek(s->pb, -14, SEEK_CUR); > mxf->fc = s; > - mxf->run_in = avio_tell(s->pb); > + run_in = avio_tell(s->pb); > + if (run_in < 0 || run_in != (int)run_in) run_in > INT_MAX is more clear It strikes me that run_in is also used in lots of places in the demuxer without checking for overflow /Tomas
tis 2022-09-20 klockan 13:07 +0200 skrev Tomas Härdin: > sön 2022-09-18 klockan 19:13 +0200 skrev Michael Niedermayer: > > Fixes: signed integer overflow: 9223372036854775807 - -2146905566 > > cannot be represented in type 'long' > > Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer- > > 6570996594769920 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/mxfdec.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > > index e63e803aa56..da81fea3bc1 100644 > > --- a/libavformat/mxfdec.c > > +++ b/libavformat/mxfdec.c > > @@ -3681,6 +3681,7 @@ static int mxf_read_header(AVFormatContext > > *s) > > KLVPacket klv; > > int64_t essence_offset = 0; > > int ret; > > + int64_t run_in; > > > > mxf->last_forward_tell = INT64_MAX; > > > > @@ -3690,7 +3691,10 @@ static int mxf_read_header(AVFormatContext > > *s) > > } > > avio_seek(s->pb, -14, SEEK_CUR); > > mxf->fc = s; > > - mxf->run_in = avio_tell(s->pb); > > + run_in = avio_tell(s->pb); > > + if (run_in < 0 || run_in != (int)run_in) > > run_in > INT_MAX is more clear > > It strikes me that run_in is also used in lots of places in the > demuxer > without checking for overflow I went and checked S377m and the run-in sequence "shall be less than 65536 bytes long". Both the 2004 and 2009 version of the spec agree on this. So we should reject run_in >= 65536, and mxf_probe() should be similarly adjusted. /Tomas
On Tue, Sep 20, 2022 at 01:20:00PM +0200, Tomas Härdin wrote: > tis 2022-09-20 klockan 13:07 +0200 skrev Tomas Härdin: > > sön 2022-09-18 klockan 19:13 +0200 skrev Michael Niedermayer: > > > Fixes: signed integer overflow: 9223372036854775807 - -2146905566 > > > cannot be represented in type 'long' > > > Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer- > > > 6570996594769920 > > > > > > Found-by: continuous fuzzing process > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavformat/mxfdec.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > > > index e63e803aa56..da81fea3bc1 100644 > > > --- a/libavformat/mxfdec.c > > > +++ b/libavformat/mxfdec.c > > > @@ -3681,6 +3681,7 @@ static int mxf_read_header(AVFormatContext > > > *s) > > > KLVPacket klv; > > > int64_t essence_offset = 0; > > > int ret; > > > + int64_t run_in; > > > > > > mxf->last_forward_tell = INT64_MAX; > > > > > > @@ -3690,7 +3691,10 @@ static int mxf_read_header(AVFormatContext > > > *s) > > > } > > > avio_seek(s->pb, -14, SEEK_CUR); > > > mxf->fc = s; > > > - mxf->run_in = avio_tell(s->pb); > > > + run_in = avio_tell(s->pb); > > > + if (run_in < 0 || run_in != (int)run_in) > > > > run_in > INT_MAX is more clear > > > > It strikes me that run_in is also used in lots of places in the > > demuxer > > without checking for overflow > > I went and checked S377m and the run-in sequence "shall be less than > 65536 bytes long". Both the 2004 and 2009 version of the spec agree on > this. So we should reject run_in >= 65536, and mxf_probe() should be > similarly adjusted. ok, will do thx for checking i will change the patch by: @@ -3717,7 +3717,7 @@ static int mxf_read_header(AVFormatContext *s) avio_seek(s->pb, -14, SEEK_CUR); mxf->fc = s; run_in = avio_tell(s->pb); - if (run_in < 0 || run_in != (int)run_in) + if (run_in < 0 || run_in > 65535) return AVERROR_INVALIDDATA; mxf->run_in = run_in; @@ -4125,7 +4125,7 @@ static int mxf_read_close(AVFormatContext *s) static int mxf_probe(const AVProbeData *p) { const uint8_t *bufp = p->buf; - const uint8_t *end = p->buf + p->buf_size; + const uint8_t *end = p->buf + FFMIN(p->buf_size, 65536 + sizeof(mxf_header_partition_pack_key)); if (p->buf_size < sizeof(mxf_header_partition_pack_key)) return 0; [...]
ons 2022-09-21 klockan 11:35 +0200 skrev Michael Niedermayer: > On Tue, Sep 20, 2022 at 01:20:00PM +0200, Tomas Härdin wrote: > > tis 2022-09-20 klockan 13:07 +0200 skrev Tomas Härdin: > > > sön 2022-09-18 klockan 19:13 +0200 skrev Michael Niedermayer: > > > > Fixes: signed integer overflow: 9223372036854775807 - - > > > > 2146905566 > > > > cannot be represented in type 'long' > > > > Fixes: 50993/clusterfuzz-testcase-minimized- > > > > ffmpeg_dem_MXF_fuzzer- > > > > 6570996594769920 > > > > > > > > Found-by: continuous fuzzing process > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavformat/mxfdec.c | 6 +++++- > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > > > > index e63e803aa56..da81fea3bc1 100644 > > > > --- a/libavformat/mxfdec.c > > > > +++ b/libavformat/mxfdec.c > > > > @@ -3681,6 +3681,7 @@ static int > > > > mxf_read_header(AVFormatContext > > > > *s) > > > > KLVPacket klv; > > > > int64_t essence_offset = 0; > > > > int ret; > > > > + int64_t run_in; > > > > > > > > mxf->last_forward_tell = INT64_MAX; > > > > > > > > @@ -3690,7 +3691,10 @@ static int > > > > mxf_read_header(AVFormatContext > > > > *s) > > > > } > > > > avio_seek(s->pb, -14, SEEK_CUR); > > > > mxf->fc = s; > > > > - mxf->run_in = avio_tell(s->pb); > > > > + run_in = avio_tell(s->pb); > > > > + if (run_in < 0 || run_in != (int)run_in) > > > > > > run_in > INT_MAX is more clear > > > > > > It strikes me that run_in is also used in lots of places in the > > > demuxer > > > without checking for overflow > > > > I went and checked S377m and the run-in sequence "shall be less > > than > > 65536 bytes long". Both the 2004 and 2009 version of the spec agree > > on > > this. So we should reject run_in >= 65536, and mxf_probe() should > > be > > similarly adjusted. > > ok, will do > > thx for checking > > i will change the patch by: > @@ -3717,7 +3717,7 @@ static int mxf_read_header(AVFormatContext *s) > avio_seek(s->pb, -14, SEEK_CUR); > mxf->fc = s; > run_in = avio_tell(s->pb); > - if (run_in < 0 || run_in != (int)run_in) > + if (run_in < 0 || run_in > 65535) Let's avoid magic numbers: #define RUN_IN_MAX 65535 // S377m-2004 section 5.5 and S377-1-2009 section 6.5 > return AVERROR_INVALIDDATA; > mxf->run_in = run_in; > > @@ -4125,7 +4125,7 @@ static int mxf_read_close(AVFormatContext *s) > > static int mxf_probe(const AVProbeData *p) { > const uint8_t *bufp = p->buf; > - const uint8_t *end = p->buf + p->buf_size; > + const uint8_t *end = p->buf + FFMIN(p->buf_size, 65536 + > sizeof(mxf_header_partition_pack_key)); Seems correct. I tested this by prefixing fate-suite/mxf/Meridian- Apple_ProResProxy-HDR10.mxf with 65535 NUL bytes which worked fine, and 65536 NUL bytes which does not probe as MXF as expected /Tomas
ons 2022-09-21 klockan 11:35 +0200 skrev Michael Niedermayer: > On Tue, Sep 20, 2022 at 01:20:00PM +0200, Tomas Härdin wrote: > > tis 2022-09-20 klockan 13:07 +0200 skrev Tomas Härdin: > > > sön 2022-09-18 klockan 19:13 +0200 skrev Michael Niedermayer: > > > > Fixes: signed integer overflow: 9223372036854775807 - - > > > > 2146905566 > > > > cannot be represented in type 'long' > > > > Fixes: 50993/clusterfuzz-testcase-minimized- > > > > ffmpeg_dem_MXF_fuzzer- > > > > 6570996594769920 > > > > > > > > Found-by: continuous fuzzing process > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavformat/mxfdec.c | 6 +++++- > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > > > > index e63e803aa56..da81fea3bc1 100644 > > > > --- a/libavformat/mxfdec.c > > > > +++ b/libavformat/mxfdec.c > > > > @@ -3681,6 +3681,7 @@ static int > > > > mxf_read_header(AVFormatContext > > > > *s) > > > > KLVPacket klv; > > > > int64_t essence_offset = 0; > > > > int ret; > > > > + int64_t run_in; > > > > > > > > mxf->last_forward_tell = INT64_MAX; > > > > > > > > @@ -3690,7 +3691,10 @@ static int > > > > mxf_read_header(AVFormatContext > > > > *s) > > > > } > > > > avio_seek(s->pb, -14, SEEK_CUR); > > > > mxf->fc = s; > > > > - mxf->run_in = avio_tell(s->pb); > > > > + run_in = avio_tell(s->pb); > > > > + if (run_in < 0 || run_in != (int)run_in) > > > > > > run_in > INT_MAX is more clear > > > > > > It strikes me that run_in is also used in lots of places in the > > > demuxer > > > without checking for overflow > > > > I went and checked S377m and the run-in sequence "shall be less > > than > > 65536 bytes long". Both the 2004 and 2009 version of the spec agree > > on > > this. So we should reject run_in >= 65536, and mxf_probe() should > > be > > similarly adjusted. > > ok, will do > > thx for checking > > i will change the patch by: > @@ -3717,7 +3717,7 @@ static int mxf_read_header(AVFormatContext *s) > avio_seek(s->pb, -14, SEEK_CUR); Oh and also the call to mxf_read_sync() could be supplied with a maximum number of bytes to read, allowing the code to bail out faster /Tomas
On Wed, Sep 21, 2022 at 03:23:21PM +0200, Tomas Härdin wrote: > ons 2022-09-21 klockan 11:35 +0200 skrev Michael Niedermayer: > > On Tue, Sep 20, 2022 at 01:20:00PM +0200, Tomas Härdin wrote: > > > tis 2022-09-20 klockan 13:07 +0200 skrev Tomas Härdin: > > > > sön 2022-09-18 klockan 19:13 +0200 skrev Michael Niedermayer: > > > > > Fixes: signed integer overflow: 9223372036854775807 - - > > > > > 2146905566 > > > > > cannot be represented in type 'long' > > > > > Fixes: 50993/clusterfuzz-testcase-minimized- > > > > > ffmpeg_dem_MXF_fuzzer- > > > > > 6570996594769920 > > > > > > > > > > Found-by: continuous fuzzing process > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > --- > > > > > libavformat/mxfdec.c | 6 +++++- > > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > > > > > index e63e803aa56..da81fea3bc1 100644 > > > > > --- a/libavformat/mxfdec.c > > > > > +++ b/libavformat/mxfdec.c > > > > > @@ -3681,6 +3681,7 @@ static int > > > > > mxf_read_header(AVFormatContext > > > > > *s) > > > > > KLVPacket klv; > > > > > int64_t essence_offset = 0; > > > > > int ret; > > > > > + int64_t run_in; > > > > > > > > > > mxf->last_forward_tell = INT64_MAX; > > > > > > > > > > @@ -3690,7 +3691,10 @@ static int > > > > > mxf_read_header(AVFormatContext > > > > > *s) > > > > > } > > > > > avio_seek(s->pb, -14, SEEK_CUR); > > > > > mxf->fc = s; > > > > > - mxf->run_in = avio_tell(s->pb); > > > > > + run_in = avio_tell(s->pb); > > > > > + if (run_in < 0 || run_in != (int)run_in) > > > > > > > > run_in > INT_MAX is more clear > > > > > > > > It strikes me that run_in is also used in lots of places in the > > > > demuxer > > > > without checking for overflow > > > > > > I went and checked S377m and the run-in sequence "shall be less > > > than > > > 65536 bytes long". Both the 2004 and 2009 version of the spec agree > > > on > > > this. So we should reject run_in >= 65536, and mxf_probe() should > > > be > > > similarly adjusted. > > > > ok, will do > > > > thx for checking > > > > i will change the patch by: > > @@ -3717,7 +3717,7 @@ static int mxf_read_header(AVFormatContext *s) > > avio_seek(s->pb, -14, SEEK_CUR); > > mxf->fc = s; > > run_in = avio_tell(s->pb); > > - if (run_in < 0 || run_in != (int)run_in) > > + if (run_in < 0 || run_in > 65535) > > Let's avoid magic numbers: > #define RUN_IN_MAX 65535 // S377m-2004 section 5.5 and S377-1-2009 > section 6.5 sure, will use this thx [...]
On Wed, Sep 21, 2022 at 03:25:33PM +0200, Tomas Härdin wrote: > ons 2022-09-21 klockan 11:35 +0200 skrev Michael Niedermayer: > > On Tue, Sep 20, 2022 at 01:20:00PM +0200, Tomas Härdin wrote: > > > tis 2022-09-20 klockan 13:07 +0200 skrev Tomas Härdin: > > > > sön 2022-09-18 klockan 19:13 +0200 skrev Michael Niedermayer: > > > > > Fixes: signed integer overflow: 9223372036854775807 - - > > > > > 2146905566 > > > > > cannot be represented in type 'long' > > > > > Fixes: 50993/clusterfuzz-testcase-minimized- > > > > > ffmpeg_dem_MXF_fuzzer- > > > > > 6570996594769920 > > > > > > > > > > Found-by: continuous fuzzing process > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > --- > > > > > libavformat/mxfdec.c | 6 +++++- > > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > > > > > index e63e803aa56..da81fea3bc1 100644 > > > > > --- a/libavformat/mxfdec.c > > > > > +++ b/libavformat/mxfdec.c > > > > > @@ -3681,6 +3681,7 @@ static int > > > > > mxf_read_header(AVFormatContext > > > > > *s) > > > > > KLVPacket klv; > > > > > int64_t essence_offset = 0; > > > > > int ret; > > > > > + int64_t run_in; > > > > > > > > > > mxf->last_forward_tell = INT64_MAX; > > > > > > > > > > @@ -3690,7 +3691,10 @@ static int > > > > > mxf_read_header(AVFormatContext > > > > > *s) > > > > > } > > > > > avio_seek(s->pb, -14, SEEK_CUR); > > > > > mxf->fc = s; > > > > > - mxf->run_in = avio_tell(s->pb); > > > > > + run_in = avio_tell(s->pb); > > > > > + if (run_in < 0 || run_in != (int)run_in) > > > > > > > > run_in > INT_MAX is more clear > > > > > > > > It strikes me that run_in is also used in lots of places in the > > > > demuxer > > > > without checking for overflow > > > > > > I went and checked S377m and the run-in sequence "shall be less > > > than > > > 65536 bytes long". Both the 2004 and 2009 version of the spec agree > > > on > > > this. So we should reject run_in >= 65536, and mxf_probe() should > > > be > > > similarly adjusted. > > > > ok, will do > > > > thx for checking > > > > i will change the patch by: > > @@ -3717,7 +3717,7 @@ static int mxf_read_header(AVFormatContext *s) > > avio_seek(s->pb, -14, SEEK_CUR); > > Oh and also the call to mxf_read_sync() could be supplied with a > maximum number of bytes to read, allowing the code to bail out faster I agree, but that should be seperate from the initial bugfix thx [...]
On Wed, 21 Sep 2022, Tomas Härdin wrote: > ons 2022-09-21 klockan 11:35 +0200 skrev Michael Niedermayer: >> On Tue, Sep 20, 2022 at 01:20:00PM +0200, Tomas Härdin wrote: >> > tis 2022-09-20 klockan 13:07 +0200 skrev Tomas Härdin: >> > > sön 2022-09-18 klockan 19:13 +0200 skrev Michael Niedermayer: >> > > > Fixes: signed integer overflow: 9223372036854775807 - - >> > > > 2146905566 >> > > > cannot be represented in type 'long' >> > > > Fixes: 50993/clusterfuzz-testcase-minimized- >> > > > ffmpeg_dem_MXF_fuzzer- >> > > > 6570996594769920 >> > > > >> > > > Found-by: continuous fuzzing process >> > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> > > > --- >> > > > libavformat/mxfdec.c | 6 +++++- >> > > > 1 file changed, 5 insertions(+), 1 deletion(-) >> > > > >> > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c >> > > > index e63e803aa56..da81fea3bc1 100644 >> > > > --- a/libavformat/mxfdec.c >> > > > +++ b/libavformat/mxfdec.c >> > > > @@ -3681,6 +3681,7 @@ static int >> > > > mxf_read_header(AVFormatContext >> > > > *s) >> > > > KLVPacket klv; >> > > > int64_t essence_offset = 0; >> > > > int ret; >> > > > + int64_t run_in; >> > > > >> > > > mxf->last_forward_tell = INT64_MAX; >> > > > >> > > > @@ -3690,7 +3691,10 @@ static int >> > > > mxf_read_header(AVFormatContext >> > > > *s) >> > > > } >> > > > avio_seek(s->pb, -14, SEEK_CUR); >> > > > mxf->fc = s; >> > > > - mxf->run_in = avio_tell(s->pb); >> > > > + run_in = avio_tell(s->pb); >> > > > + if (run_in < 0 || run_in != (int)run_in) >> > > >> > > run_in > INT_MAX is more clear >> > > >> > > It strikes me that run_in is also used in lots of places in the >> > > demuxer >> > > without checking for overflow >> > >> > I went and checked S377m and the run-in sequence "shall be less >> > than >> > 65536 bytes long". Both the 2004 and 2009 version of the spec agree >> > on >> > this. So we should reject run_in >= 65536, and mxf_probe() should >> > be >> > similarly adjusted. >> >> ok, will do >> >> thx for checking >> >> i will change the patch by: >> @@ -3717,7 +3717,7 @@ static int mxf_read_header(AVFormatContext *s) >> avio_seek(s->pb, -14, SEEK_CUR); > > Oh and also the call to mxf_read_sync() could be supplied with a > maximum number of bytes to read, allowing the code to bail out faster Yeah, I wanted to suggest this as well. And please allow 65536-byte run-in, even if that is not strictly allowed by the current standard, because the MXF book has no problem with that: (run-in is) "...a sequence of up to 65536 bytes at the front of the file that precedes the first partition pack..." Thanks, Marton
ons 2022-09-21 klockan 20:56 +0200 skrev Marton Balint: > > > On Wed, 21 Sep 2022, Tomas Härdin wrote: > > > ons 2022-09-21 klockan 11:35 +0200 skrev Michael Niedermayer: > > > On Tue, Sep 20, 2022 at 01:20:00PM +0200, Tomas Härdin wrote: > > > > tis 2022-09-20 klockan 13:07 +0200 skrev Tomas Härdin: > > > > > sön 2022-09-18 klockan 19:13 +0200 skrev Michael Niedermayer: > > > > > > Fixes: signed integer overflow: 9223372036854775807 - - > > > > > > 2146905566 > > > > > > cannot be represented in type 'long' > > > > > > Fixes: 50993/clusterfuzz-testcase-minimized- > > > > > > ffmpeg_dem_MXF_fuzzer- > > > > > > 6570996594769920 > > > > > > > > > > > > Found-by: continuous fuzzing process > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > --- > > > > > > libavformat/mxfdec.c | 6 +++++- > > > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > > > > > > index e63e803aa56..da81fea3bc1 100644 > > > > > > --- a/libavformat/mxfdec.c > > > > > > +++ b/libavformat/mxfdec.c > > > > > > @@ -3681,6 +3681,7 @@ static int > > > > > > mxf_read_header(AVFormatContext > > > > > > *s) > > > > > > KLVPacket klv; > > > > > > int64_t essence_offset = 0; > > > > > > int ret; > > > > > > + int64_t run_in; > > > > > > > > > > > > mxf->last_forward_tell = INT64_MAX; > > > > > > > > > > > > @@ -3690,7 +3691,10 @@ static int > > > > > > mxf_read_header(AVFormatContext > > > > > > *s) > > > > > > } > > > > > > avio_seek(s->pb, -14, SEEK_CUR); > > > > > > mxf->fc = s; > > > > > > - mxf->run_in = avio_tell(s->pb); > > > > > > + run_in = avio_tell(s->pb); > > > > > > + if (run_in < 0 || run_in != (int)run_in) > > > > > > > > > > run_in > INT_MAX is more clear > > > > > > > > > > It strikes me that run_in is also used in lots of places in > > > > > the > > > > > demuxer > > > > > without checking for overflow > > > > > > > > I went and checked S377m and the run-in sequence "shall be less > > > > than > > > > 65536 bytes long". Both the 2004 and 2009 version of the spec > > > > agree > > > > on > > > > this. So we should reject run_in >= 65536, and mxf_probe() > > > > should > > > > be > > > > similarly adjusted. > > > > > > ok, will do > > > > > > thx for checking > > > > > > i will change the patch by: > > > @@ -3717,7 +3717,7 @@ static int mxf_read_header(AVFormatContext > > > *s) > > > avio_seek(s->pb, -14, SEEK_CUR); > > > > Oh and also the call to mxf_read_sync() could be supplied with a > > maximum number of bytes to read, allowing the code to bail out > > faster > > Yeah, I wanted to suggest this as well. And please allow 65536-byte > run-in, even if that is not strictly allowed by the current standard, > because the MXF book has no problem with that: > > (run-in is) "...a sequence of up to 65536 bytes at the front of the > file > that precedes the first partition pack..." The MXF Book is not S377m. It also has other errors in it IIRC /Tomas
diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index e63e803aa56..da81fea3bc1 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -3681,6 +3681,7 @@ static int mxf_read_header(AVFormatContext *s) KLVPacket klv; int64_t essence_offset = 0; int ret; + int64_t run_in; mxf->last_forward_tell = INT64_MAX; @@ -3690,7 +3691,10 @@ static int mxf_read_header(AVFormatContext *s) } avio_seek(s->pb, -14, SEEK_CUR); mxf->fc = s; - mxf->run_in = avio_tell(s->pb); + run_in = avio_tell(s->pb); + if (run_in < 0 || run_in != (int)run_in) + return AVERROR_INVALIDDATA; + mxf->run_in = run_in; mxf_read_random_index_pack(s);
Fixes: signed integer overflow: 9223372036854775807 - -2146905566 cannot be represented in type 'long' Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-6570996594769920 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mxfdec.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)