[FFmpeg-devel] [PATCH 4/7] avformat/nistspheredec: Check bits_per_coded_sample and channels

Michael Niedermayer michael at niedermayer.cc
Thu Jan 21 20:35:30 EET 2021


On Wed, Jan 20, 2021 at 09:41:49AM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2021-01-19 19:00:47)
> > On Mon, Jan 18, 2021 at 09:06:10PM +0100, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2021-01-17 00:07:26)
> > > > Fixes: signed integer overflow: 80 * 92233009 cannot be represented in type 'int'
> > > > Fixes: 26910/clusterfuzz-testcase-minimized-ffmpeg_dem_NISTSPHERE_fuzzer-6669100654919680
> > > > 
> > > > 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/nistspheredec.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c
> > > > index 079369929f..fec5c88892 100644
> > > > --- a/libavformat/nistspheredec.c
> > > > +++ b/libavformat/nistspheredec.c
> > > > @@ -80,7 +80,9 @@ static int nist_read_header(AVFormatContext *s)
> > > >  
> > > >              avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
> > > >  
> > > > -            st->codecpar->block_align = st->codecpar->bits_per_coded_sample * st->codecpar->channels / 8;
> > > > +            if (st->codecpar->bits_per_coded_sample * (uint64_t)st->codecpar->channels / 8 > INT_MAX)
> > > > +                return AVERROR_INVALIDDATA;
> > > > +            st->codecpar->block_align = st->codecpar->bits_per_coded_sample * (uint64_t)st->codecpar->channels / 8;
> > > >  
> > > >              if (avio_tell(s->pb) > header_size)
> > > >                  return AVERROR_INVALIDDATA;
> > > > -- 
> > > > 2.17.1
> > > 
> > > Both bits_per_coded_sample and channels seem to be arbitrary ints, so
> > > you are assuming sizeof(int) < sizeof(int64)
> > 
> > I wonder if we shouldnt limit these 2 fields in a platform independant way
> > But if we dont then the change below should do more correct checks
> 
> I would be in favor of limiting them both to 16 bits, that should be way
> beyond anything needed by any reasonable use case.
> 
> > 
> > @@ -80,7 +80,11 @@ static int nist_read_header(AVFormatContext *s)
> >  
> >              avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
> >  
> > -            st->codecpar->block_align = st->codecpar->bits_per_coded_sample * st->codecpar->channels / 8;
> > +            if (st->codecpar->bits_per_coded_sample < 0 || st->codecpar->channels <= 0 ||
> > +                st->codecpar->bits_per_coded_sample > UINT64_MAX / st->codecpar->channels ||
> > +                st->codecpar->bits_per_coded_sample * (uint64_t)st->codecpar->channels / 8 > INT_MAX)
> > +                return AVERROR_INVALIDDATA;
> > +            st->codecpar->block_align = st->codecpar->bits_per_coded_sample * (uint64_t)st->codecpar->channels / 8;
> >  
> >              if (avio_tell(s->pb) > header_size)
> >                  return AVERROR_INVALIDDATA;
> 
> This looks ok.
> 
> Though it would be even better to sanity-check each value for
> zero/negative right after the sscanf() that reads it (not insisting you
> do it).

will apply with these changes

thx

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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/20210121/0ceecfbb/attachment.sig>


More information about the ffmpeg-devel mailing list