[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 11:31:42PM +0300, Uoti Urpala wrote:
> On Sat, 2017-06-10 at 16:18 +0200, Michael Niedermayer wrote:
> > 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.
> 
> It looks like the current code has quadratic time requirements. Is
> everything else in FFmpeg actually guaranteed to not need quadratic
> time? Can anything really rely on FFmpeg decoding of hostile data not
> taking a long time?

There are currently 16 open timeout bugs in ffmpeg oss fuzz.
Iam interrested in fixing these. I dont know if there are more but thats
what ossfuzz found so far.
Some of these may be true infinite loops,

I admit fully my interrest is in fixing these issues and not in asking
how many more there are. Iam interrested even if there are codecs
for which it cannot be fixed. Some users only need a subset of codecs


> To the degree that it would be reasonable to have a
> system using FFmpeg on untrusted data without timeouts or capacity to
> kill the process? To me being slow on malicious data doesn't seem like
> a real security issue.

If possible exterenal limits on time and memory are always a good idea.


> 
> 
> > Do you have a better idea than the next_closep code to fix this ?
> > If not, do you agree that we push this fix ?
> 
> Since the slow behavior seems unlikely to occur except with
> intentionally malicious or completely corrupted data, I'm not sure it's
> worth actually fixing it. But I think it could be done somewhat more
> cleanly as follows:
> Instead of using scanf to find matches for "{Y:xxx}" or "{\xxx}", where
> "xxx" is arbitrarily long, match for "{Y:" or "{\", and then do the
> "skip until next }" as a separate step after you've confirmed a match.
> Then you can optimize that to avoid quadratic behavior by setting a
> flag when you find no "}", and not searching again if it's already set.

ill send a patch that does that.


> 
> Also, the current code seems buggy, or at least I don't see why you'd
> want it to behave like it now does. It skips "{\xxx}" tags when the
> "an" variable is not set to 1. I assume the intent was to only allow
> the first "\an" tag through. But as implemented now it seems to allow
> ANY "{\xxx}" tags through after the first \an and before possible
> second one. I don't think that's intentional?

iam not the author of the original code


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

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- 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/20170611/0193800d/attachment.sig>