| Message ID | 20220922180852.20655-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/vividas: Check packet size | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting Michael Niedermayer (2022-09-22 20:08:51) > Fixes: signed integer overflow: 119760682 - -2084600173 cannot be represented in type 'int' > Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-6745781167587328 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/vividas.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/libavformat/vividas.c b/libavformat/vividas.c > index e9954f73ed0..e8efe49a5c0 100644 > --- a/libavformat/vividas.c > +++ b/libavformat/vividas.c > @@ -683,6 +683,7 @@ static int viv_read_packet(AVFormatContext *s, > > if (viv->sb_entries[viv->current_sb_entry].flag == 0) { > uint64_t v_size = ffio_read_varlen(pb); > + int last, last_start; > > if (!viv->num_audio) > return AVERROR_INVALIDDATA; > @@ -704,14 +705,22 @@ static int viv_read_packet(AVFormatContext *s, > start = ffio_read_varlen(pb); > pcm_bytes = ffio_read_varlen(pb); > > - if (i > 0 && start == 0) > - break; > + if (i > 0) { > + if (start == 0) > + break; > + if (start < last || start - (unsigned)last > INT_MAX) What is the second condition for?
On Wed, Sep 28, 2022 at 05:16:05PM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2022-09-22 20:08:51) > > Fixes: signed integer overflow: 119760682 - -2084600173 cannot be represented in type 'int' > > Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-6745781167587328 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/vividas.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/vividas.c b/libavformat/vividas.c > > index e9954f73ed0..e8efe49a5c0 100644 > > --- a/libavformat/vividas.c > > +++ b/libavformat/vividas.c > > @@ -683,6 +683,7 @@ static int viv_read_packet(AVFormatContext *s, > > > > if (viv->sb_entries[viv->current_sb_entry].flag == 0) { > > uint64_t v_size = ffio_read_varlen(pb); > > + int last, last_start; > > > > if (!viv->num_audio) > > return AVERROR_INVALIDDATA; > > @@ -704,14 +705,22 @@ static int viv_read_packet(AVFormatContext *s, > > start = ffio_read_varlen(pb); > > pcm_bytes = ffio_read_varlen(pb); > > > > - if (i > 0 && start == 0) > > - break; > > + if (i > 0) { > > + if (start == 0) > > + break; > > + if (start < last || start - (unsigned)last > INT_MAX) > > What is the second condition for? start is signed int so are "copyies" of it "start < last" would allow a negative last with a large start the 2nd check handles that. the difference of consequtive values are stored as int later The patch tried to leave the storage types and check for it. The types could be changed or some other checks could be used I was undecided on this patch a bit too. I picked this mainly to keep changes more minimal but maybe this was not the best choice Thx [...]
Quoting Michael Niedermayer (2022-09-29 00:35:09) > On Wed, Sep 28, 2022 at 05:16:05PM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2022-09-22 20:08:51) > > > Fixes: signed integer overflow: 119760682 - -2084600173 cannot be represented in type 'int' > > > Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-6745781167587328 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavformat/vividas.c | 13 +++++++++++-- > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > diff --git a/libavformat/vividas.c b/libavformat/vividas.c > > > index e9954f73ed0..e8efe49a5c0 100644 > > > --- a/libavformat/vividas.c > > > +++ b/libavformat/vividas.c > > > @@ -683,6 +683,7 @@ static int viv_read_packet(AVFormatContext *s, > > > > > > if (viv->sb_entries[viv->current_sb_entry].flag == 0) { > > > uint64_t v_size = ffio_read_varlen(pb); > > > + int last, last_start; > > > > > > if (!viv->num_audio) > > > return AVERROR_INVALIDDATA; > > > @@ -704,14 +705,22 @@ static int viv_read_packet(AVFormatContext *s, > > > start = ffio_read_varlen(pb); > > > pcm_bytes = ffio_read_varlen(pb); > > > > > > - if (i > 0 && start == 0) > > > - break; > > > + if (i > 0) { > > > + if (start == 0) > > > + break; > > > + if (start < last || start - (unsigned)last > INT_MAX) > > > > What is the second condition for? > > start is signed int so are "copyies" of it > "start < last" would allow a negative last with a large start > the 2nd check handles that. > the difference of consequtive values are stored as int later > > The patch tried to leave the storage types and check for it. > The types could be changed or some other checks could be used > I was undecided on this patch a bit too. I picked this mainly > to keep changes more minimal but maybe this was not the best > choice Checking that start >= 0 would fix this as well, wouldn't it? I don't think it makes sense for it to be negative.
On Thu, Sep 29, 2022 at 04:10:22PM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2022-09-29 00:35:09) > > On Wed, Sep 28, 2022 at 05:16:05PM +0200, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2022-09-22 20:08:51) > > > > Fixes: signed integer overflow: 119760682 - -2084600173 cannot be represented in type 'int' > > > > Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-6745781167587328 > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavformat/vividas.c | 13 +++++++++++-- > > > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/libavformat/vividas.c b/libavformat/vividas.c > > > > index e9954f73ed0..e8efe49a5c0 100644 > > > > --- a/libavformat/vividas.c > > > > +++ b/libavformat/vividas.c > > > > @@ -683,6 +683,7 @@ static int viv_read_packet(AVFormatContext *s, > > > > > > > > if (viv->sb_entries[viv->current_sb_entry].flag == 0) { > > > > uint64_t v_size = ffio_read_varlen(pb); > > > > + int last, last_start; > > > > > > > > if (!viv->num_audio) > > > > return AVERROR_INVALIDDATA; > > > > @@ -704,14 +705,22 @@ static int viv_read_packet(AVFormatContext *s, > > > > start = ffio_read_varlen(pb); > > > > pcm_bytes = ffio_read_varlen(pb); > > > > > > > > - if (i > 0 && start == 0) > > > > - break; > > > > + if (i > 0) { > > > > + if (start == 0) > > > > + break; > > > > + if (start < last || start - (unsigned)last > INT_MAX) > > > > > > What is the second condition for? > > > > start is signed int so are "copyies" of it > > "start < last" would allow a negative last with a large start > > the 2nd check handles that. > > the difference of consequtive values are stored as int later > > > > The patch tried to leave the storage types and check for it. > > The types could be changed or some other checks could be used > > I was undecided on this patch a bit too. I picked this mainly > > to keep changes more minimal but maybe this was not the best > > choice > > Checking that start >= 0 would fix this as well, wouldn't it? i agree > I don't > think it makes sense for it to be negative. well no, but i tried to minimize the chance of breaking something so the fix ended up looking a bit wonky ill change it to check for start >= 0 thx [...]
diff --git a/libavformat/vividas.c b/libavformat/vividas.c index e9954f73ed0..e8efe49a5c0 100644 --- a/libavformat/vividas.c +++ b/libavformat/vividas.c @@ -683,6 +683,7 @@ static int viv_read_packet(AVFormatContext *s, if (viv->sb_entries[viv->current_sb_entry].flag == 0) { uint64_t v_size = ffio_read_varlen(pb); + int last, last_start; if (!viv->num_audio) return AVERROR_INVALIDDATA; @@ -704,14 +705,22 @@ static int viv_read_packet(AVFormatContext *s, start = ffio_read_varlen(pb); pcm_bytes = ffio_read_varlen(pb); - if (i > 0 && start == 0) - break; + if (i > 0) { + if (start == 0) + break; + if (start < last || start - (unsigned)last > INT_MAX) + return AVERROR_INVALIDDATA; + } viv->n_audio_subpackets = i + 1; + last = viv->audio_subpackets[i].start = start; viv->audio_subpackets[i].pcm_bytes = pcm_bytes; } + last_start = viv->audio_subpackets[viv->n_audio_subpackets].start = (int)(off - avio_tell(pb)); + if (last_start < last || last_start - (unsigned)last > INT_MAX) + return AVERROR_INVALIDDATA; viv->current_audio_subpacket = 0; } else {
Fixes: signed integer overflow: 119760682 - -2084600173 cannot be represented in type 'int' Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-6745781167587328 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/vividas.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)