[vlc-devel] [PATCH] demux/aiff: fix integer overflow leading to infinite loop

[Rémi Denis-Courmont]

Le lauantaina 29. lokakuuta 2016, 9.52.48 EEST Filip Roséen a écrit :
> Hi Remi,
> 
> On 2016-10-29 10:51, Rémi Denis-Courmont wrote:
> > Le lauantaina 29. lokakuuta 2016, 0.10.41 EEST Filip Roséen a écrit :
> > > The previous implementation could potentially overflow the uint32_t
> > > holding the data-size when adding the size of the mandatory header, in
> > > order to consume the entire chunk. If that happened we could end up in
> > > an infinite loop (given that we are not guaranteed to make progress).
> > > 
> > > These changes fixes the issue by introducing another variable that is
> > > only used for the purpose of storing the chunk (total) size.
> > > 
> > > fixes #17562
> > > 
> > > --
> > > 
> > > I will clean-up the implementation shortly, but thought I would
> > > address this immidiate issue as soon as possible.
> > > 
> > > The demuxer can be made a lot simpler, and a lot safer.
> > > ---
> > > 
> > >  modules/demux/aiff.c | 16 ++++++----------
> > >  1 file changed, 6 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/modules/demux/aiff.c b/modules/demux/aiff.c
> > > index bdd0308..02cc711 100644
> > > --- a/modules/demux/aiff.c
> > > +++ b/modules/demux/aiff.c
> > > @@ -123,14 +123,13 @@ static int Open( vlc_object_t *p_this )
> > > 
> > >      for( ;; )
> > >      {
> > > 
> > > -        uint32_t i_size;
> > > -
> > > 
> > >          if( vlc_stream_Peek( p_demux->s, &p_peek, 8 ) < 8 )
> > >          
> > >              goto error;
> > > 
> > > -        i_size = GetDWBE( &p_peek[4] );
> > > +        uint32_t i_data_size = GetDWBE( &p_peek[4] );
> > > +        ssize_t i_chunk_size = ( i_data_size + 8 + 1 ) ^ 1;
> > 
> > On 32-bits, this is undefined overflow if i_data_size > 0x7ffffff6.
> 
> Yeah, I replied saying that I am working on a more extensive fix of
> the issue (you should see patches on the mailing list shortly)!

More extensive fix? It looks like a simple integer overflow.

In general, I am all for clean-ups, but in this case, I think that a simple 
fix that can be backported to VLC 2.2 is desirable.


-- 
Rémi Denis-Courmont
Nonsponsored VLC developer
http://www.remlab.net/CV.pdf