[FFmpeg-devel] [PATCH 6/6] avformat/hls: Fix DoS due to infinite loop

[Michael Niedermayer]

On Fri, Aug 25, 2017 at 01:03:35PM +0200, wm4 wrote:
> On Fri, 25 Aug 2017 11:59:54 +0200
> Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
> > On Fri, Aug 25, 2017 at 10:08:23AM +0200, wm4 wrote:
> > > On Fri, 25 Aug 2017 01:15:32 +0200
> > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > >   
> > > > Fixes: loop.m3u
> > > > 
> > > > The max iteration count of 10000 is arbitrary and ideas for a better solution are welcome
> > > > 
> > > > Found-by: Xiaohei and Wangchu from Alibaba Security Team
> > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > ---
> > > >  libavformat/hls.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/libavformat/hls.c b/libavformat/hls.c
> > > > index 01731bd36b..26f4ebd965 100644
> > > > --- a/libavformat/hls.c
> > > > +++ b/libavformat/hls.c
> > > > @@ -1263,6 +1263,7 @@ static int read_data(void *opaque, uint8_t *buf, int buf_size)
> > > >      HLSContext *c = v->parent->priv_data;
> > > >      int ret, i;
> > > >      int just_opened = 0;
> > > > +    int reload_count = 0;
> > > >  
> > > >  restart:
> > > >      if (!v->needed)
> > > > @@ -1294,6 +1295,9 @@ restart:
> > > >          reload_interval = default_reload_interval(v);
> > > >  
> > > >  reload:
> > > > +        reload_count++;
> > > > +        if (reload_count > 10000)
> > > > +            return AVERROR_EOF;
> > > >          if (!v->finished &&
> > > >              av_gettime_relative() - v->last_load_time >= reload_interval) {
> > > >              if ((ret = parse_playlist(c, v->url, v, NULL)) < 0) {  
> > > 
> > > Why 1000?  
> > 
> > Please read the commit message at the top of the mail you reply to,
> > that is explained in it
> > 
> > 
> > > 
> > > This patch is unnecessary. The interrupt callback can break out of this
> > > loop anyway on user request. Or is this patch again intended for
> > > transcode servers with hilariously insecure amateurish configuration,
> > > like most of these patches seem to be?  
> > 
> > Do you object to this issue being fixed ?
> 

> I content that there is an issue. It was found by a fuzzer, and has no
> security implications.

I belive you should CC the security researchers who found
the issue.
Also to repeat, my oppinion is that this is a security issue and
requires a fix.


> 
> > In which case i will of course respect your veto and leave the issue
> > open.
> > Otherwise i will here note that i disagree with your oppinion.
> > 
> > And will resubmit a patch with the changes requested by the maintainer
> 
> A user option is even worse, because it creates API for a thing that
> doesn't matter.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"Nothing to hide" only works if the folks in power share the values of
you and everyone you know entirely and always will -- Tom Scott

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170825/09524757/attachment.sig>