[FFmpeg-devel] [PATCH 1/2] avcodec/htmlsubtitles: Protect very slow redundant sscanf() calls by optimized use of strchr()

[Michael Niedermayer]

On Sat, Jun 10, 2017 at 01:36:29PM +0200, wm4 wrote:
> On Fri, 9 Jun 2017 19:39:36 +0200
> Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
> > On Fri, Jun 09, 2017 at 04:32:41PM +0200, wm4 wrote:
> > > On Thu,  8 Jun 2017 23:53:55 +0200
> > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > >   
> > > > Fixes Timeout
> > > > Fixes: 2127/clusterfuzz-testcase-minimized-6595787859427328
> > > > 
> > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > ---
> > > >  libavcodec/htmlsubtitles.c | 12 ++++++++++--
> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/libavcodec/htmlsubtitles.c b/libavcodec/htmlsubtitles.c
> > > > index 16295daa0c..ba4f269b3f 100644
> > > > --- a/libavcodec/htmlsubtitles.c
> > > > +++ b/libavcodec/htmlsubtitles.c
> > > > @@ -56,6 +56,7 @@ int ff_htmlmarkup_to_ass(void *log_ctx, AVBPrint *dst, const char *in)
> > > >      char *param, buffer[128], tmp[128];
> > > >      int len, tag_close, sptr = 1, line_start = 1, an = 0, end = 0;
> > > >      SrtStack stack[16];
> > > > +    const char *next_closep = NULL;
> > > >  
> > > >      stack[0].tag[0] = 0;
> > > >      strcpy(stack[0].param[PARAM_SIZE],  "{\\fs}");
> > > > @@ -83,8 +84,15 @@ int ff_htmlmarkup_to_ass(void *log_ctx, AVBPrint *dst, const char *in)
> > > >                          and all microdvd like styles such as {Y:xxx} */
> > > >              len = 0;
> > > >              an += sscanf(in, "{\\an%*1u}%n", &len) >= 0 && len > 0;
> > > > -            if ((an != 1 && (len = 0, sscanf(in, "{\\%*[^}]}%n", &len) >= 0 && len > 0)) ||
> > > > -                (len = 0, sscanf(in, "{%*1[CcFfoPSsYy]:%*[^}]}%n", &len) >= 0 && len > 0)) {
> > > > +
> > > > +            if(!next_closep || next_closep <= in) {
> > > > +                next_closep = strchr(in+1, '}');
> > > > +                if (!next_closep)
> > > > +                    next_closep = in + strlen(in);
> > > > +            }
> > > > +
> > > > +            if (*next_closep == '}' && (an != 1 && (len = 0, sscanf(in, "{\\%*[^}]}%n", &len) >= 0 && len > 0)) ||
> > > > +                *next_closep == '}' && (len = 0, sscanf(in, "{%*1[CcFfoPSsYy]:%*[^}]}%n", &len) >= 0 && len > 0)) {
> > > >                  in += len - 1;
> > > >              } else
> > > >                  av_bprint_chars(dst, *in, 1);  
> > > 
> > > I'm not convinced that bad performance with an obscure fuzzed sample is
> > > worth the complexity increase here.  
> > 
> > If we want to make ff_htmlmarkup_to_ass() not go into near infinite
> > loops with crafted data then this or a equivalent change is likely
> > needed.
> 
> Maybe if someone starts abusing this case.

If iam not mistaken, all the issues found by oss fuzz are made
public 90 days or so after they are found. Anyone who wants to exploit
a timeout/infinite loop just needs to search for timeout and ffmpeg
and use the tescase ...

So we are guranteed that anyone who wants to exploit this has the
ability to do so as long as they can use the search mask and are able
to remux the data into whatever format they need.
And i belive this publication of issues is the right thing to do.

Do you have a better idea than the next_closep code to fix this ?
If not, do you agree that we push this fix ?


> 
> > 
> > > 
> > > I'd rather ask, why the heck is it using sscanf in the first place?  
> > 
> > I suspect the use of scanf is fewer lines of code than the alternative
> > but iam not the author of this, i dont know for sure why it was written
> > as it was.
> 
> I'd probably prefer explicit code here (as long as it's not typical
> "tricky C string processing" style), but it's hard to tell.

its a few calls to C standard lib string functions and some
straight in[1] == this && in[2] == that stuff i think


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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/20170610/80231315/attachment.sig>