[FFmpeg-devel] [PATCH 1/2] avformat/vividas: Check packet size

[Michael Niedermayer]

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 at 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220929/0a42e0b9/attachment.sig>